Skip to content

Conversation

@Sergio0694
Copy link
Member

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

The TimedKeyFrame APIs are using an incorrect expression, resulting in the animation not playing.

What is the new behavior?

Fixed the expression to calculate the normalized timestamp of timed keyframes, so they work fine now 😄

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)
  • 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

@Sergio0694 Sergio0694 added bug 🐛 An unexpected issue that highlights incorrect behavior animations 🏮 labels Apr 3, 2021
@ghost
Copy link

ghost commented Apr 3, 2021

Thanks Sergio0694 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 🙌

@ghost ghost requested review from Kyaa-dost, Rosuavio, azchohfi and michael-hawker April 3, 2021 10:22
@ghost
Copy link

ghost commented Apr 5, 2021

Hello @RosarioPulella!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@Sergio0694 Sergio0694 force-pushed the bugfix/timed-keyframe-animations branch from 8e46c5d to 1c5269a Compare April 5, 2021 14:50
@Sergio0694 Sergio0694 force-pushed the bugfix/timed-keyframe-animations branch from 1c5269a to 09cb085 Compare April 14, 2021 17:11
@Sergio0694 Sergio0694 force-pushed the bugfix/timed-keyframe-animations branch from 09cb085 to b57da01 Compare April 16, 2021 12:57
@michael-hawker
Copy link
Member

@Sergio0694 @JustinXinLiu seems like something serious enough we may want to patch? Thoughts?

@michael-hawker
Copy link
Member

@Sergio0694 was this exposed in one of the samples? Is there a way we can visually validate the fix for comparison?

@michael-hawker michael-hawker added this to the 7.1 milestone Apr 22, 2021
@Sergio0694
Copy link
Member Author

"seems like something serious enough we may want to patch"

Yes, if it was possible to do a hotfix release before 7.1 I'd say this should probably be included.
Right now keyframe animations with timed keyframes are essentially not working otherwise 😅

Granted, it's a pretty niche scenario since you need to:

  1. Be using AnimationBuilder manually in code behind
  2. Be using custom keyframe animations
  3. Be specifically using timed keyframes on top of it

Which I guess also explain why we didn't notice it before.
But yeah, the sooner this fix is out the better 😄

"was this exposed in one of the samples?"

It wasn't, as these APIs are not available from XAML (it only supports normalized keyframes, which are the default).

"Is there a way we can visually validate the fix for comparison?"

It's not much about a visual difference, it's more that without this fix the animation wasn't playing at all 😅
I'll see if I can also add some unit tests. Not the easiest scenario to test there since we need to validate "while an animation plays" to ensure it's going as we want it to (as opposed to other tests that just await for it and then check final values), but I can try out a few things with a timer I guess. It could definitely help with regression testing at least.

@michael-hawker
Copy link
Member

Thanks @Sergio0694 let's merge this in to get the fix so things work, and think about general animation testbed for checking things 'in-flight' for the future.

@ghost ghost merged commit 68888d2 into CommunityToolkit:master Apr 23, 2021
@michael-hawker michael-hawker modified the milestones: 7.1, 7.0.2 May 11, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

animations 🏮 auto merge ⚡ bug 🐛 An unexpected issue that highlights incorrect behavior hotfix 🌶 priority 🚩

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants