Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StandardMeshWriter writes using current thread culture #28

Open
sandord opened this issue Jan 9, 2018 · 9 comments
Open

StandardMeshWriter writes using current thread culture #28

sandord opened this issue Jan 9, 2018 · 9 comments

Comments

@sandord
Copy link
Contributor

sandord commented Jan 9, 2018

StandardMeshWriter writes using current thread culture, which results in data such as

v 461,939769107802 0 -191,341709296824

since I'm located in The Netherlands and my PC's culture uses a comma as a decimal separator.

Obviously, this should be:

v 461.939769107802 0 -191.341709296824

Therefore, I think the writer should use the invariant culture at all times.

@rms80
Copy link
Contributor

rms80 commented Jan 10, 2018

agreed, that would be better. Would you mind checking something for me? Can you try putting this line before the code where you are writing out your OBJ, and see if it works? If it does, I will push/pop this state internally inside StandardMeshWriter, but I'm not sure how to test it on my local machine (ie my culture is dot)

System.Threading.Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture;

@sandord
Copy link
Contributor Author

sandord commented Jan 10, 2018

That is exactly how I currently hack this issue so I confirm it works.

@rms80
Copy link
Contributor

rms80 commented Jan 17, 2018

OK, I added this. StandardMeshReader and StandardMeshWriter internally push/pop the current culture. Can be disabled via flags.

If you do get around to checking that this works, would be appreciated if you let me know. Thanks for reporting!

@sandord
Copy link
Contributor Author

sandord commented Jan 23, 2018

I haven't had the time yet to test it but I wonder why you're changing the current culture and not use the IFormatProvider argument for string conversions instead (e.g. .ToString(CultureInfo.InvariantCulture)). The latter is generally the preferred way.

@sandord
Copy link
Contributor Author

sandord commented Jan 29, 2018

This bug affects debugging as well. As you can see it makes reading the values pretty difficult.

image

@rms80
Copy link
Contributor

rms80 commented Jan 29, 2018

yes, that does seem irritating.

As to our first question, I agree that is the preferred way, it's right in theory...but I am a realist. It is almost certain that at some point in the future I or another developer would forget to add the culture flag to some string. Doing it at the thread level for the exporter is safer.

your second problem...I'm not sure what to do about. To handle it I have to pass in InvariantCulture all over the place, and this is not necessarily something that every user will always want. All I can think of is adding a global static field for the entire library...

@sandord
Copy link
Contributor Author

sandord commented Jan 29, 2018

Not to be witty or anything but can you give some examples of cases in which users would want this library to use a culture different from InvariantCulture? I can't think of any, the library doesn't do any UI as far as I know.

I agree that sooner or later someone will forget the culture specification but you could consider adding an automated StyleCop rule for that.

@rms80
Copy link
Contributor

rms80 commented Jan 29, 2018

It is entirely possible that someone will want to use this library to generate text files containing coordinates that will need to be read into some other software that expects numbers to be formatted in a specific culture.

One example I have personally dealt with is to work around bugs - for example if the reading software doesn't handle this properly, you might need to generate an OBJ with comma decimal separators so that it's text-reader parses them properly. We made this mistake w/ meshmixer, it would work fine as long as you only read back in files generated in the same culture. But english files opened w/ german meshmixer would go all blocky.

@sandord
Copy link
Contributor Author

sandord commented Jan 29, 2018

That sounds like a very solid reason. Defaulting to invariant culture with an option to override it (which I think is a common approach) sounds like the right way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants