Skip to content

Add Conditional Control logic to settings display for GUI#2400

Open
LinkofOrigin wants to merge 23 commits intoOoTRandomizer:Devfrom
LinkofOrigin:add-conditional-visibility-rules-to-settings
Open

Add Conditional Control logic to settings display for GUI#2400
LinkofOrigin wants to merge 23 commits intoOoTRandomizer:Devfrom
LinkofOrigin:add-conditional-visibility-rules-to-settings

Conversation

@LinkofOrigin
Copy link
Copy Markdown

@LinkofOrigin LinkofOrigin commented Apr 4, 2025

Conditional Enable/Disable Feature

GitHub Issue: #1827 (comment)

As it stands today, relationships between settings are limited to "disable only" per value. That is, if a setting is set to particular value, that value can "disable" other settings from being altered in the GUI.

This change aims to both reduce rigidity in the existing "disable" logic as well as open up the possibility of more complex logic. Not only can settings be enabled or disabled, they can also have their values updated (and potentially their visibility state changed)!

Goals

Here is a non-exhaustive list of target goals for this work:

Goal 1: Disable logic remains in-tact

The existing "disable" logic should continue to work as it does now. Further, disable logic should take priority over conditional logic if a setting would be affected by both. This serves a few purposes:

  1. This keeps the change backwards compatible, so we don't have to update every other option to use conditional rules instead. Each setting could be updated to use the conditional logic instead if there's a need to do so - the only extra change to other settings would be to remove the "disable" logic from another setting in the scenario where you no longer want that disable logic to occur. That said, replicating the "disable" logic using the new conditional logic can be overly verbose so its nice to have a simple option as well.
  2. The current assumption the app makes is that a setting should always be disabled if there is any logic that could enable it but isn't explicitly doing so. This is probably a reasonably safe assumption to make - if one setting is set such that it is wildly incompatible with or without another setting, that other setting should probably be forced to reflect that fact. This assumption could certainly be challenged and updated in the future, but this change is not attempting to do so for now.

Goal 2: More refined controls over setting relationships

We want to be able to update a setting based on the state of one or more other settings. This change allows a setting to have its value, enabled state (enabled or disabled), and visibility state (shown or hidden) updated if a condition passes. (See the Feature Breakdown and Examples sections below for more details.)

This allows for something simple like:

  • SettingA should be enabled if SettingB is set to "true"
  • SettingA should be set to a value of "ValueD" if SettingF is set to "ValueG"

This also allows more complex logic like:

  • SettingA should be shown if:
    • (SettingB is set to "true" OR SettingC is set to "peanuts") AND (SettingD is set to "false")
  • SettingC should be shown AND disabled AND set to "ValueP" if:
    • (SettingR is set to "ValueT") AND (SettingO is set to "ValueM" OR SettingO is set to "ValueN")

Feature Breakdown & Examples

Feature Breakdown
  • A setting's JSON can now be updated with a new dictionary object named "conditional_controls".
    • Each entry in "conditional_controls" is a "Condition". If a Condition is met for the setting, then the setting will be altered in the GUI to match the given state. The key for each Condition object is a string for human-readable purposes only, which is nice for debugging in the GUI itself.
      • A Condition can have four parts:
        • "value" = The value the setting should be set to if the condition is met
        • "enabled" = Set to true/false to enable/disable the setting respectively if the condition is met
        • "visible" = Set to true/false to show/hide the setting respectively if the condition is met
        • "conditions" = A list of "Partial Conditions", each an object of key/value pairs representing a different setting's name and a value for that setting. If at least one of the given settings is set to the respective value, the Partial Condition is considered to be met (OR logic).
    • For a Condition to be met, all child Partial Conditions must also be met (AND logic).
  • A setting can have multiple Conditions. If any of them are met, then the setting will be altered to reflect that condition's desired state.
  • If a setting has logic would disable another setting, that disabling logic will take priority over any conditional logic the secondary setting may have. No conditional logic will be respected in this scenario.
Examples

Show "Triforce Per World" when "Triforce Hunt" is enabled, and hide it if "Triforce Hunt" is disabled

This JSON simply mimics the existing behavior of these two settings. Currently the "triforce_count_per_world" setting is only shown and enabled if the "triforce_hunt" setting is enabled (set to 'true'). Otherwise, "triforce_count_per_world" is disabled by "triforce_hunt" when it too is disabled as well as being hidden entirely due to the "hide_when_disabled" metadata being set.

