Skip to content

Conversation

@ThakeeNathees
Copy link
Contributor

Fix: #40589

writing and parsing -nan(ind) and -inf damage the .tscn file as the first negative eaten as float (-0.0) and the reset identifier is unexpected and parsing(reading) these special case would break stuff so I've changed them to nan, "inf_neg" respectively.

(regardless of the sign of the NAN, godot using -nan(ind) for both)

@ThakeeNathees ThakeeNathees force-pushed the nan-inf-tscn-parsing-bug-fixed branch 2 times, most recently from 9385a08 to 1c90493 Compare July 23, 2020 05:06
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rtoss uses sprintf, and it can have more output variants depending on system / C runtime:

  • C99 standard CRTs should output inf, nan, nan(snan), nan(ind).
  • Old Visual Studio CRTs (MinGW may use it too) will output it as 1.#INF random-digits, .#IND random-digits or .#NAN random-digits.

And any of these can be prefixed by -.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm checking before stringifying the the value, but not sure could it be there any negative nan
@bruvzg

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think there's any meaning to the sign of NaN if it is set, it's just artifact of previous operation. isnan should handle any NaN correctly and there's not reason to save it.

@pouleyKetchoupp pouleyKetchoupp added this to the 4.0 milestone Jul 23, 2020
@ThakeeNathees ThakeeNathees marked this pull request as draft July 23, 2020 09:40
@ThakeeNathees ThakeeNathees force-pushed the nan-inf-tscn-parsing-bug-fixed branch from 1c90493 to 508dcbc Compare July 23, 2020 10:44
@ThakeeNathees ThakeeNathees marked this pull request as ready for review July 23, 2020 10:44
@aaronfranke
Copy link
Member

@ThakeeNathees Is this still desired? If so, it needs to be rebased and tested on the latest master branch.

@briansemrau
Copy link
Contributor

briansemrau commented Mar 30, 2021

This is something I'd like to see merged in in 3.x as well. This issue is currently annoying me in my own project. If necessary, I'm willing to fork this branch and open new PRs.

@Calinou
Copy link
Member

Calinou commented Mar 30, 2021

@briansemrau Since ThakeeNathees appears to be inactive, I'd suggest opening a rebased version of this pull request against the master branch, then open another pull request against the 3.x branch.

@akien-mga
Copy link
Member

Superseded by #47497.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using INF as float breaks scene

7 participants