Add Conditional Control logic to settings display for GUI#2400
Add Conditional Control logic to settings display for GUI#2400LinkofOrigin wants to merge 23 commits intoOoTRandomizer:Devfrom
Conversation
Conditional visibility added as a feature to the SettingsList
| 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 |
There was a problem hiding this comment.
Sanity-saving comments, not overly attached to these in either wording or existence
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.
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.
Sneaky code...
| return typeof (variable); | ||
| } | ||
|
|
||
| settingIsEnabled(setting_name: string) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Similar to above, this gives more control over what determines if a setting is "visible" or "hidden".
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
| // 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'. |
There was a problem hiding this comment.
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.
GUI/src/app/providers/GUIGlobal.ts
Outdated
|
|
||
| async parseGeneratorGUISettings(guiSettings, userSettings) { | ||
| const isRGBHex = /[0-9A-Fa-f]{6}/; | ||
| globalThis.Settings = {}; |
There was a problem hiding this comment.
This enables the user to access the setting's details from the browser console. Handy for debugging, testing, confirming current state, and whatever else.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh good call, I'm definitely down for all that. Consider it another TODO for the pile o7
| 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
The previously aforementioned "later" has arrived.
| json.dump(output_json, f) | ||
|
|
||
|
|
||
| def resolve_conditional_control_dependencies(output_json: dict) -> None: |
There was a problem hiding this comment.
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.
SettingsToJson.py
Outdated
| 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 |
There was a problem hiding this comment.
Any particular reason this uses a string with commas in it rather than a JSON array of strings?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Update: I think I found the reason I did this originally. This line from the GUI code is using this same format:
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 :)
|
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 |
|
(Pardon the delayed response. Apparently my notifications weren't set up to email me for whatever reason. Fixed that now lol... anyway...)
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.
|
Updated the JSON thing and updated the 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). :) |
|
Will take a look at this over the coming days |
There was a problem hiding this comment.
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?
GUI/src/app/providers/GUIGlobal.ts
Outdated
|
|
||
| async parseGeneratorGUISettings(guiSettings, userSettings) { | ||
| const isRGBHex = /[0-9A-Fa-f]{6}/; | ||
| globalThis.Settings = {}; |
There was a problem hiding this comment.
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']; |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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']; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- If it is set, a "default" value is always a value within the subset list of values of a setting.
- 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.
- ^ Per those points, a setting will always have a value - regardless of how it was derived, it will be set to something.
- 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. - 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):
- "Starting Age" is conditionally set to "Child" if the "Forest" setting is set to "Closed Forest".
- The default value for "Starting Age" is currently set to "Child".
- The default value for "Forest" is currently set to "Closed Forest".
- 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).
Notes/GUI/architecture.md
Outdated
| * "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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😛
GUI/src/app/providers/GUIGlobal.ts
Outdated
| // 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, { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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:
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. |
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. |
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 |
Can revisit in the future when/if the visibility logic gets cleaned up.
| 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 = {}; |
There was a problem hiding this comment.
Updated name - open to suggestions because #NamingIsHard
|
@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! |
|
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
left a comment
There was a problem hiding this comment.
I'm not qualified to give a full review, sorry. The Python changes look good to me.
|
@dragonbane0 Should I try merging this into my branch for testing purposes? |
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:
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 BreakdownandExamplessections below for more details.)This allows for something simple like:
This also allows more complex logic like:
Feature Breakdown & Examples
Feature Breakdown
Examples
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).
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.
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.
Task List
Expand (Task List)
Required Tasks
Here's some things I can think of so far that need addressed:
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.
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?
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
Multiple PRs
Consider breaking out "disable" logic from "visibility" logic
hide_when_disabledflag 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 withhide_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?hide_when_disabled)Add conditional "hide" logic
Consider adding "prioritization" logic to the condition list
Find a better way to communicate to the user why a given setting is hidden or disabled
Disabled by - {insert_setting_name}. Maybe can even click on it to bring focus to the disabling setting. As mentioned byflagrama, a tooltip could work as well.flagrama: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