The following JSON mimics that same exact behavior. With it more explicitly written out like this, its a little easier to see what the end states are (though it is admittedly more verbose in comparison).

conditional_controls = {
    "show_for_triforce_hunt": {
        "conditions": [
            {"triforce_hunt": True},
        ],
        "visible": True,
        "enabled": True,
    },
    "disable_when_triforce_hunt_off": {
        "conditions": [
            {"triforce_hunt": False},
        ],
        "visible": False,
        "enabled": False,
    },
}

Set "Starting Age" to "Child" when "Forest" is set to "Closed Forest"

Currently the "open_forest" setting will disable the "starting_age" setting if it is set to "closed_forest". However, this visibly locks the "starting_age" setting as whatever value it was previously - it could still show visually as "Adult" or "Random" while being disabled due to the "Closed Forest" option. Due to the "closed_forest" option being set to actively disable the "starting_age" setting, this JSON functionally does nothing (disable logic takes priority and conditional logic is ignored entirely).

However, if we were to update the "Forest" setting to no longer disable "starting_age" when set to "closed_forest", the change below could help with user confidence and understanding. This JSON updates the value of "starting_age" to reflect the fact that the starting age will in fact be "child" when "closed_forest" is set. The "visible" and "enabled" metadata are there to mimic what the old behavior was - showing the setting visually to the user along with its newly selected option, but disabling the input so the user can't interact with it.

conditional_controls = {
    "set_start_age_to_child_for_closed_forest": {
        "conditions": [
            {"open_forest": "closed"},
        ],
        "value": "child",
        "visible": True,
        "enabled": False,
    }
}

Bonus Features

Expand (Bonus Features)

Extra features added as part of this change, but not directly related to supporting the "Conditional Control" logic. If someone asks, I'm happy to extract these into their own changesets to minimize the amount of code in this PR.

  • Lookup setting state in browser console

    • Open the browser console and type "Settings" to access a new global variable that can help you lookup the current state of a given setting. Each setting is a property on this object that can be accessed by the setting's name. Because these are properties on an object, this allows for fuzzy searching, suggestions, and auto-completion. The object returned will represent that setting's state, including if it's enabled, what its current value is, and its JSON blob from SettingsList.
      • Eg. "Settings.shuffle_ganon_bosskey" => The fuzzy search means that typing "Settings.sgbk" will give you "shuffle_ganon_bosskey" as a suggestion matching that string of characters.

Task List

Expand (Task List)

Required Tasks

Here's some things I can think of so far that need addressed:

  • Update remaining option types to support "conditional_controls"

Needs Testing

Stuff that needs tested still. Testing is likely to surface more work as things are discovered or decisions are made.

  • Figure out how to deal with dependency chains of settings.

    • At a minimum this should be enforced in the actual ROM patching code (and likely already is), but it should probably also be in the GUI code to help mitigate user error. There could become a nasty web of interconnected settings that all depend on shared bits and pieces of other settings.
    • Example: We want to add "Triforce Pieces" as a new option for Rainbow Bridge Requirement. We'd need to display the "Triforce*PerWorld" settings when its set to that new option and respect the values for those settings. But what if Triforce Hunt is also enabled - which do we respect? Or, let's say we also add this "Triforce Pieces" option to Ganon's Boss key - what happens if both the Rainbow Bridge and Ganon's Boss Key are set to "Triforce Pieces"?
  • What if a setting relies on one or more other settings of which none have any "disable" or "conditional_controls" json? Does this still get processed properly?

    • Tested and resolved this with a code change. Update a line that ensures the logic is executed for either "disable" or "conditional" logic.

Possibly out of scope tasks

