gradientspace / geometry3Sharp

C# library for 2D/3D geometric computation, mesh algorithms, and so on. Boost license.
http://www.gradientspace.com
Boost Software License 1.0
1.71k stars 384 forks source link

OBJReader should parse doubles and floats using culture invariant #133

Open PedroQuelhas opened 4 years ago

PedroQuelhas commented 4 years ago

We had an issue using the library in two different machines, where one would load the obj perfectly and the other would not. Turns out, that the reader e doing double and float parses without being culture invariant. This leads to various issues.

Will make a fix and submit a pull request as soon as possible.

PedroQuelhas commented 4 years ago

Did the fix.. its working. Not able to push my branch or make a pull request.

rms80 commented 4 years ago

If you were to use StandardMeshReader you would find that it already handles this problem.

PedroQuelhas commented 4 years ago

Yes and i tried using it.

///

/// Read mesh file at path, with given Options. Result is stored in MeshBuilder parameter /// public IOReadResult Read(Stream stream, string sExtension, ReadOptions options)

Tried this method, could no access the mesh although it states that "Result is stored in MeshBuilder parameter". Since the only thing i wanted was to read an OBJ i went directly to the public class that does it, and stumbled upon the parses without InvariantCulture flag.

After your answer, i went again to the drawing board, and got it to work with the standardmeshreader.

Just my 2 cents:

Exposing IMeshBuilder which does not contain a property with the builded meshes and stating "Result is stored in MeshBuilder parameter" does not make sense. You have to cast IMeshBuilder to DMesh3Builder to access that, and, unless you have access to the source code, you have no way of confirming the cast will work.. if you guys change the concrete class you are using from DMesh3Builder to another, it will break everyones code. A bit more documentation about this would go a long way :)

Changing the current thread locale is bad policy by default as it can affect more that the code you are running (C# Tasks run in a thread pool, you never know if you have something running on the thread you just changed that can start behaving strangely or blow up).

On another note, changing a thread locale to "fix" the behavior of the parses that happen on another class (for this matter a public class that can be used stand alone) is an anti pattern and should be avoided, as you cannot easily understand the causality of things.. Having invarianteculture as an option in this case is also strange, since parsing numbers is not optional unless you want to be sure it only works in the US. I understand that it would be a bit painful to change all the .parse in all the files, but that ,in my opinion, would be the way to go.

Sorry for the long response and for being a bit anal about it.. just trying to make it better. Keep up the good work.