briochie / wow.unity

A collection of assets to make working with wow.export easier in Unity.
MIT License
18 stars 8 forks source link

Parsing Floats does Not Take Culture Data Into Account #2

Closed StefanoCos closed 1 year ago

StefanoCos commented 1 year ago

Description:

When spawning game objects from data read from a CSV file, the position values assigned to the new objects are extremely large, leading to a warning in Unity about floating point precision limitations. The issue appears to occur after reading and parsing the position data from the CSV file, performing some calculations, and assigning the calculated values to the position of the new game object.

Expected behavior:

The position values of the new game objects should be similar to the position values specified in the CSV file, within the range of the Unity world coordinates.

Actual behavior:

The position values assigned to the new game objects are extremely large, often in the range of -1.758448e+13, which is beyond the scope of what Unity's single-precision floating point numbers can handle with high accuracy.

image

Cause:

The issue is related to the parsing of the position data from the CSV file. It's possible that float.Parse() is not correctly converting the string values to float, potentially due to culture-specific number formatting issues. In some cultures, the decimal separator is a comma , instead of a period ., which can lead to incorrect parsing of floating point numbers.

StefanoCos commented 1 year ago

Possible Solution:

Ensure that every float.Parse() is correctly parsing the string to a float, regardless of the system's culture settings. This can be achieved by using an overload of float.Parse() that takes a NumberStyles and IFormatProvider parameter, and specifying NumberStyles.Float and CultureInfo.InvariantCulture.

using System.Globalization;

// ...

float positionXValue = float.Parse(row["PositionX"], NumberStyles.Float, CultureInfo.InvariantCulture);
float positionZValue = float.Parse(row["PositionZ"], NumberStyles.Float, CultureInfo.InvariantCulture);
float positionYValue = float.Parse(row["PositionY"], NumberStyles.Float, CultureInfo.InvariantCulture);

( In visual studio ) This can be achieved by using the Find and Replace feature in Visual Studio with regular expressions.

Open Visual Studio's Find and Replace dialog by pressing Ctrl+H.

Check the "Use Regular Expressions" checkbox. It is represented by an asterisk .* symbol.

In the "Find what" box, put: float.Parse((.+?)).

In the "Replace with" box, put: float.Parse($1, NumberStyles.Float, CultureInfo.InvariantCulture).

Click on "Replace All" to replace all occurrences.

This should replace all instances of float.Parse(row["PositionX"])-like expressions with float.Parse(row["PositionX"], NumberStyles.Float, CultureInfo.InvariantCulture)-like expressions.

oyvind-stromsvik commented 1 year ago

I've fixed this locally myself, not for this specific problem, but due to a different problem related to float.Parse() not receiving the expected input in my locale. I wasn't sure if this was the correct solution or not. I thought it felt cumbersome having to specify this "culture info" every single time you called this method, and I was looking for a way to globally set this flag or something, but maybe this is the way it has to be done?

There also seemed to be so many different ways of specifying this, and I couldn't really tell what the differences were. Based on what I found on Google I think I tried:

float.Parse(value, NumberStyles.Float, CultureInfo.InvariantCulture.NumberFormat);
float.Parse(value, NumberStyles.Float, CultureInfo.InvariantCulture);
float.Parse(value, CultureInfo.InvariantCulture.NumberFormat);
float.Parse(value, CultureInfo.InvariantCulture);

And all of them fixed my problems at least, and seemed to do the same thing for all I could tell :p I went with the last one since it was the least verbose of all of them.

Can you submit a PR for this? @briochie has let me know she'll gladly accept PR's :)

briochie commented 1 year ago

wow.export only exporting to Bri'ish english is one part of the cause, but I think it's safe to say that will be the case for the time being, for a lot of technical reasons that are valid. Making this tool work for people all over the world is important to me. I didn't even consider this as something that could cause issues before it was raised to me.

Please, keep these kind of things coming. :) My ignorant American-ass will do my best to keep up.

oyvind-stromsvik commented 1 year ago

I've made a PR for this: https://github.com/briochie/wow.unity/pull/11

Hopefully someone can verify this is the correct approach towards fixing this before merging :)