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.

Sign up for GitHub

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

Jump to bottom

Enforce limits of settings that could cause buggy behaviour #12450

Merged
merged 5 commits into from
Jul 9, 2022
Merged

Enforce limits of settings that could cause buggy behaviour #12450

merged 5 commits into from
Jul 9, 2022

Conversation

SmallJoker
Copy link
Member

@SmallJoker SmallJoker commented Jun 18, 2022 •
edited by rubenwardy
Loading

Enforces the setting value bounds that are currently only limited by the GUI (settingtypes.txt).

See: #9000 and #11463

Fixes #11738 and fixes #11739

To do

This PR is Ready for Review.

How to test

  1. Code review
  2. Get creative

@SmallJoker SmallJoker added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label Jun 18, 2022
@Zughy
Copy link
Member

Zughy commented Jun 18, 2022

This will fix #11738 and #11739

@appgurueu
Copy link
Contributor

I don't like the duplication of these limits here.

nerzhul
nerzhul requested changes Jun 20, 2022
View reviewed changes
builtin/mainmenu/dlg_contentstore.lua Show resolved Hide resolved
src/client/camera.cpp Outdated Show resolved Hide resolved
src/client/camera.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 added the Bugfix 🐛 PRs that fix a bug label Jun 21, 2022
sfan5
sfan5 reviewed Jun 21, 2022
View reviewed changes
src/emerge.cpp Outdated Show resolved Hide resolved
sfan5
sfan5 reviewed Jun 21, 2022
View reviewed changes
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
sfan5
sfan5 reviewed Jun 21, 2022
View reviewed changes
src/map.cpp Show resolved Hide resolved
@Zughy
Copy link
Member

Zughy commented Jun 22, 2022

Will this also take care of #8979 (and #11740) ?

@SmallJoker
Copy link
Member Author

@Zughy A future PR might. After this one is merged, I will update that issue accordingly.

rubenwardy
rubenwardy reviewed Jul 2, 2022
View reviewed changes
src/client/camera.cpp Outdated Show resolved Hide resolved
src/client/hud.cpp Outdated Show resolved Hide resolved
src/client/renderingengine.cpp Outdated Show resolved Hide resolved
src/gui/guiTable.cpp Outdated Show resolved Hide resolved
src/gui/modalMenu.cpp
@@ -40,7 +40,7 @@ GUIModalMenu::GUIModalMenu(gui::IGUIEnvironment* env, gui::IGUIElement* parent,
m_menumgr(menumgr),
m_remap_dbl_click(remap_dbl_click)
{
m_gui_scale = g_settings->getFloat("gui_scaling");
m_gui_scale = std::max(g_settings->getFloat("gui_scaling"), 0.5f);
Copy link
Member

Choose a reason for hiding this comment

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

max of 20 too?

src/mapgen/mapgen.cpp Show resolved Hide resolved
@rubenwardy rubenwardy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jul 2, 2022
@appgurueu
Copy link
Contributor

Manually implementing this on the C++ side of things is error-prone code duplication. Couldn't C++ be made to read settingtypes.txt to apply the limits? Alternatively, C++ could use Lua to do the clamping to not duplicate the settingtypes reading code (create settings object -> read settingtypes -> do clamping / erroring).

@SmallJoker
Copy link
Member Author

SmallJoker commented Jul 3, 2022 •
edited
Loading

@appgurueu Could? Yes sure, it's possible if someone's willing to spend their time on it. A similar attempt (but much more into depth) was made in #6728.
In the future, a Settings could (for example) have std::map<settingname, const limitdata> limits which is read from settingtypes.txt once, and looked up for each getXXX function call. Changing the overloaded getFloat function would be trivial at this point.

@SmallJoker
Copy link
Member Author

PR updated with limits to fix #12493 because the relevant C++ line was touched anyway.

@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jul 4, 2022
@Zughy
Copy link
Member

Zughy commented Jul 4, 2022

@SmallJoker please don't go lower than the default, or people can still cheat of half a second per block if they know about the parameter

sfan5
sfan5 reviewed Jul 5, 2022
View reviewed changes
builtin/settingtypes.txt Outdated Show resolved Hide resolved
src/client/renderingengine.cpp Show resolved Hide resolved
src/map.cpp Outdated Show resolved Hide resolved
@SmallJoker SmallJoker merged commit 051181f into minetest:master Jul 9, 2022
@Desour Desour mentioned this pull request Aug 20, 2022
Closed
@Zughy Zughy mentioned this pull request Nov 20, 2022
Closed
@Desour Desour mentioned this pull request Jan 21, 2023
Merged
@rubenwardy rubenwardy mentioned this pull request Apr 14, 2023
Open
74 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubenwardy rubenwardy rubenwardy left review comments

@appgurueu appgurueu appgurueu left review comments

@nerzhul nerzhul nerzhul requested changes

@sfan5 sfan5 sfan5 approved these changes

Assignees
No one assigned
Labels
Bugfix 🐛 PRs that fix a bug Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Milestone
No milestone
6 participants
@SmallJoker @Zughy @appgurueu @nerzhul @sfan5 @rubenwardy