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

Change inf, inf_neg, nan serialization to uppercase, and add INF_NEG GDScript constant #3398

Open
theraot opened this issue Oct 7, 2021 · 5 comments
Labels
breaks compat Proposal will inevitably break compatibility topic:core topic:gdscript
Milestone

Comments

@theraot
Copy link

theraot commented Oct 7, 2021

Describe the project you are working on

See #3390

Describe the problem or limitation you are having in your project

Most float literals can be typed verbatim in the code inspector. Except for inf, nan, and inf_neg. Instead we have to use INF, NAN and -INF.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Change the Variant serialization that was consolidated in godotengine/godot#47497 / godotengine/godot#47497 to use uppercase.

However, notice that platforms that were unaffected by godotengine/godot#40589 were already using the inf serialization. See godotengine/godot#40589 (comment)

Thus, this is a breaking change. Any scene saved with `inf` would fail to load. Perhaps this is acceptable since such scene would not be common. At least it should be acceptable for a version that is already braking stuff.

We can allow migration if we still parse the lower case versions along side the uppercase version, but only serialize to the uppercase versions.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Since the inspector uses the float serialization, with the proposed change Infinity would show as INF (instead of inf), and we would be able to type it as we see it, since INF is already also a GDScript constant. The same logic applies to NAN. Negative infinity would be INF_NEG, which would be a new GDScript constant.

If this enhancement will not be used often, can it be worked around with a few lines of script?

It is not possible to change the Variant serialization with a script.

Is there a reason why this should be core and not an add-on in the asset library?

It is not possible to change the Variant serialization with a script.

@aaronfranke
Copy link
Member

For INF_NEG you can just do -INF. Or const INF_NEG = -INF.

@theraot
Copy link
Author

theraot commented Oct 7, 2021

@aaronfranke I know I can do -INF in GDScript, and I know I can type -INF in the inspector panel. But it won't show -INF. It will show how that value is serialized, which is currently inf_neg.

I can't type inf_neg in the inspector, so I won't be able to type it the way it see it. And I can't define constants for the expressions used in the inspector.

Handling -INF in serialization is harder. Currently it catches these as identifiers, here: https://github.com/briansemrau/godot/blob/master/core/variant/variant_parser.cpp#L522-L535 - However, it should be possible to make an special case for -INF. Is that what you suggest to do?

@briansemrau
Copy link

briansemrau commented Oct 12, 2021

Do you actually see inf_neg anywhere when using the editor? I see -inf. The only place that inf_neg should ever show is in the .tscn file plaintext, or perhaps a few other places that use variant serialization.
The relevant part of the inspector, EditorPropertyFloat, doesn't use VariantParser, but instead uses the formatter from ustring.cpp, seen here:
https://github.com/godotengine/godot/blob/f9aec342dcd51d65c5970dd395e3c7a66cac446c/core/string/ustring.cpp#L1381

This is where the value you type into the inspector gets parsed:
https://github.com/godotengine/godot/blob/f9aec342dcd51d65c5970dd395e3c7a66cac446c/core/math/expression.cpp#L1384

If you're looking for congruency between what you input and what the inspector shows, I would suggest adding a check for just "inf" and "nan" here:
https://github.com/godotengine/godot/blob/f9aec342dcd51d65c5970dd395e3c7a66cac446c/core/math/expression.cpp#L398
Or modify String::num to format INF to "INF", NAN to "NAN".

@theraot
Copy link
Author

theraot commented Oct 12, 2021

Double checked. No, I don't see inf_neg in the editor. I don't know about the best way to fix this. If it does not involve changing scene parsing, then that is good.

@Calinou Calinou added the breaks compat Proposal will inevitably break compatibility label May 11, 2022
@aaronfranke aaronfranke modified the milestones: 4.0, 5.0 Feb 24, 2023
@geekley
Copy link

geekley commented Dec 13, 2024

I would prefer if inf_neg was dropped/deprecated in favor of -INF in serialization too. It already uses - in front of ints and floats, it would be more consistent, so we could have INF, -INF and NAN everywhere (GDScript, GDShader, inspector, resource/scene files like .tscn and .tres).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat Proposal will inevitably break compatibility topic:core topic:gdscript
Projects
None yet
Development

No branches or pull requests

5 participants