Here's some tasks that could either be broken out as separate work or included in this change, some of which I currently have in this change. Just a matter of weighing options:

  • One large PR

    • PROS: just one merge could be easier depending on release cycles
    • CONS: harder to test, more possibility for bugs to slip through, one tricky feature could delay getting the others available to users)
  • Multiple PRs

    • PROS: easier to test, less to understand in your head at once, can give to users earlier
    • CONS: may take longer if each merge is a process
  • Consider breaking out "disable" logic from "visibility" logic

    • I've updated the code to make this possible, haven't actually altered the logic itself yet though. Some questions need answered about how to handle this at run-time. If we continue to allow the hide_when_disabled flag in the JSON, then we could potentially change that state at run-time. This would break the rules around an assumption the programmer could normally make. For example, if I set a setting with hide_when_disabled=true, but a condition alters the setting such that it will visually display the setting AND disable it... Now what do I trust as a dev if I want to set that flag and it just doesn't work?
      • PS. Worth noting the truth table here:
        • disabled + visible = a valid state (the user can see the input, but can't change it. current displayed value may or may not reflect its true value when patching begins)
        • disabled + hidden = a valid state (via hide_when_disabled)
        • enabled + visible = a valid state
        • enabled + hidden = INVALID STATE! We should (probably) never hide an input in the GUI that the user should also be able to change.
  • Add conditional "hide" logic

    • This is functionally supported in the conditional logic now. However, this is dependent on the work noted above regarding breaking out the "disable" and "visibility" logic for it to actually work. Currently it just does nothing at all.
  • Consider adding "prioritization" logic to the condition list

    • Currently all conditions are treated equal, and if one passes then the setting is shown. Might be pertinent to add logic that allows a condition to pass and be ignored if a condition of higher priority already passed. Many ways to approach that...
  • Find a better way to communicate to the user why a given setting is hidden or disabled

    • First thought is to still display the setting (maybe just the title text with the setting name?) and give it a gray + partially transparent overlay. Then you can throw some white text on top of it like Disabled by - {insert_setting_name}. Maybe can even click on it to bring focus to the disabling setting. As mentioned by flagrama, a tooltip could work as well.
    • Comment from flagrama:

      I haven't taken a look at any specifics, but I do want to point out that outright hiding stuff is going to result in a bad UX. There will definitely need to be some way of dynamically displaying to the user that a setting is disabling something else (tooltip probably works even though nobody reads them, lol).

  • Add ability to look up a setting and its current state via the JS console

  • Add ability to set a "default" value for the setting based its conditions

Conditional visibility added as a feature to the SettingsList
@LinkofOrigin LinkofOrigin marked this pull request as draft April 4, 2025 16:50
if info.disable is not None:
for option, disabling in info.disable.items():
negative = False
negative = False # If this option is enabled, the "disabling" settings will be disabled
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity-saving comments, not overly attached to these in either wording or existence

@fenhl fenhl linked an issue Apr 5, 2025 that may be closed by this pull request
@fenhl fenhl added Type: Enhancement New feature or request Component: GUI/Website Specifically affects the OoTR GUI Status: Needs Review Someone should be looking at it Component: Setting specific to setting(s) Status: Needs Testing Probably should be tested labels Apr 5, 2025
@LinkofOrigin LinkofOrigin changed the title Add Conditional Visibility logic to settings display for GUI Add Conditional Enable/Disable logic to settings display for GUI Apr 7, 2025
Successfully patched a ROM with this change. Did not load it up and playtest it.
Can always cherry-pick revert this change if you want to try it
This change is effectively a no-op, so no actual change in behavior or logic here. However, extracting the logic into a function means we can more dynamically determine if an element should be displayed/hidden or enabled/disabled, etc.

Currently these states are tightly coupled AND there is no way to change that coupling during run-time. With this change, there can be a distinction between whether or not a setting is enabled/disabled and whether it is hidden/visible. This function can be updated to dynamically determine if something should be "disabled and visible" and then later make it "disabled and hidden", which was previously impossible.
Still not 100% confident in this particular logic, but this seems correct after looking at it again
Hoping I got it right now... Hard to keep this straight sometimes
Conditional controls can now do the following when a condition is met:
- Define a value the setting should be set to
- Enforce whether the setting should be enabled or not
- Enforce whether the setting should be visible or not (currently not possible until more updates are made deeper in the pipeline)
With the last change, this certainly goes beyond just altering visibility. Updating language throughout to reflect that. Plus, it helps distinguish it from the current "visibility" code that is already a bit of a misnomer.
@LinkofOrigin LinkofOrigin changed the title Add Conditional Enable/Disable logic to settings display for GUI Add Conditional Control logic to settings display for GUI Apr 8, 2025
New "Settings" global variable that can be accessed via the browser console. Type "Settings." followed by the name of the setting you want to get info for. Allows for fuzzy searching, suggestions, and auto-complete.
return typeof (variable);
}

