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

Inspector doesn't allow entering values of NAN, INF, etc. for float fields #87664

Closed
MrEgggga opened this issue Jan 28, 2024 · 12 comments · May be fixed by #100414
Closed

Inspector doesn't allow entering values of NAN, INF, etc. for float fields #87664

MrEgggga opened this issue Jan 28, 2024 · 12 comments · May be fixed by #100414

Comments

@MrEgggga
Copy link

Tested versions

  • Reproducible in 4.3-dev2, 4.2.1-stable, 4.2-stable, 4.2-beta1, 4.2-dev5, 4.2-dev4. Not reproducible in 4.1.3-stable, 4.2-dev3.

System information

Godot v4.2.stable - macOS 14.0.0 - GLES3 (Compatibility) - ANGLE (Apple, ANGLE Metal Renderer: Apple M1, Version 14.0 (Build 23A344)) - Apple M1 (8 Threads)

Issue description

It is impossible to enter NaN or positive or negative infinity while editing a float field for a node or resource in the inspector. This was previously possible by typing NAN, INF, etc. into the field, but now attempting to do this results in the value not being changed. There also seems to be a problem where values which are set to NAN by other means are displayed as 0 when loaded into the inspector (although it doesn't seem like the value is actually changed when this happens), but I haven't tested which versions this happens in.

Steps to reproduce

  1. Create a new project.
  2. Create a new resource type with a float field.
  3. Create a new resource of that type, open it in the inspector, and type NAN or INF into the float field.
  4. The field's value will not be changed in the inspector or in the .tres file.

Minimal reproduction project (MRP)

test.zip

@PossumsAndMoss
Copy link

PossumsAndMoss commented Jan 28, 2024

I believe I found the pr to partially blame.
#81076

Using a build from today I can trace a floating point value such as 5.0f not enter that early exit. Inputting 'NAN' from editor causes p_val to be nan(0x8000000000000) and thus enter that early exit.

@Mickeon
Copy link
Contributor

Mickeon commented Jan 28, 2024

Out of curiosity, what do you need these values for?

There also seems to be a problem where values which are set to NAN by other means are displayed as 0 when loaded into the inspector (although it doesn't seem like the value is actually changed when this happens)

This is being tracked in #50715

@AThousandShips
Copy link
Member

AThousandShips commented Jan 28, 2024

I'd say we should be careful about allowing inputting NaN, there's really very little legitimate reason for this, and I'd say we should handle input that produces NaNs as well, like sqrt(-1), but printing them is another matter as linked above and tracked

To clarify, this is two different parts, one is for Expression which is internal and should probably handle NaN properly, but I'd say the inspector shouldn't

@Mickeon
Copy link
Contributor

Mickeon commented Jan 28, 2024

Some users may use non-finite numbers to denote undefined values, where 0.0 is meaningful on its own. I think there's merit to restore this in a more intended form.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 28, 2024

It was done by design though, to fix bugs that bricked the editor, this was considered a bug, not a feature, it wasn't intended to support it

If we are to support it you should have an option to opt in, as it's too dangerous otherwise

I'd say we should add a default false option to range to allow non-finite values, but note that it doesn't make sense for many range types, like scroll bars etc., and then a new option to property hint range that allows non-finites

But I think using NaN as an explicit input is a codesmell, not good practice

@timothyqiu
Copy link
Member

Out of curiosity, what do you need these values for?

I'd say that part of the Inspector is designed to display & edit values of the float type. NANs and INFs are valid float values so they should be supported by default.

Bricking the editor is an editor bug. It's a mistake to have been fixed at Range level.

At least, displaying & editing logic could be separated. It's definitely wrong to display actual NAN value as 0 even if we add something to the range hint to explicity exclude it.

@MrEgggga
Copy link
Author

MrEgggga commented Jan 29, 2024

While using NAN and similar values for undefined etc. is probably bad practice, Godot lacks nullable/optional types (besides Variant which is a pain to edit in the inspector) so there aren't many other options for undefined values without overengineering everything. It also seems strange to prohibit users from entering whatever values they like into the fields they create, although it makes sense as the default behavior for game UI and fields which are known to cause problems when NAN is entered.

I would go further to say that the use of Range for fields in the inspector seems really strange in other ways, primarily with values entered in the inspector automatically being rounded. If I type 1.0/3 into a field with rounding enabled, I probably want the value of that field to be changed to 0.3333333333333333, not 0.33. This has actually caused problems for me before when I wanted to enter a precise value into a Timer node but was completely unable to without opening the scene file in a text editor and changing the value. However, this seems like a pretty major change and again there is probably a conflict between the desired behavior for Range in the Godot editor and the desired behavior for Range in other applications made using Godot.

My preferred behavior would be to have the Range class disallow non-finite float values by default but have @export variables allow non-finite float values unless a range is specified -- this would let users do whatever they want with the fields for their own scripts while presumably preventing all of the crashes from entering NAN into fields of built-in nodes. However, seeing as the problem I was having was not a bug and would require adding new features to solve, I'll close this issue and open a new one on godot-proposals.

@MrEgggga MrEgggga closed this as not planned Won't fix, can't repro, duplicate, stale Jan 29, 2024
@MrEgggga
Copy link
Author

@bgie
Copy link
Contributor

bgie commented Feb 5, 2024

This issue mentions a (serious) bug as well, that was not picked up by the tagging team.

Any float variable containing the value NAN or INF is shown as "0" in the inspector and debugger!!!

Especially in the debugger this is rather nasty.
NaN or INF could be the result of a calculation, wanted or unwanted, and the debugger will hide this and show 0 instead.
If someone is debugging their code because their formula contains a bug resulting in NAN or INF, this will be deceptively hidden by the debugger!
You would have to be aware of this bug and double-check with code and not trust the debugger. Code does still pick up and == INF or isNaN work

@bgie
Copy link
Contributor

bgie commented Feb 5, 2024

In case this helps anyone: tooltips in the script editor DO correctly show nan or inf values inside a float variable during debugging.

@Calinou
Copy link
Member

Calinou commented Feb 5, 2024

Any float variable containing the value NAN or INF is shown as "0" in the inspector and debugger!!!

Please open a separate issue for this. To resolve this, we may have to do something like partially revert #81076 to make it effective only outside the editor. Alternatively, we need to figure out a way to allow displaying invalid values without allowing them to be entered – which may be easier said than done. There are other cases where this would be beneficial too (such as Range minimum/maximum values edited after-the-fact).

@bgie
Copy link
Contributor

bgie commented Feb 6, 2024

Tested the bug mentioned above some more and created #88006

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants