Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settings GUI: Add setting dependencies #13704

Merged
merged 1 commit into from Aug 5, 2023

Conversation

rubenwardy
Copy link
Member

@rubenwardy rubenwardy commented Jul 30, 2023

Part of #13476

The aim of this PR is to improve User Experience by only showing settings when they make sense. This is currently done in a hardcoded way for shader settings in "Most Used", this PR makes it more generic and will allow removing Most Used in the future.

Settings can have a list of requirements, defined in the settingtypes.txt file. For example:

#    Some example setting description
#
#    Requires: shaders, enable_dynamic_shadows, !touchscreen_gui
example_setting (Example setting) float 0.2 0.001 10.0

These requirements may be:

  • The value of a boolean setting, such as enable_dynamic_shadows
  • An engine-defined value:
    • shaders (both enable_shaders and a video driver that supports it)
    • desktop
    • android
    • touchscreen_gui for builds with touchscreen controls
    • opengl / gles
  • You can negate any requirement by prepending with !

To do

This PR is a Work in Progress

  • Hide non-setting components (Ie: "change keys")
  • Remove sections with no content
  • Remove pages with no content
  • Clicking reset on enable_shaders doesn't show settings again
  • Trim trailing whitespace
  • Show fields as greyed out with a tooltip if it's possible to enable them? Future PR
  • Refactor

Question: Should setting and engine-defined keys be distinguished? Perhaps all engine-defined keys should be in capitals - TOUCHSCREEN_GUI, SHADERS, DESKTOP, etc. This may be a good idea for future compat. Say we add a new engine-defined value in the future: foobar. A mod may add this as a requirement in their settingtypes.txt, but this would result in older versions of the engine assuming that it is a setting value not an engine-defined value, and thus showing a warning and considering it not fulfilled. But perhaps its a good thing to have unknown values be unfulfilled

How to test

  • Try enabling and disabling shaders. Try various shader options as well

  • In check_requirements, add the following to the top to pretend to be Android:

    local TOUCHSCREEN_GUI = true
    local PLATFORM = "Android"

@rubenwardy rubenwardy mentioned this pull request Jul 30, 2023
17 tasks
@rubenwardy rubenwardy added WIP The PR is still being worked on by its author and not ready yet. Roadmap The change matches an item on the current roadmap. labels Jul 30, 2023
@rubenwardy rubenwardy added this to the 5.8.0 milestone Jul 30, 2023
@rubenwardy rubenwardy marked this pull request as ready for review August 2, 2023 15:46
@rubenwardy rubenwardy removed WIP The PR is still being worked on by its author and not ready yet. Testing needed labels Aug 2, 2023
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I did a few tests previously and did not find any strange behaviour.

@rubenwardy rubenwardy merged commit d16d1a1 into minetest:master Aug 5, 2023
2 checks passed
@rubenwardy rubenwardy deleted the new_settings_dependent branch August 5, 2023 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature Formspec @ Mainmenu One approval ✅ ◻️ Roadmap The change matches an item on the current roadmap. @ Startup / Config / Util UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants