YarnSpinnerTool / YarnSpinner

Yarn Spinner is a tool for building interactive dialogue in games!
https://yarnspinner.dev
MIT License
2.3k stars 201 forks source link

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

Closed fmoo closed 11 months ago

fmoo commented 1 year ago

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:

Other information

McJones commented 11 months ago

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 commented 11 months ago

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)