VariantParser: NaN, INF read/write bug fixed

[briansemrau]

Fix: #40589, supersedes #40619.

This is a rebase of #40619 due to inactivity on that PR.

This was tested on Windows 10 with Visual Studio 2019.

[theraot]

Would you rebase (this and the 3.x PR) again? Github is reporting a conflict in the modified file.

By the way, I ran into a related issue: Infinity gets stored as 1.#INF but parsed as 1.0. I had a look at the changes in this pull request, and I believe it fixes the problem.

[theraot]

I tried the artifacts made by github for this (and also for the 3.x branch), and I can confirm it solves the both the original bug (Which was about RayCast2D), and the related issue I was having.

However:

• In the inspector, I need to type INF (typing inf does not work) but it shows as inf. Perhaps you could serialize and deserialize in all caps instead?
• RayCast(3D) outputs the error "The axis Vector3 must be normalized." a few times upon opening a scene that has one of the cast_to attributes set to inf (Does not happen with RayCast2D). I don't think you should be addressing that.

[theraot]

I went and tried both RayCast(3D) and RayCast2D with infinity on both the 3.x and this one, and they don't work. So, yeah, they can spam "The axis Vector3 must be normalized.", it is fine. If anything RayCast2D should complain too.

[briansemrau]

@theraot I don't believe the serialization is related to how values show/input in the inspector (but I could be wrong). You may want to look for/submit bug reports for those issues.

[theraot]

@briansemrau This PR does not change how I type infinity in the inspector (I type the constant INF, other constants work, e.g. PI). But it does change how it shows. In both Godot 3.3.3 and Godot 3.4 beta5 infinity shows as 1.#IO (and there were issues about that). But with this PR it shows as inf. Thus I believe it is your change, and I would prefer if it matched the constant INF.

However, in other builds of the master branch it did show as inf even without this PR. Which I thought was a compiler issue, at least according to @bruvzg, see #40589 (comment). But perhaps it no longer uses the serialization in Godot 4.0.

Anyway, I suppose this could be merged and then I could open an issue afterwards.

CC @Calinou @akien-mga

[akien-mga]

What do sfixtor and sfix mean?

[akien-mga]

I guess it's a misunderstanding of what rtosfix means, trying to reverse this around?

rtosfix means "Real TO String FIX", i.e. it fixes the conversion of reals to strings.
So this should be storfix with e.g. parameter p_str.

Or both should be renamed to be more explicit.

[briansemrau]

This code is carryover from the abandoned PR. I think keeping naming inline with core/string/ustring.h is a good idea, but perhaps a function renaming PR is in the future if it's not clear enough.

[briansemrau]

Updated so sfixtor is renamed to stor_fix and rtosfix is renamed to rtos_fix.

[akien-mga]

Is there a specific reason why this is inf_neg and not -inf?

[briansemrau]

Looks like it's to avoid having this be parsed as a TK_NUMBER token instead of TK_IDENTIFIER.

[akien-mga]

Added some review comments, but overall this PR looks good to me. It's an important bugfix too.

This PR does not change how I type infinity in the inspector (I type the constant INF, other constants work, e.g. PI). But it does change how it shows. In both Godot 3.3.3 and Godot 3.4 beta5 infinity shows as 1.#IO (and there were issues about that). But with this PR it shows as inf. Thus I believe it is your change, and I would prefer if it matched the constant INF.

INF and NAN are GDScript (and here actually Expression) constants, so per se they don't have to match how Variant serializes C++ inf and nan as literals. It could be made to match but it's a not requirement that Variant should match GDScript/Expression constants (for example if you input PI in the inspector, it will be serialized as the actual value of PI as 32-bit or 64-bit float, not the PI constant).

[theraot]

@akien-mga

I'll elaborate why I would prefer it to match:

With PI, for example, I type PI and the inspector changes it to 3.142. The inspector changed it, that is fine. Now, if I type 3.142, I get 3.142. That is, I can type the same way it is represented.

In general, if I see a number in the inspector panel, I can also type that number. And if I type a number, I get that number. Perhaps with some rounding.

However, with infinity I need to type INF. Which the inspector shows as inf. However, if I type inf, it does not work (because inf is not a GDScript constant). It is instinct to try to write it the way I see it written.

---

To be fair, this is not the issue being dealt with here (and there are other avenues to fix it, such as adding inf as a constant). I'm willing to open issues for that and for warning in RayCasts (my current thinking is that those could be proposals, and there could be discussion on how best to approach them).

[akien-mga]

@theraot I understand, but in practice it's the same, you type the PI constant and you get the corresponding float literal. You type the INF constant and you get the corresponding float literal, inf. INF is a constant that maps to the inf literal.

This seems to be the pre-existing behavior on platforms not affected by #40589, without this PR for me typing INF in the inspector does give me inf.

One could use INF as literal to match the constant, but since -INF is apparently not possible without modifying the parser further (#47497 (comment)), it would be INF_NEG which isn't really nice if you typed -INF using the constant.

I also don't think that inputting NAN or INF in the inspector is a very common use case. Those values usually come up as the result of floating point operations, not as starting points. Could still be changed if it's really needed but this shouldn't be a blocker for this bugfix PR (though it's worth noting that changing it would break compatibility).

[akien-mga]

I understand, but in practice it's the same, you type the PI constant and you get the corresponding float literal. You type the INF constant and you get the corresponding float literal, inf. INF is a constant that maps to the inf literal.

For the record, the same would be true in C++:

#include <cmath>
#include <cstdio>
int main() {
    printf("%f\n", INFINITY);
    return 0;
}

Prints inf.

[akien-mga]

Thanks!