Fix constant float values being converted to ints

[DeclanRussell]

Description

This issue was caused by https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/commit/8c52d48965db74f56daa59c4a668b34daf32b220 . The problem is that the constant float values are being turned into ints. The PR restores the previous behavior and preserves the float format.

Checklist:

• [x] I have read the contribution guidelines.
• [x] I have previously submitted a Contributor License Agreement.
• [x] I have updated the documentation, if applicable.
• [x] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
• [x] My code follows the prevailing code style of this project.

[AlexMWells]

If there wasn't a failing unit test to catch the original issue, could one be added to check this?

[lgritz]

This is a totally reasonable change, but...

Can you show us a snippet of before/after of the oso, explain how the symptom manifests in practice?

The "{}" should be the equivalent of "{:g}". I think that changing to "{:.9g}" primarily serves to make sure it's expressed as a floating point number instead of rounding to an integer if it's an integral value (e.g., 1.0 would be "1"). But how is that harmful? Is there someplace downstream when reading these oso files that are getting confused by the fact that the number looks like it could be an int rather than that it could only be a float because it has the decimal point? Maybe in some sense that is the real bug to deal with?

[DeclanRussell]

I've attached an example that shows the problem that manifesting. I have 3 files, an example .osl shader, and the compiled oso before and after the change. The osl shader simply assigns the value -1e12 to a float, and then prints that value. Looking at the oso produced, you can see the value assigned to the myfloat variable changes between versions. Without the :.9g the assigned value is the integer value -1000000000000, while with the change it is a floating point value -9.99999996e+11. This may not seem like too much of a difference, but if we actually run this shader and look at the printed value, the value printed before the change is -2147483648.000000 i.e. -2^31. So it seems that before the change the assigned const value is first being cast to an integer, and then the value is being assigned to the float variable. After the change, the printed value is the full floating point -999999995904.000000 value. I will look into creating some kind of unit test for this.

test.osl.txt oso_old.oso.txt oso_new.oso.txt

[lgritz]

I see, weird!

I was still not quite getting why this was problematic, so I poked around a bit, and here's what I think is a more full explanation:

When outputting the oso and using fmt library to write the value, {} (equivalent to {:g}) prints the "simplest" text representation that, when re-parsed as a float, will give you the SAME binary float value. So the "true" value of -9.99999996e+11 is printed as -1000000000000, which when reparsed will give you the closest float to that, which is the original value. (Remember that the value of -1e12 is not exactly representable as a float.)

But if both will parse to the same float, why does the shader print -2147483648.000000 instead of -999999995904.000000? That's the interesting part.

The answer, and the real core of the problem, is that when we parse the oso file it sees -1000000000000 and thinks it's an int, lexically. Now, once it gets to figuring out what to do with that, if the initial value of a float parameter looks like an int, it's still no problem, it will convert the int to the equivalent float.

Except that in this case, because the text that looked lexically like an int is out of range for a 32 bit int, it gets INT_MIN, so by the time it's converted back to float, it's -2147483648.000000

Your fix works (at least in this instance) because it forces the value to print in a way that lexically can only be a float and not an int.

I'm going to keep tinkering with this just a bit. I feel like a more full solution is going to involve more robust handling of out-of-range int values in the oso parser.

[DeclanRussell]

Thanks for finding the root cause of the issue, that makes much more sense as to what is going wrong 🙂