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

Variables corrupted (in some locales) when calling Custom Commands with float arguments #369

Closed
fmoo opened this issue Jul 12, 2023 · 2 comments
Labels

Comments

@fmoo
Copy link

fmoo commented Jul 12, 2023

What is the current behavior?

When a variable is declared as a float, and passed to a custom command that expects a float. No string conversions are expected nor should be implied.

Consider the following script:

<<declare $myvar = 0.55>>
<<LogFloat {$myvar}>>

I would expect the handler to receive the float value of 55 hundredths (0.55), not 55.

The latter occurs in some locales (e.g., Austria, Argentia, and others) that use commas as decimal separators, and periods as thousands separators.

Please provide the steps to reproduce, and if possible a minimal demo of the problem:

In YarnSpinner-Unity, define the following commands in c#:

			runner.AddCommandHandler<bool>("SetCommaDecimals", CommandSetCommaDecimals);
			runner.AddCommandHandler<float>("LogFloat", CommandLogFloat);

//...
		void CommandSetCommaDecimals(bool enabled) {
			if (enabled) {
				Thread.CurrentThread.CurrentCulture = new CultureInfo("de-AT");
			} else {
				Thread.CurrentThread.CurrentCulture = new CultureInfo("en-US");
			}
		}
		void CommandLogFloat(float value) {
			Debug.Log(value.ToString(System.Globalization.CultureInfo.InvariantCulture));
		}

And run the following YS script:

<<set $myvar = 0.55>>
<<SetCommaDecimals true>>
<<LogFloat {$myvar}>>
<<SetCommaDecimals false>>
<<LogFloat {$myvar}>>

When running this, unity outputs:

55
0.55

What is the expected behavior?

When running this, I would expect unity to output:

0.55
0.55

Please tell us about your environment:

  • Yarn Spinner Version: 2.1
  • Unity Version: 2019.4.36f1

Other information

@fmoo fmoo added the bug label Jul 12, 2023
@fmoo fmoo changed the title Varables unexpectedly corrupted when calling Custom Commands with float arguments in some locales Variables corrupted (in some locales) when calling Custom Commands with float arguments Jul 13, 2023
@McJones
Copy link
Collaborator

McJones commented Sep 27, 2023

Ok so this is a bit of an interesting bug that is the side-effect of a quirk of Yarn Spinner and I just wanted to add a little bit of context to this "fix".
As it currently stands string interpolation is always culture dependant, and we feel that this is the right move as the majority of the time a value will be interpolated into a user facing string, hence we want it to respect culture.

The wrinkle comes from when using interpolation to inject values into commands.
Because the interpolation happens before the commands are split and parsed these values have to be interpolated as if they are strings.
Which means they are printed using the current culture.
Which then means when it comes time to read the commands and parse them as invariant, information is (or can be) changed.

A temporary fix for this is the newly added format_invariant method, this will always interpolate the number as culture invariant, so <<LogFloat {format_invariant($myvar)}>> will prevent this bug from occurring regardless of current culture.
This is clunky though, so a longer term and better solution will be needed.
At this stage we are not really sure what that should be.

@fmoo
Copy link
Author

fmoo commented Sep 27, 2023

Yeah I basically ended up writing my own helper that does exactly this as a workaround.

It mostly feels icky because we're formatting to string and then immediately parsing it back to a float to call the c# handler.

My main worry about these workarounds is discoverability. Perhaps the vscode plugin could detect float variable usage with command args and flag/wrap them automatically?

Though, if the language server can do this, maybe the compiler could even transparently inject the float2str() call internally? (Still icky but less of a time bomb)

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

No branches or pull requests

2 participants