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

Use InvariantCulture for float parsing #23

Merged
merged 1 commit into from
Oct 9, 2021

Conversation

nedoxff
Copy link
Contributor

@nedoxff nedoxff commented Oct 9, 2021

Why?

Depending on the culture, C# chooses the decimal separator to be either , or .. For example, for the russian culture, it uses ,. And since Tiled Map Format uses . (for variables such as x, y, width, height, offsetX and offsetY), it throws an exception that the parsing failed.

Solution

Using an invariant CultureInfo, float.TryParse() will use . for the decimal separator, which will not throw an exception.

@TheBoneJarmer
Copy link
Owner

Hi @NedoProgrammer

Thank you for opening up a pull request. And well spotted! Up till now I never used floating point decimals within my tiled map so I never hit the issue you are solving with this pull request. Changes look good so I will approve the changes and draft a new release.

That said, I noticed you targeted my main branch. Under normal circumstances I don't allow this as main is the production-stable branch and develop is the branch used for well, active development and testing. However, considering the size of the change and the fact it does break stuff for you I allow it. But next time, please target the develop branch even if the change is breaking.

With kind regards,
Ruben Labruyere

@TheBoneJarmer TheBoneJarmer merged commit bea9404 into TheBoneJarmer:main Oct 9, 2021
@nedoxff
Copy link
Contributor Author

nedoxff commented Oct 9, 2021

Ok, sorry! Didn't notice there was the dev branch 😅

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

Successfully merging this pull request may close these issues.

2 participants