Prevent floats from being represented in scientific notation in expression animation strings

[arcadiogarcia]

Fixes

PR Type

What kind of change does this PR introduce?

Bugfix

What is the current behavior?

Currently we use the default ToString() representation of floats to serialize the values in expression nodes into the expression animation string. When the values are too small or too large, .net uses scientific notation, which is not recognized by the composition API. E.g.

visual.StartAnimation("Orientation", (QuaternionNode)Quaternion.CreateFromYawPitchRoll(0, MathF.PI, 0));

will throw

System.ArgumentException: 'The parameter is incorrect.

One or more parameters have not been set on the ExpressionAnimation.
Context: E
Expression: Quaternion(1,0,0,-4.371139E-08)
Start Position: 26, End Position: 27'

What is the new behavior?

I did some research and couldn't find any format parameter that would prevent ToString from sometimes using scientific notation (which I find very surprising, if someone knows of a built in way to accomplish this please leave a comment). A new extension method that always returns non-scientific notation representation of floats, and is used in the relevant GetValue() calls.

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link:
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

Other information

[ghost]

Thanks arcadiogarcia for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[Sergio0694]

Asked @tannergooding about formatting options, he mentioned there's "G768" on .NET 6 to guarantee that scientific notation isn't used. That can result in a formatted number literally having 768 digits though, so even if we could use .NET 6 here that wouldn't really be a good fit. Following up from a suggestion from him, we should probably just rethink the whole approach here and possibly stop passing formatted values entirely. There is the SetScalarParameter API that just allows setting a float parameter with a name, can't we use that instead, simply set the float value as is, and then reference it by name in the expression? 🙂

@michael-hawker These expression extensions are from before my time in the Toolkit so I don't know the story behind then, do you know whether that API wasn't being used on purpose, maybe to work around some other limitation or something, or whether it might simply have been an oversight? Anyway I think it might be worth exploring using this other approach, as it could likely be more reliable, precise and definitely more efficient 😄

[tannergooding]

he mentioned there's "G768" on .NET 6 to guarantee that scientific notation isn't used.

You can also just use F## or a custom-numeric format string. However, this will potentially result in truncation and using strings in general is a potential performance and determinism issues. Determinism in part because of bugs and back-compat preservation on full framework or .NET Core versions prior to 3.1.

SetScalarParameter (and similar APIs for other primitive types) seem like the right choice here as it avoids any problems around strings (scientific notation, negative zero, infinities, nans, etc) and preserves the value as given.

[michael-hawker]

@Sergio0694 these APIs were created to create a better code-based way to create the composition expression animation strings required for composition animations which are connected to other properties of object. Our Toolkit docs on the existing feature: https://docs.microsoft.com/en-us/windows/communitytoolkit/animations/expressions

[XAML-Knight]

This extension method solves an immediate need. An alternate approach can be implemented at a later date.

[Sergio0694]

As per previous discussion, we agreed that this fix is fine given that the scenario was just broken before this PR anyway, and we might always rework/optimize this in the future. I like that the happy path of the new extension is just the previous ToString() with an extra IndexOf (negligible), but without any additional noteworthy work there. The fallback path is not ideal as Tanner mentioned too, but then again it does unblock folks using this today and is definitely better than the current situation. Looks good! 😄

[michael-hawker]

Ugh, we didn't notice that this PR had duplicate commits with #4183 here: c4f6009

with a removal here: 7c205fe

That's messed with the history and screwed up the other PR now.

FYI @arcadiogarcia @RosarioPulella @XAML-Knight in these cases if you've accidently crossed the streams, don't make a revert commit (they're evil). Instead, create the new branch for the 2nd feature and interactive rebase on top of it instead to separate out the commits of the two features, or with the copied branch reset it back to the main common starting point, clean-up the code and make a new independent commit.

[arcadiogarcia]

@michael-hawker Oops my bad, I'm too used to squash on merge. Will fix the other PR's branch.

[michael-hawker]

@arcadiogarcia yeah we normally don't squash PRs to maintain history and commit records; we also changed recently to streamline not having to constantly update from the main branch. This is a good thing for us to know though to keep an eye on. Thanks for fixing the other PR, will be on the lookout!