Add toggle to Range for allowing non finite numbers

[quirkylemon]

Related to #88006

[AThousandShips]

Loos generally good, will test further

Kd say it should add a separate option for NaN, and maybe control for what it means, possibly a "allow displaying invalid" while not allowing it to be input, but something to discuss

[Mickeon]

I would not be overly specific and add another option. No one has requested this, and the current variable exists sorely to fix regressions (internally).

[AThousandShips]

This was discussed here, with various of these points

[AThousandShips]

I think we should discuss separating displaying values from inputting values, this is specifically for being able to display non-finite values, not for inputting them, so I think this reinforces this need especially

So my suggestion would be:

• Control the input of infinities
• Control the input of NaN
• Control the display, this would involve having a new method for just setting the value in a custom way that isn't the one used by the value input from other sources, such as text input or sliders

Allowing both NaN and infinities feels unsafe for me, I'm not even sure I think we should even allow NaN as input, it's a value with minimal usefulness as an input, it's mainly useful as a diagnostic thing (well, useful is a strong word even then)

But the whole reason the code was changed before to prevent non-finite values was because it's relatively easy to cause NaN unintentionally (and unknowingly) with various things like sqrt(-1)

Edit: I think that if we are going to limit this PR to just the display in the editor we should do it with a dedicated display method, essentially one that just allows non-finites for just display, and then we can look at a broader improvement that allows input and display control too

Edit 2: To clarify what I mean by "input" here, I mean input via text, or scrolling, etc, as opposed to code based assigning the value

[quirkylemon]

Do you mean we would split allow_non_finite into allow_infinite and allow_nan?

Something else we should discuss is should there be a warning whenever you set a variable to nan?
it is really buggy and like you said not useful outside of representing invalid math equations.

[AThousandShips]

Looks good now, will test later today and do a deeper review!

[quirkylemon]

the gui becomes a bit buggy when you set the number to nan in the script editor, but I think that has something to do with nan not being able to be compared properly.

[AThousandShips]

So my suggestion was a bit different from this result, but it generally follows my idea, but I think the solution for entering values with the no-signal part is a bit hacky, it also changes existing behavior in a way I don't think is appropriate without the controls for this being exposed

I think that if we are going to limit this PR to just the display in the editor we should do it with a dedicated display method, essentially one that just allows non-finites for just display, and then we can look at a broader improvement that allows input and display control too

Currently using set_value or set_value_no_signal from scripts blocks non-finites, but now this is changed, depending on the display value, I'd say it should instead be controlled directly by that, not indirectly as that's error prone

As I suggested I'd prefer a new method for dedicated display updates, without the is_display_non_finite_allowed part, instead adding a new method like set_display_value which ignores the whole non-finite checks

Without exposing the controls for inputting these values I don't think that code should be added in this PR, as I suggested above, they serve no purpose in this change as the editor makes no use of them

So my suggestion would be:

diff --git a/scene/gui/range.cpp b/scene/gui/range.cpp
index 358dca85d9..3dfbb985cb 100644
--- a/scene/gui/range.cpp
+++ b/scene/gui/range.cpp
@@ -86,17 +86,17 @@ void Range::Shared::redraw_owners() {

 void Range::set_value(double p_val) {
        double prev_val = shared->val;
-       _set_value_no_signal(p_val);
+       _set_value_no_signal(p_val, false);

        if (shared->val != prev_val) {
                shared->emit_value_changed();
        }
 }

-void Range::_set_value_no_signal(double p_val) {
+void Range::_set_value_no_signal(double p_val, bool p_for_display) {
        if (!Math::is_finite(p_val)) {
-               if (shared->allow_infinite_input || shared->allow_nan_input) {
-                       if (Math::is_nan(p_val) && shared->allow_nan_input) {
+               if (p_for_display) {
+                       if (Math::is_nan(p_val)) {
                                shared->val = p_val;
                                return;
                        }
@@ -129,20 +129,19 @@ void Range::_set_value_no_signal(double p_val) {
 }

 void Range::set_value_no_signal(double p_val) {
-       bool prev_nan = is_nan_input_allowed();
-       bool prev_inf = is_infinite_input_allowed();
+       double prev_val = shared->val;
+       _set_value_no_signal(p_val, false);

-       if (is_display_non_finite_allowed()) {
-               set_allow_non_finite(true);
+       if (shared->val != prev_val) {
+               shared->redraw_owners();
        }
+}

+void Range::set_value_display(double p_val) {
        double prev_val = shared->val;
-       _set_value_no_signal(p_val);
-
-       set_allow_nan_input(prev_nan);
-       set_allow_infinite_input(prev_inf);
+       _set_value_no_signal(p_val, true);

-       if (shared->val != prev_val || Math::is_nan(shared->val)) {
+       if (shared->val != prev_val) {
                shared->redraw_owners();
        }
 }
@@ -390,39 +389,6 @@ bool Range::is_lesser_allowed() const {
        return shared->allow_lesser;
 }

-void Range::set_allow_non_finite(bool p_allow) {
-       set_allow_infinite_input(p_allow);
-       set_allow_nan_input(p_allow);
-}
-
-bool Range::is_non_finite_allowed() const {
-       return shared->allow_infinite_input && shared->allow_nan_input;
-}
-
-void Range::set_allow_infinite_input(bool p_allow) {
-       shared->allow_infinite_input = p_allow;
-}
-
-bool Range::is_infinite_input_allowed() const {
-       return shared->allow_infinite_input;
-}
-
-void Range::set_allow_nan_input(bool p_allow) {
-       shared->allow_nan_input = p_allow;
-}
-
-bool Range::is_nan_input_allowed() const {
-       return shared->allow_nan_input;
-}
-
-void Range::set_allow_display_non_finite(bool p_allow) {
-       shared->allow_display_non_finite = p_allow;
-}
-
-bool Range::is_display_non_finite_allowed() const {
-       return shared->allow_display_non_finite;
-}
-
 Range::Range() {
        shared = memnew(Shared);
        shared->owners.insert(this);
diff --git a/scene/gui/range.h b/scene/gui/range.h
index 8ecf683e72..15c979a173 100644
--- a/scene/gui/range.h
+++ b/scene/gui/range.h
@@ -45,9 +45,6 @@ class Range : public Control {
                bool exp_ratio = false;
                bool allow_greater = false;
                bool allow_lesser = false;
-               bool allow_infinite_input = false;
-               bool allow_nan_input = false;
-               bool allow_display_non_finite = true;
                HashSet<Range *> owners;
                void emit_value_changed();
                void emit_changed(const char *p_what = "");
@@ -63,7 +60,7 @@ class Range : public Control {

        void _value_changed_notify();
        void _changed_notify(const char *p_what = "");
-       void _set_value_no_signal(double p_val);
+       void _set_value_no_signal(double p_val, bool p_for_display);

 protected:
        virtual void _value_changed(double p_value);
@@ -78,6 +75,7 @@ protected:
 public:
        void set_value(double p_val);
        void set_value_no_signal(double p_val);
+       void set_value_display(double p_val);
        void set_min(double p_min);
        void set_max(double p_max);
        void set_step(double p_step);
@@ -103,18 +101,6 @@ public:
        void set_allow_lesser(bool p_allow);
        bool is_lesser_allowed() const;

-       void set_allow_non_finite(bool p_allow);
-       bool is_non_finite_allowed() const;
-
-       void set_allow_infinite_input(bool p_allow);
-       bool is_infinite_input_allowed() const;
-
-       void set_allow_nan_input(bool p_allow);
-       bool is_nan_input_allowed() const;
-
-       void set_allow_display_non_finite(bool p_allow);
-       bool is_display_non_finite_allowed() const;
-
        void share(Range *p_range);
        void unshare();

And then use this method in the editor

[AThousandShips]

At minimum I'd say the part with allowing non-finites for input is out of place if not exposed

The current method allows entering any kind of value with set_value_no_signal, which is exposed, which is in my opinion breaking compatibility, so it should at least be controlled by the display option

Edit: My bad updated the code above

For an improvement that exposes this functionality to scripting this is good, needs some polish in some areas and needs to respect compatibility, but since we're not exposing these controls it doesn't really make sense to add this functionality, nothing ever sets set_allow_non_finite except Range

[AThousandShips]

At least I think this code needs to leverage the display more

But the fact still remains that this change, without allowing users to configure it from script, means that set_value_no_signal called from scripting will now allow values it didn't before, which IMO is breaking

[AThousandShips]

I'm not requesting changes as I think this is a matter of opinion, though I do think the hacky solution for the changes to set_allow_non_finite needs to be improved

But I'd say that if we're to add this level of control we need to use it, otherwise there's little point to add it without establishing it will be useful

And for adding this and exposing it (which I think is something we should do, to be clear) I'd say the following is needed:

• Exposing these controls
• Adding support for this for export ranges, with a allow_non_finite option to PROPERTY_HINT_RANGE, with relevant controls

There is a more pressing issue though that needs resolving for this, this breaks various Range types that aren't for input of this kind, like ScrollBar

If you use these changes and you call $MyScrollBar.set_value_no_signal(NAN) it will break and can't be scrolled any longer, which is not good

Further SpinBox with a set_value_no_signal(NAN) can't use the up/down arrows any longer but has to be typed into to fix this

Further further: If you use this on a ProgressBar, with NaN it will now display a very negative percentage

So with these changes there needs to be updates to the various Range types to properly handle NaN (this won't be needed for my solution above assuming it's not exposed, as you then can't cause this manually)

[quirkylemon]

sorry it took so long to respond.

How are options added to PROPERTY_HINT_RANGE?
I will update the code to make it less hacky and expose the controls.

[KoBeWi]

Check _parse_range_hint() and its usages.

[jitspoe]

If the value is infinite and allow nan is set but allow infinite is not set, this would allow it to be set, right? Or is that expected behavior that allow nan allows anything, including infinite?

[quirkylemon]

work in progress commit to make sure this is the right way.

so now you set up the range using @export_range and allow_inf_input and allow_non_finite (allows nan/inf in the code editor). right now you need to use or_greater for inf but that is bug I am working on fixing.

Known issues:
setting min/max to inf breaks a lot of different things :(
inf requires "or_greater"

also unrelated to the actual code: why does github say I added 4 commits even though 3 of them are older commits?

[AThousandShips]

They were rebased, they're different from the other ones as they build on top of other ones

Will give this a review when I find the time, hopefully today!

[AThousandShips]

I think this looks quite good, I'd suggest exposing the setters/getters and add properties for them to make them useful in scripting, haven't tested but I think this is a good solution, will test some later and would be good to get some other feedback as well

[quirkylemon]

Wouldn't exposing properties cause issues with nodes ProgressBar? as far as I understood the only uses for this is the debugger and gdscript variables.
also String::to_float() can't handle inf or -inf there is a pretty easy work around but adding support for inf might be worth considering.

[quirkylemon]

I think everything is working properly.

Something to note if allow_inf_input or allow_non_finite ever need to be exposed the editor won't accept non finite numbers even if they are enabled. probably because the editor uses range and by default non finite numbers are disabled.

[Mickeon]

This PR would partially address godotengine/godot-proposals#10068 .

[MrEgggga]

Allowing both NaN and infinities feels unsafe for me, I'm not even sure I think we should even allow NaN as input, it's a value with minimal usefulness as an input, it's mainly useful as a diagnostic thing (well, useful is a strong word even then)

I personally disagree -- both infinities and NaN can be useful as special values for user scripts. There's no real way to have export fields with optional values other than maybe making a custom resource type for them (which would be extremely cumbersome to edit especially for simple fields like floats), so the only real alternative at the moment is to use special values of floats. The game I'm working on has a speedrunning system where every level has two ranks for times, a "gold" time which is shown, and a "perfect" time which isn't (as a sort of easter egg for players who find super-optimal routes through levels), but there are some levels that I don't want gold/perfect times for, and I currently use NaN for this purpose; #81076 made this much harder to do. In an ideal system, there should be no need to use NaN, but I think the current state of Godot makes it necessary sometimes.

In any case, it feels silly to have the designated Thing You Use To Edit Floats not let you enter certain float values (NaN; infinity; zero; 1/3; sqrt(2); anything that isn't a multiple of 0.001), especially for a tool made for software developers -- Godot shouldn't be making it harder for developers to do what they want, and a user probably knows what they're doing when they type NAN into a field. If it is known that a specific field will do things that nobody would ever want when set to NaN, then it makes sense to prevent that from being input (and also there should probably be checks in place so that nothing too disastrous happens in addition to disallowing input), but it isn't known in the case of user scripts that nobody would ever want the value NaN. It's similar logic to why allow_greater and allow_lesser are set to true by default for fields without a specified range.

(all of this is kind of a repeat of godotengine/godot-proposals#10068, sorry if that's bad form or something)