Fixed inspector precision issues when editing floats

[aqnuep]

For some reason the editor limits the precision of all floating-point values,
including vectors, matrices, etc. to 3 decimal places after the decimal point
(as they all have a step of 0.001).

This change gets rid of this artificial limitation at least for the basic
types and uniforms.

The implementation of step_decimals() is also changed to use log10 instead of
a loop, and it will now correctly handle a step size of zero and return maxn
while previously it incorrectly returned 0 in this case.

Fixes #18251 (at least).

[aqnuep]

I know some people will complain after this change that they type e.g. "100.01" in and get "100.0100021362" displayed (due to the precision limitations of FP32), but that's actually good, because you are at least aware of what is the actual value stored in memory.

However, without this change you can't even use simple values like 1/16 (0.0625) as values of floats, vector components, etc. because both the displayed and the serialized value gets truncated to 3 decimal digits.

[aqnuep]

I'd also note that this change still does not allow being able to use any float value as the maximum number of digits after the decimal point are still limited by maxn=10 in step_decimals() and by String::num() which also maximizes the number of decimals to 16.

[akien-mga]

I see various issues with this PR:

• The default step for EditorProperty controls is used not only to format the visual representation of floats, but also to specify the step of the EditorSpinSlider. With this PR, all properties without a manually specified step lose their sliders. The relation between slider step and decimal representation in SpinBox and EditorSpinSlider is a known limitation, tracked in #19242.

• Showing floating point precision errors on all float properties is definitely not wanted. The majority of Godot users is happy to input 300.1 and see that in the inspector, even if it's 300.000937 behind the scenes. A surprisingly high number of users are not familiar with single floating point precision, and we often get bug reports about that where people compare floats and are surprised by inequalities. If we were to show that so clearly, sure some would end up learning about float point precision, but we'd also get lots of users telling that Godot is outright broken. Usability wise, that would be a very bad decision.

• Eventually what you want is just more precision by default, and this can be achieved by making the default 0.001 step configurable. I'll make a PR that implements that.

• Your changes to step_decimals() break it, as they don't account for the precision issues that the previous implementation explicitly catered to. With your implementation of step_decimals() + my configurable default step, I would get eg. 5 decimal digits for a configured step of 0.0001 (because precision issues would turn it into 0.000099.., which gave 5 in your implementation).

[akien-mga]

Eventually what you want is just more precision by default, and this can be achieved by making the default 0.001 step configurable. I'll make a PR that implements that.

See #30776. It should achieve most of what this PR attempted to do, without introducing regressions or usability issues.

There's only the part about shader hint ranges that I left unmodified for now, as I'm not sure how to handle it. It could default to 0, but as far as I understand that would always take precedence over the configured default float step, so unless another step is specified manually, it would always be 0 / no limit.

[aqnuep]

• The default step for EditorProperty controls is used not only to format the visual representation of floats, but also to specify the step of the EditorSpinSlider. With this PR, all properties without a manually specified step lose their sliders. The relation between slider step and decimal representation in SpinBox and EditorSpinSlider is a known limitation, tracked in #19242.

For values which don't have an explicitly defined range and/or step (i.e. unbounded, unrestricted default behavior of floats) thus not having semantics what's the point of having a slider?

• Showing floating point precision errors on all float properties is definitely not wanted. The majority of Godot users is happy to input 300.1 and see that in the inspector, even if it's 300.000937 behind the scenes. A surprisingly high number of users are not familiar with single floating point precision, and we often get bug reports about that where people compare floats and are surprised by inequalities. If we were to show that so clearly, sure some would end up learning about float point precision, but we'd also get lots of users telling that Godot is outright broken. Usability wise, that would be a very bad decision.

That's debatable and one way to fix it is to round the value displayed but not the value stored if user friendliness of this sort is preferred. That I'd say is probably okay, but I disagree with the principle of trying to hide the fact that floating point precision limitations exist at all costs. Even today you can encounter values being snapped to another one when the step is sufficiently small to cause floating point rounding errors, so hiding that fact is not really possible in all cases and I'd argue it's counter-productive. But it's your call as I think this is sort of a positioning question on who are the target audience of the engine.

• Eventually what you want is just more precision by default, and this can be achieved by making the default 0.001 step configurable. I'll make a PR that implements that.

Thanks, appreciated, personally I don't need it as I forked the code anyway due to this and other issues where I guess my requirements are different to that of majority. Though I agree that some solution to this problem will likely come handy to other users as well.

• Your changes to step_decimals() break it, as they don't account for the precision issues that the previous implementation explicitly catered to. With your implementation of step_decimals() + my configurable default step, I would get eg. 5 decimal digits for a configured step of 0.0001 (because precision issues would turn it into 0.000099.., which gave 5 in your implementation).

Huh, really? I thought I tested the precision with all the specific values previously hard-coded in the function, but I may recall it incorrectly. It's been a long time I wrote that code. Still, it should be trivial to fix that using an appropriate scalar. I'll look into it, at least so far it didn't bite me.

[aqnuep]

• Eventually what you want is just more precision by default, and this can be achieved by making the default 0.001 step configurable. I'll make a PR that implements that.

Oh, just one more note on this with respect to my particular requirements. I'm not really looking for more precision by default, but for maximum precision allowed by 32-bit floats, and AFAICT you can't achieve that with a configurable negative-power-of-ten step value, because the value that'd allow full precision is simply itself not representable as a 32-bit float. Right?

Edit: If zero is an allowed value for the default step to indicate full precision like it does internally then that resolves the problem.

[akien-mga]

For values which don't have an explicitly defined range and/or step (i.e. unbounded, unrestricted default behavior of floats) thus not having semantics what's the point of having a slider?

I don't have a strong opinion on that one, but that's the way Godot has always worked. EditorSpinSlider is designed so that you can slide the pin or drag on the LineEdit to increment/decrement the value. Say you want to adjust the position of an object on X without need for a high precision, you can use the slider, or better drag on the control to adjust this position relatively finely (I think the drag behavior has modifiers to adjust the step too). In this case it makes sense to have a reasonable default step, even if the actual value doesn't need to be bound.

Note also that the property hint range is of the form min,max,step, so currently it's not possible to only set the step via PROPERTY_HINT_RANGE, you'd be forced to set a min and max value too (in which case or_greater and or_lesser can be used to allow manual input beyond the min and max bounds.

That's debatable and one way to fix it is to round the value displayed but not the value stored if user friendliness of this sort is preferred. That I'd say is probably okay, but I disagree with the principle of trying to hide the fact that floating point precision limitations exist at all costs. Even today you can encounter values being snapped to another one when the step is sufficiently small to cause floating point rounding errors, so hiding that fact is not really possible in all cases and I'd argue it's counter-productive. But it's your call as I think this is sort of a positioning question on who are the target audience of the engine.

AFAIK that is what happens already. The editor rounds the issue displayed in the widget, but the value saved in your scene is the original one. Even for your original example in #18251, you were already allowed to input 1/16 in the field and have its decimal single floating point representation saved in memory. The rounded display in the editor is only cosmetic (with the gotcha that if you then try to edit the value again, it would start from the rounded value, so you'd lose the finer precision you had previously when saving again -- that might be the source of the confusion, and I agree it's something that should likely be addressed to some extent).

Edit: If zero is an allowed value for the default step to indicate full precision like it does internally then that resolves the problem.

Zero is an allowed value, yes, that's why I added https://github.com/godotengine/godot/pull/30776/files#diff-422087b7a934519b4112d21b9e9073eeR82 (I didn't want to change the behavior of step_decimals() which is bound to scripting languages and may be used by end users).

[akien-mga]

Note that I don't consider #30776 a complete solution for the problems discussed here and in #18251. IMO solving #19242 is really necessary, and this can be done by removing this dependency between range hint and the actual values that can be typed and shown.

A good UX would be IMO:

• That by default hint ranges define the behaviour of the slider/spinbox/drag, but don't restrict manual input and the display of actual values.
• For properties we were actually want to restrict for some reasons (e.g. can't be negative, or must be between 1 and 16), additional hint options should be added (so that it's non-restricted by default, and restrictions to manual input are added on purpose).
• The display of floats should not be rounded based on the hint range's step, but to their last non-zero significant digit.

[akien-mga]

Closing as superseded by #30776. I'll link the discussion here in related issues though to keep track of what would be left to do.