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

Add many limits to settingtypes #11463

Merged
merged 2 commits into from Jun 30, 2022
Merged

Conversation

Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Jul 17, 2021

This PR adds the missing upper and lower limits to many settings (int and float) in settingtypes.txt. Now I will describe how I came up with those limits:

  • Limits found by testing: I used a manual binary search for control/rendering-related settings to find the point at which problems start to occur, then I reduced the value a little above that point (as a "buffer") and used that as a limit. There are the settings for which I did it:
    • mouse_sensitivity: Above 10 it's very fast and hard to control, it is close to being unplayable. At 100 (current limit in source code) it's completely unplayable.
    • cloud_radius: Serious render bugs start to occur above ca. 65. Clouds disappear completely, error message in console: "Too many vertices for 16bit index type, render artifacts may occur.". Note this is not the same as the "sharp cutoff" as stated in the description
    • 3d_paralax_strength: Beyond -0.087 or 0.087, the camera goes wild at certain or all view angles, making it unplayable
    • hud_scale: HUD is de facto unusable below 0.1
    • gui_scale: Below 0.5 it becomes too hard to use (tested at multiple resolutions, big and small)
    • font_size (and similar): 5pt is the smallest size in the default font at which I still can make out letters, barely. At 4pt, it's just a pixel mush. At 1000pt or above, Minetest crashes. At 72pt, the interface starts to break down (Minetest clearly isn't made for large fonts)
    • view_bobbing_amount: Above 56 the effect is so strong, the camera moves below the ground. Above 16, the camera often moves into the left or right wall when you walk on the center of the node
    • movement_liquid_fluidity: Can't be safely 0 like the other movement_ settings because of a division by 0 and you'd get teleported to (-nan, -nan, -nan) if you touch any liquid.
  • Limits set for other reasons:
    • chunk_size: The description says there is no reason or benefit for this to be above 5, so 5 is the upper limit then.
  • Mapgen Y limits: For all mapgen settings that ask for a Y height, it's always (-31000, 31000). In the code, those are s16 anyway.
  • Mapgen taper height: 32767 because those always use s16
  • Technical limits: 255, 32767, 65535, 2147483647, 4294967295 and 18446744073709551615 were used as upper limit when there was no other reasonable upper bound, in order to match the C++ data type limits (u8, s16, u16, s32, u32, u64).
  • Logical limits: I often used 0 or 1 as lower bound for logical reasons, e.g. minimum waiting time, distances can't be lower than 0
  • Special lower bound: Sometimes, the number 0 or -1 has a special meaning, so I used that sometimes as a lower bound
  • No upper limit: Rarely, usually when it's based on Lua code (builtin), I placed no upper limit. Many floats also have no upper limit. For some settings, I had no idea what to do, so I also placed no upper bound

The overal idea here is, when a (previously permitted) value causes crashes, render bugs, or similar, it must be moved out of the allowed range. And then apply somewhat of a buffer as a "safe zone", just in case.

As for performance, I generally used the maximum value of the C++ data type because those values might work in theory if you have strong enough hardware.

I also made minor changes to the setting descriptions, usually for mentioning the missing unit (milliseconds or seconds).

To do

This PR is ready for review.

How to test

First, just read the changeset and my justifications above and think whether the new number makes sense.
Especially complain if you think a range is too small and there are reasonable values beyond the range I chose.
Also complain if you think that any in-range value might lead to errors, crashes, etc.

Then, check if the All Settings menu still behaves properly.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jul 17, 2021

Fixes #8979.

@SmallJoker
Copy link
Member

This won't fix the underlying problem though. These limits should primary applied on engine-side.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jul 17, 2021

I know. That's not the goal here anyway. Consider it as some kind of documentation and/or first step. As for the engine side, I think it makes sense mostly for those values for which I have determined breakage.

@SmallJoker SmallJoker added @ Startup / Config / Util Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Jul 17, 2021
@appgurueu
Copy link
Contributor

This won't fix the underlying problem though. These limits should primary applied on engine-side.

Are the limits in settingtypes.txt not enforced?

@SmallJoker
Copy link
Member

Are the limits in settingtypes.txt not enforced?

Some are rangelimed for extreme cases, some aren't. There's yet no settingtypes.txt parsing by the C++ engine.

@lhofhansl
Copy link
Contributor

I have used 3d_paralax_strength of 0.1 with good effect. (with anaglyph)

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jul 19, 2021

@lhofhansl: Look exactly downwards or upwards. Camera starts shaking.

@Extex101
Copy link

I tested out some of these beyond limits, managed to get some pretty crazy effects with view_bobbing
tilt

When you walk and stop when the tilt is super insanely high and fly you can get a camera tilt effect

@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author. label Sep 30, 2021
@SmallJoker
Copy link
Member

(rebase needed)

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Oct 1, 2021

Rebase done!

@SmallJoker SmallJoker removed the Rebase needed The PR needs to be rebased by its author. label Oct 1, 2021
@Df458 Df458 added the Rebase needed The PR needs to be rebased by its author. label Feb 20, 2022
@rubenwardy rubenwardy added the Concept approved Approved by a core dev: PRs welcomed! label Apr 25, 2022
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented May 2, 2022

Rebase done!

@Zughy
Copy link
Member

Zughy commented May 2, 2022

( @Wuzzy2 you're a triager as well, please remove the label by yourself next time. Same for "Adoption needed" in the other PR you've just closed)

@Zughy Zughy removed the Rebase needed The PR needs to be rebased by its author. label May 2, 2022
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.

Better than nothing. I'll work on the C++ side to ensure the limits are actually enforced.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jun 24, 2022

@rubenwardy: Done.

@rubenwardy rubenwardy merged commit 7494ff2 into minetest:master Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements @ Startup / Config / Util >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants