Fix Animator for progress values less than zero

[MrJul]

What does the pull request do?

This PR fixes the handling of animations having progress values < 0, which was broken after #13775.
This happens with some easings such as ElasticEaseIn or BackEaseIn.

Unit tests have been added.

What is the current behavior?

The animation progress is becoming NaN.

What is the updated/expected behavior with this PR?

Animations with such easings work.

How was the solution implemented (if it's not obvious)?

If present, the existing key frames for 0% and 100% are correctly reused even if the progress is <0% or >100%.
Previously, a second neutral/fill frame was incorrectly added at 0%, effectively making afterTime - beforeTime always 0, resulting in a division by zero.

Fixed issues

• Fixes Translate animation flickers control visibility when using certain easings as of v11.0.6 #15704

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0048465-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[MrJul]

Added more tests and fixes: a single key frame with a 100% cue was failing with the out-of-range easings.
This PR is now ready for review.

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0048467-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[jmacato]

looking good so far! although since we're touching the keyframe fetching code anyway, should we try to handle the integer -> double runtime animation error in this PR as well?

Ex:

            var animation = new Avalonia.Animation.Animation
            {
                Duration = TimeSpan.FromSeconds(1.0),
                Children =
                {
                    new KeyFrame
                    {
                        Cue = new Cue(0.0),
                        Setters = { new Setter(TranslateTransform.YProperty, 10u) } // explicitly uint'd, but happens to int vals as well. 
                        // the TargetProp is a double type prop and the target val is uint and that has been broken since time immemorial.
                    },
                    new KeyFrame
                    {
                        Cue = new Cue(1.0),
                        Setters = { new Setter(TranslateTransform.YProperty, 20u) } // same goes here
                    }
                },
                IterationCount = new IterationCount(5),
                PlaybackDirection = PlaybackDirection.Alternate,
                Easing = easing
            };

[MrJul]

although since we're touching the keyframe fetching code anyway, should we try to handle the integer -> double runtime animation error in this PR as well?

I'd prefer if we make a separate PR for that, as this seems a bit out of scope of this PR :)

IMO, we shouldn't even change the behavior here, as trying to convert the value on each animation tick might cause performance issues: we should instead throw when building the animator (this matches style setters that throw when creating the ISetterInstance).

Also, it only matters for C#, since the XAML compiler already type the value correctly.

I'm happy to discuss that further!

[jmacato]

Also, it only matters for C#, since the XAML compiler already type the value correctly.

Hmm, I honestly still recall this happening on XAML too before, we might need to both corroborate and test that scenario

IMO, we shouldn't even change the behavior here, as trying to convert the value on each animation tick might cause performance issues

I thought boxing numeric types wouldnt have too much severe impact on perf? Also since this is going to be a one time cost (only on building the animator) the cost should be minimal right?

I'd prefer if we make a separate PR for that, as this seems a bit out of scope of this PR :)

Yep, agreed :)

[MrJul]

Hmm, I honestly still recall this happening on XAML too before, we might need to both corroborate and test that scenario

Unless I've misunderstood the scenario you're talking about, if the XAML compiler didn't special case setters, we would have strings everywhere in styles and animations, and setters wouldn't work at all. I've just double checked for sanity though!

The code is here:

Avalonia/src/Markup/Avalonia.Markup.Xaml.Loader/CompilerExtensions/Transformers/AvaloniaXamlIlSetterTransformer.cs

Lines 79 to 83 in 5e323b8

	if (!XamlTransformHelpers.TryGetCorrectlyTypedValue(context, valueProperty.Values[0],
	propType, out var converted))
	throw new XamlStyleTransformException(
	$"Unable to convert property value to {propType.GetFqn()}",
	valueProperty.Values[0]);

Also, using bindings in setters already convert the value correctly (also rechecked).

Off the top of my head, the place I frequently encounter which requires the correct (non-string) type in XAML is Binding.ConverterParameter.

I thought boxing numeric types wouldnt have too much severe impact on perf? Also since this is going to be a one time cost (only on building the animator) the cost should be minimal right?

Indeed, perf would be perfectly fine if we do it only on building the animator.

Another argument for doing nothing is that dynamic converters aren't trimming friendly (they were initially removed from the binding system refactor for this reason). In my opinion, we should do all conversions at XAML compile time if possible - a lot is already done - and throw for incorrect value types passed at runtime.

Regarding C# usages, we could add some helper method to create a setter from a typed property if needed, reducing errors.
e.g. Setter Create<TValue>(AvaloniaProperty<TValue> property, TValue value) => new Setter(property, value).

[jmacato]

In my opinion, we should do all conversions at XAML compile time if possible - a lot is already done - and throw for incorrect value types passed at runtime.

Agreed, Im personally not that well versed in the XAML compiler side but that's a good way to do it.

Regarding C# usages, we could add some helper method to create a setter from a typed property if needed, reducing errors.
e.g. Setter Create(AvaloniaProperty property, TValue value) => new Setter(property, value).

Yeah, this may need an API review with @grokys at least, since this is touching the Bindings infra already.

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0048503-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]