settingIsEnabled(setting_name: string) {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this doesn't do anything different in its current state, this opens up possibilities in the (near?) future to more dynamically control what defines a setting as "enabled" or "disabled".

return this.global.generator_settingsVisibilityMap[setting_name];
}

settingIsFullyHidden(setting: any) {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, this gives more control over what determines if a setting is "visible" or "hidden".

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much like this. Much cleaner, as much as I prefer to control as much as possible directly in Angular HTML

}

if ("controls_visibility_setting" in targetSetting) {
if ("controls_visibility_setting" in targetSetting || 'conditionally_controls_setting' in targetSetting) {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New flag to be processed in the JSON. If this isn't added, then both "disable" logic and "conditional" logic will only get processed if the setting option already has "disable" logic. In other words, a setting option that can only conditionally alter other settings would never have those conditions processed.

Comment on lines +1274 to +1278
// targetSetting = The current option of the setting to process.
// targetValue = 'true' if the settings this option controls should be enabled, 'false' if they should be disabled.
// (Note: This is passed in 'checkVisibility' as "option != value", in other words: "This option is NOT the option the setting is being changed to".)
// triggeredChange = Set to 'true' to force this function to return 'true', suggesting a change occurred regardless of how things processed.
// Otherwise, the function will return 'true' if a dependent setting's state was altered, otherwise it will return 'false'.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding on these values waffled for a long time as I kept confusing myself on what each variable represented. Finally just wrote it down so I could stop trying to remember or guess.


async parseGeneratorGUISettings(guiSettings, userSettings) {
const isRGBHex = /[0-9A-Fa-f]{6}/;
globalThis.Settings = {};
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enables the user to access the setting's details from the browser console. Handy for debugging, testing, confirming current state, and whatever else.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hangup with this particular line. I appreciate making debugging easier and principally it's a good idea to expose the settings like this. But since this is embedded on the website, I would appreciate making the name quite a bit more non generic to absolutely ensure it could never conflict, as the site already uses some globals. Definetly at least include the word Angular as it will be never used outside this context. Also there should be a bold comment above this notifying about the global injection and the debugging purpose.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good call, I'm definitely down for all that. Consider it another TODO for the pile o7

Comment on lines +57 to +59
def mark_conditional_control_option_for_setting(setting_name: str, conditional_settings: dict[str, Any]) -> None:
if setting_name not in conditional_control_dependencies:
conditional_control_dependencies[setting_name] = conditional_settings
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to build the full settings JSON before we can inject any conditional data into it. We would either need to inject early and be sure to carefully update it later, or do this where we deal with it at the end. This felt like the least disruptive option for now.

Comment on lines +93 to +95
if setting_info.conditional_controls is not None:
mark_conditional_control_option_for_setting(setting_info.name, setting_info.conditional_controls)
setting_json['conditional_controls'] = setting_info.conditional_controls
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Save this to be processed for later and also save it to this setting's JSON data. When the aforementioned "later" comes, we'll update the other settings to denote that they can alter this setting.

output_json['cosmeticsArray'].append(tab_json_array)

# Resolve conditional visibility settings now that the full json is available
resolve_conditional_control_dependencies(output_json)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previously aforementioned "later" has arrived.

json.dump(output_json, f)


def resolve_conditional_control_dependencies(output_json: dict) -> None:
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really ugly nesting logic here. I'm directly walking through the conditional logic we saved for later AND the actual json object we built up for all the settings. This is so we can locate the settings that could enable other settings per any conditional logic.

With this, we update those settings' options with a new conditionally_controls_setting entry with a list of settings that it may alter when set. We do this to distinguish it from controls_visibility_setting since that has a very baked-in set of logic to disable a setting. The new logic could also enable it, so we need to handle that too.

if 'conditionally_controls_setting' not in setting_obj_option_json:
setting_obj_option_json['conditionally_controls_setting'] = dependent_setting_name
elif dependent_setting_name not in setting_obj_option_json['conditionally_controls_setting']:
setting_obj_option_json['conditionally_controls_setting'] += ',' + dependent_setting_name
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason this uses a string with commas in it rather than a JSON array of strings?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, I can't think of a good reason lol. Def more readable as JSON. I can get that swapped, looks like it should be easy on both sides here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I think I found the reason I did this originally. This line from the GUI code is using this same format:

targetSetting["controls_visibility_tab"].split(",").forEach(tab => {

When I was writing this out originally, I was effectively using that as a template for this new bit of metadata (or at least I think I was, been a hot minute so memory is fuzzy). So I was trying to follow an existing code pattern with whatever iteration I had at the time. That said, looking over things again now I've moved much of the code dealing with conditional logic to be separate from the old "visibility" logic. At this point I think it makes sense to roll with a JSON array instead of the CSV as it doesn't hinder the readability of the bits of code around it.

All that to say, I agree with the JSON array format here :)

@fenhl
Copy link
Copy Markdown
Collaborator

fenhl commented May 16, 2025

The “Feature Breakdown & Examples” section of the PR description would be useful to have as documentation in the repo as well, perhaps as a new document in Notes/GUI, or as a new section in Notes/GUI/architecture.md.

@LinkofOrigin
Copy link
Copy Markdown
Author

(Pardon the delayed response. Apparently my notifications weren't set up to email me for whatever reason. Fixed that now lol... anyway...)

The “Feature Breakdown & Examples” section of the PR description would be useful to have as documentation in the repo as well, perhaps as a new document in Notes/GUI, or as a new section in Notes/GUI/architecture.md.

More than happy to add that! I love me some documentation, but wasn't sure if/where it would be appropriate to add. To your point there are at least a couple files that feel like it could fit into. If you don't have a strong opinion, I'll scour them a bit and give it whatever home feels best. Can always move it :)

- Updated the JSON blob that is generated for conditional controls to use a JSON array instead of a CSV (improves readability and maintainability)
- Added check for the old "controls_visibility_setting" to not run the logic if that setting didn't have such rules. This was causing it to error out and stop execution entirely (not sure how I missed this before)
- Added an extra bit of logic to help re-enable a setting if a condition is not actively disabling it when deriving the target state the setting should be in. This fixes an issue where if a setting wasn't explicitly being disabled via the old "disable" rule AND only had conditional rules for disabling it, the setting could get disabled and would never be re-enabled. You could add a workaround to the SettingsList where you just add every option of the dependent setting and explicitly set the enable/visible flags as desired, but that would be very clunky and kind of silly to read.

PS. This also just starts to crack the surface towards breaking the current assumption that anything that could be disabled should be IF some other setting would enable it. That remains true for the old disable logic (and will still override with this change as normal), but now that some of the logic has been separated we can start to see what challenging that assumption may look like and how we'd implement it.
@LinkofOrigin LinkofOrigin requested a review from fenhl June 12, 2025 21:12
@LinkofOrigin
Copy link
Copy Markdown
Author

Updated the JSON thing and updated the architecture notes. Also fixed some small bugs along the way that I'm not sure how I missed before.

Open to further feedback, happy to keep adjusting stuff around as needed. Those doc notes in particular are naturally a bit outdated, and what I added is pretty wordy (sticks out like the sorest of thumbs). :)

@fenhl fenhl requested review from dragonbane0 and removed request for fenhl July 17, 2025 06:44
@dragonbane0
Copy link
Copy Markdown
Collaborator

Will take a look at this over the coming days

Copy link
Copy Markdown
Collaborator

@dragonbane0 dragonbane0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for taking so long to get to this PR. This mostly looks excellent and makes the code quite a bit cleaner. I still need to do some testing, but purely logically speaking I don't see anything wrong with it. I only marked the use of a global as needing a change. And some clarification is required for the documentation and whether the "value" conditional setter is stable in all SettingsList.py conditions.

Since you introduce this as a future feature with no particular changes to the SettingsList right now with full backwards compatibility, did you test the current state with only the "legacy" flags in-depth without observing any degradation in the expected behavior?


async parseGeneratorGUISettings(guiSettings, userSettings) {
const isRGBHex = /[0-9A-Fa-f]{6}/;
globalThis.Settings = {};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hangup with this particular line. I appreciate making debugging easier and principally it's a good idea to expose the settings like this. But since this is embedded on the website, I would appreciate making the name quite a bit more non generic to absolutely ensure it could never conflict, as the site already uses some globals. Definetly at least include the word Angular as it will be never used outside this context. Also there should be a bold comment above this notifying about the global injection and the debugging purpose.

// If the condition sets one of these keys, we'll use that value. Otherwise use the current value.
targetSettingState['value'] = conditionToTest['value'] != null ? conditionToTest['value'] : targetSettingState['value'];
targetSettingState['enabled'] = conditionToTest['enabled'] != null ? conditionToTest['enabled'] : targetSettingState['enabled'];
targetSettingState['visible'] = conditionToTest['visible'] != null ? conditionToTest['visible'] : targetSettingState['visible'];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you marked it as ToDo "currently not working" then it's fine to leave it like this, as long as the SettingsList.py makes no mention of it and it doesn't go into any settings documentation. People indeed probably won't look in here to notice and just go off the existing SettingsList

return this.global.generator_settingsVisibilityMap[setting_name];
}

settingIsFullyHidden(setting: any) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much like this. Much cleaner, as much as I prefer to control as much as possible directly in Angular HTML

// If a setting won't be forcibly disabled, allow conditions to update the setting
for (let settingName in conditionalSettingUpdates) {
if (!settingsDisabled.includes(settingName)) {
this.global.generator_settingsMap[settingName] = conditionalSettingUpdates[settingName]['value'];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I'm not entirely sure about. Did you test if conditional updating of settings is properly stored in presets and it works when loading them back? Does the py safeguard against outputting a valid JSON if the user changes the settings type but leaves an incompatible conditional value setter in the list e.g. change from number to enum but there is a still a conditional number "value" setting? Additionally is it ensured the "default" value in the JSON is always correct e.g. assuming the ultimate default state one day becomes one where some settings are already conditionally disabled forcing other settings to a specific value, but the "default" is set to something else, is that possible and/or an potential issue?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I did just now test each of these a moment ago, I'm also answering much this purely off of memory, so apologies if any of this comes off as facetious:

Did you test if conditional updating of settings is properly stored in presets and it works when loading them back?

I don't recall testing this in the past, but a quick test now with my given example suggests it is working. If I load the S8 Tournament preset, I get "Forest = Closed Deku" and "Starting Age = Random". If I then load the Default / Beginner preset, I get "Forest = Closed Forest" and "Starting Age = Child (disabled)". This is what I would expect once all the logic runs and does it's thing.

The tricky part about this one is that a preset is likely to (or even should?) define a value for every single setting. In other words, there are no settings missing an explicit value as determined by the preset. So even if a setting does have a conditional rule for a value that is set by the preset, the value may not be logically driven by the condition so much as driven by the preset itself.

The parts that would be driven by the conditional logic are the visibility and enabled status, which do appear to work from cursory testing. If we want to test the scenario where a preset doesn't explicitly set a value for a setting that should have its value set by some condition, we'd have to hack up a scenario that likely isn't valid to begin with. (Tell me if I'm wrong though, there's certainly some assumptions in there worth validating)

Does the py safeguard against outputting a valid JSON if the user changes the settings type but leaves an incompatible conditional value setter in the list e.g. change from number to enum but there is a still a conditional number "value" setting?

Pretty confident I did not test this scenario previously, but I can say from testing right this moment that the Python script definitely explodes when trying to run SettingsToJson.py with an invalid type - which is good! I suspect this is because somewhere it tries to cast the setting value to the type assigned to the setting itself (which is maintained in SettingsTypes.py), so if the type doesn't cast properly it throws an error. I also just tested that even changing the value to a non-existent value within a discrete list while retaining the type (eg. changing "closed" to "BIPPITYBOPPITYBOO") also results in an error.

^ That is all outside of any conditional logic as well, just purely the settings values as they are. Though I won't pretend that means the safeguard will cover 100% of the scenarios opened by these changes, but at a basic level it seems to cover some. Worth some extra attention to see if we can find any odd edge cases (eg. maybe testing 0 for a boolean type or something).

(I would also lightly challenge that any issues in this area may be "out of scope" for this particular change. Not to say that we can't - or even shouldn't - fix any issues like this with this PR as needed. Just that if we do uncover any "python json validation" bugs may be pre-existing in some capacity. If we do find some, it would be worth confirming if they are due to this change specifically -vs- if they can be replicated on main or elsewhere)

Additionally is it ensured the "default" value in the JSON is always correct e.g. assuming the ultimate default state one day becomes one where some settings are already conditionally disabled forcing other settings to a specific value, but the "default" is set to something else, is that possible and/or an potential issue?

Also very likely didn't test this before, though let me make sure I'm following here. If I understand things correctly, I think this is the flow/expectation:

  1. If it is set, a "default" value is always a value within the subset list of values of a setting.
  2. Even for those without an explicit default value, the object type on the setting defines a default (or at least should). Therefore every setting should always have at least some value by default.
  3. ^ Per those points, a setting will always have a value - regardless of how it was derived, it will be set to something.
  4. A default value is only applied when there is no value currently recorded in the locally saved settings files (settings.sav) as the GUI loads.
  5. From an order-of-operations perspective, values for settings are always "registered" before the disable/conditional logic is run. (This is currently the case and importantly reliant on point 3 above)

Hopefully that all is accurate and makes some amount of sense. Now to set the stage for your scenario with a specific example (which I just used to test with):

  1. "Starting Age" is conditionally set to "Child" if the "Forest" setting is set to "Closed Forest".
  2. The default value for "Starting Age" is currently set to "Child".
  3. The default value for "Forest" is currently set to "Closed Forest".
  4. Now, I'm going to change the default value for "Starting Age" to be "Adult" instead.

So I update the setting via SettingsToJson -> delete my saved savings file -> re-run the GUI from a fresh state. As of this exact moment, this scenario seems to work correctly. On load, the "Starting Age" value is set to "Child" due to the condition on the "Forest" setting - even though that wasn't the default defined by the base settings list.

Given the first set of bullet points, I think that makes sense. The values are all registered before the conditional logic runs, and (at the risk of stating the obvious) the conditional logic does run in an on-load state with no settings saved locally. If you debugged the loading path, I bet you could see the Starting Age get set to "Adult" initially (or at least saved in a payload of sorts), and then change to "Child" after the other logic runs.

Further, from point 4 in the first set of notes, changing a default value after this point has no effect since the settings have been saved locally already (this happens immediately on first load). The values are set and the conditional logic will run off of those instead. So at a glance this scenario seems covered (again, outside of hacking up a likely invalid scenario just for the sake of it).

* "key" -> {object}: "key" has no functional purpose and is purely for human readability and debugging purposes. "object" contains key/value pairs that define the behavior of the condition.
* value: If the condition passes, the setting will be changed to this value.
* enabled: If the condition passes, `True` will enable the setting and `False` will disable it.
* visible: If the condition passes, `True` will display the setting in the UI if the condition passes and `False` will hide it.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the "visible" flag have no effect at all right now as per your above comment? If so it should probably not be in here for the time being. Or at least marked as not having any effect

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a good point. Yeah I'll TODO that for removal as well. Worst case I suppose this version is in a commit now so someone can find it for copy/paste purposes... Though I suspect it will take infinitely less time to just write out another version when the time comes 😛

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Bind a property as a function that returns an object representing this setting for easy debugging
// By using the setting name as the property name, it allows for auto-complete and fuzzy searching/suggestions
// This works by binding a property to a getter function that has the current 'this' value bound to the function context
Object.defineProperty(globalThis.Settings, setting.name, {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the name is adjusted, same goes for here. As far as I can tell you did not use this global anywhere in the actual code, so it is purely for debugging yes?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll say with confidence "yes, definitely just for debugging" while also acknowledging that I don't fully remember LOL. At a glance that appears to be the purpose, and I recall someone made a specific request for this so I believe that's why it exists.

Thanks for tagging the extra usage too! Easier to refactor o7

@fenhl fenhl added the Status: Waiting for Author Changes or response requested label Mar 21, 2026
@LinkofOrigin
Copy link
Copy Markdown
Author

I apologize for taking so long to get to this PR. This mostly looks excellent and makes the code quite a bit cleaner. I still need to do some testing, but purely logically speaking I don't see anything wrong with it. I only marked the use of a global as needing a change. And some clarification is required for the documentation and whether the "value" conditional setter is stable in all SettingsList.py conditions.

No worries at all! Just the nature of projects like this 🙂 I could just as well have been more pushy to drive this to completion sooner as anyone could have reviewed it sooner. C'est la vie

I'll respond to the other individual comments for context purposes, but as for this part:

Since you introduce this as a future feature with no particular changes to the SettingsList right now with full backwards compatibility, did you test the current state with only the "legacy" flags in-depth without observing any degradation in the expected behavior?

Even if it hadn't been nearly a year since I last looked at this, I still wouldn't say that I have tested it thoroughly with other flags/settings beyond the examples I shared above. Frankly I doubt I have the expertise or knowledge to go about that in a reasonable way that wouldn't be manually testing every combination I could could mathematically come up with (ie. not particularly reasonable). Of what I did test, everything worked fine (if memory serves)... but that was just surface-level testing that certainly didn't cover much ground.

All that said, I'm more than happy to proceed with some deeper testing, especially if there's a desire to introduce this feature. I'll need to refamiliarize myself with most (read: all) of this, but otherwise I'd love to do whatever you and others feel is appropriate to knock this out! If you have any particular guidance on how to go about executing a more thorough test strategy I'd appreciate anything you can share. Stuff like common paths, edge cases that are known pain points, etc.

@flagrama
Copy link
Copy Markdown

as anyone could have reviewed it sooner.

That's not exactly true. dragonbane0 and Trez are the best equipped with reviewing GUI stuff since they can catch issues that will interfere with the web side. I'm not sure how much of the development team has access to the web aspects now, but I don't think it's more than a couple more of them, and the GUI is not their focus. Only a couple of people have even tried to touch the GUI.

@dragonbane0
Copy link
Copy Markdown
Collaborator

Even if it hadn't been nearly a year since I last looked at this, I still wouldn't say that I have tested it thoroughly with other flags/settings beyond the examples I shared above. Frankly I doubt I have the expertise or knowledge to go about that in a reasonable way that wouldn't be manually testing every combination I could could mathematically come up with (ie. not particularly reasonable). Of what I did test, everything worked fine (if memory serves)... but that was just surface-level testing that certainly didn't cover much ground.

All that said, I'm more than happy to proceed with some deeper testing, especially if there's a desire to introduce this feature. I'll need to refamiliarize myself with most (read: all) of this, but otherwise I'd love to do whatever you and others feel is appropriate to knock this out! If you have any particular guidance on how to go about executing a more thorough test strategy I'd appreciate anything you can share. Stuff like common paths, edge cases that are known pain points, etc.

Yeah the goal is to merge this. There is another PR open that competes with your implementation in terms of AND: #2457

But I think merging your PR first is the better pathway here. I hope your implementation might fix the bug that that PR was trying to address as well.

Apart from the minor fixes and clarifications, I don't really need testing of every single combination and edge case. As you said, that seems impossible. I think if we can test this out on a few of the branches currently featured on the site (by throwing your GUI on top of their code) and there is no obvious degradation to the current state then I'm fine merging this and fix the rest as it comes

state and metadata as is relevant, including a direct JSON-decoded object of that settings'
data from the generated settings list JSON file.
*/
globalThis.SettingsListZOOTR_Angular = {};
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated name - open to suggestions because #NamingIsHard

@fenhl fenhl removed the Status: Waiting for Author Changes or response requested label Mar 23, 2026
@LinkofOrigin
Copy link
Copy Markdown
Author

@dragonbane0 Updates for the variable name and the note in the architecture docs have been adjusted. LMK if you want any other updates or want to discuss any bits and/or pieces. Also open to a voice call if you feel it would be helpful to talk verbally over any of this for the sake of review or understanding (on either side of the fence). Thanks again!

@dragonbane0
Copy link
Copy Markdown
Collaborator

Long name but for debugging that's fine I think haha. I think this PR is in a good state now. Just need to test some of the popular branch default states with it to see if it plays nice and then I'm okay with merging it

@fenhl fenhl removed the Status: Needs Review Someone should be looking at it label Mar 24, 2026
@LinkofOrigin LinkofOrigin requested a review from fenhl March 28, 2026 20:10
Copy link
Copy Markdown
Collaborator

@fenhl fenhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not qualified to give a full review, sorry. The Python changes look good to me.

@fenhl
Copy link
Copy Markdown
Collaborator

fenhl commented Mar 29, 2026

@dragonbane0 Should I try merging this into my branch for testing purposes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: GUI/Website Specifically affects the OoTR GUI Component: Setting specific to setting(s) Status: Needs Testing Probably should be tested Type: Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting enable relations

4 participants