Preserve current Animation progress if SpeedRatio or PlaybackDirection changes
#19830
Conversation
timiimit-tam
force-pushed
the
anim-speedratio-fix
branch
from
0731317 to
0daf138
Compare
timiimit-tam
changed the title
Animation progress on SpeedRatio or PlaybackDirection changeAnimation progress if SpeedRatio or PlaybackDirection changes
|
I have cleaned up and force-pushed different commits to make it more easily reviewable. |
|
@Timotej Korda Mlakar, Please read the following Contributor License Agreement (CLA). If you agree with the CLA, please reply with the following: Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by AvaloniaUI OÜ and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant AvaloniaUI OÜ, and those who receive the Submission directly b. Patent License. You grant AvaloniaUI OÜ, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to AvaloniaUI OÜ. You agree to notify AvaloniaUI OÜ in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the Republic of Estonia, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and AvaloniaUI OÜ dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. 2 out of 3 committers have signed the CLA.
Timotej Korda Mlakar doesn't seem to be a GitHub user. |
|
@cla-avalonia agree |
MrJul
added
enhancement
backport-candidate-11.3.x
|
You can test this PR using the following package version. |
|
Hello, I would like to request a review. Should I open an Issue for a discussion about the problem that this PR fixes? |
MrJul
left a comment
MrJul
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
The PR is very well described and the code clean and easy to follow.
That said, please bear with me while I review this, it's been a while since I dove into the animation code and while well written, the animation algorithm is starting to be quite complex :)
| } | ||
| } | ||
|
|
||
| private bool IsIterationReversed(bool isAlternatingPlaybackDirection, long? iterationIndex) |
| } | ||
|
|
||
| // Check if normalized time needs to be reversed according to PlaybackDirection | ||
| var timeUntilIterDelay = iterDuration - iterTime; |
There was a problem hiding this comment.
Yes, leftover from ~8 iterations of logical changes. Removed now.
| } | ||
|
|
||
| if (!_gotFirstKFValue) | ||
| private void ApplyLimitedClamp(ref long animTime, long animTimeOrigin) |
| var iterTime = animTimePositive % iterDurationTotal; | ||
|
|
||
| bool playbackReversed; | ||
| if (animTimePositive < 0) |
There was a problem hiding this comment.
I have trouble understanding this condition. By definition, animTimePositive should always be >= 0, so it's never going to be taken? If taken, this would even make iterIndex negative, messing up with the itersUntilEnd computation below.
There was a problem hiding this comment.
animTimePositive is meant to be the animation time where positive numbers are increasing in direction of proper animation direction. That said, it is negative iff animation is reversed backwards past the initial starting point.
just like animTimePositive can be negative, so can iterIndex. This does not mess up later itersUntilEnd computation. For example when iterIndex == -1, animation is 1 iteration further from the end than it was when animation started. iterIndex is exactly what is illustrated in the picture in original PR description as iter.
Since % modulo can also result in zero when used on negative values, I decrement it so that there is only a single unique iteration 0.
I have added more comments to make this part of code clearer for future readers.
| _iterationCount = _animation.IterationCount.Value; | ||
| { | ||
| if (_animation.IterationCount.Value > long.MaxValue) | ||
| throw new InvalidOperationException("IterationCount value cannot be larger than long.MaxValue."); |
There was a problem hiding this comment.
While we can certainly live with "only" long.MaxValue iterations, why the change? I see _iterationCount + 1 in DoComplete() but that can be rewritten to avoid overflow. The other usage with itersUntilEnd should always be positive as far as I understood (don't hesitate to correct me if I'm wrong).
There was a problem hiding this comment.
I have attempted to make _iterationCount back into ulong now that logic is more or less finalized... You are correct that itersUntilEnd is supposed to be always positive and DoComplete() can be rewritten.
But looking at
var itersUntilEnd = _iterationCount - iterIndex;which if changed back to ulong would be ulong = ulong - long. This can underflow 1 - 2 == ulong.max which makes itersUntilEnd <= 0 false and animation would not stop. Though for this to happen the duration of iteration needs to be shorter than 1 animation frame.
Whole algorithm uses long type throughout so in general I think its best to just use it everywhere just to be on a safe side of checking all edge cases of over/under-flowing.
IMO it is fine to keep it limited to long because:
- why in the world would anyone ever want a truly specific number higher than
long.MaxValueand notINFINITEinstead animTimewill at all times be MUCH bigger thaniterIndex, soanimTimewill over/under-flow way beforeiterIndexdoes. ThereforeanimTimeshould break algorithm beforeiterIndex.
There was a problem hiding this comment.
This if statment can also include a check that would prevent algo breakage. And I see 2 solutions for that. Change condition to check for long.MaxValue / iterDurationTotal and:
- Throw if its higher
- Set
_iterationCount = null
This also raises another issue with the code. For which I'm unsure whether I should also try to handle or if im just being too pedantic. IterationCount.INFINITE will break the animation logic same way as another really high value does. Should I make sure to explicitly handle animTime over/under-flow? I'm pretty sure this issue has been present already in old animation logic. Maybe it would be smart to just skip unneccessary computation for now unless someone actually needs it and reports an issue.
This does not sound correct, |
That makes sense. Could you please add an unit test for this special case? |
Remove leftover vars/params Replace some `if`s with terniary assignment Improve comments Unduplicate `playbackReversed` code
Yes, I imagined that So at start of animation and at its end FillMode will still act normally. My change only affects the middle of animation if user keeps changing |
|
I will add the UnitTest you requested soon. |
|
You can test this PR using the following package version. |
DelayBetweenIterations_Behind_Initial_Point_Is_In_Front_Of_Iterations
|
There are a few additional changes I made besides the unit test:
|
|
You can test this PR using the following package version. |
4 participants
Apologies for not opening an Issue first... Let me know if there's a better way of doing this and I can change anything.
What does the pull request do?
This PR makes it possible to change
SpeedRatioandPlaybackDirectionwhile animation is running without affecting the current animation progress. PR keeps compatibility with old behavior when speed does not change.What is the current behavior?
When
Animation.SpeedRatioorAnimation.PlaybackDirectionchanges, the animation progress/timeline IS affected.What is the updated/expected behavior with this PR?
When
Animation.SpeedRatioorAnimation.PlaybackDirectionchanges, the animation progress/timeline IS NOT affected.How was the solution implemented (if it's not obvious)?
In short
When relevant properties change during a running animation, the animation's current progress is remembered and animation continues from that point on with respect to property changes.
This is all that's needed to handle a changing
SpeedRatio. ForPlaybackDirection, a lot more care has to be put into the logic as explained in the following subsections.Definition of proper animation direction
Proper animation direction is the value of
PlaybackDirectionat first frame of the animation. In code it is indicated by_timeMovesBackwardsfield.PlaybackDirection.NormalandPlaybackDirection.Alternateresult in positive proper animation direction, whilePlaybackDirection.ReverseandPlaybackDirection.AlternateReverseresult in negative proper animation direction. IfPlaybackDirectionchanges direction some time after the first frame of animation, the animation will start playing backwards. If left running, animation will eventually reach the starting position of animation the second time.Behavior at starting position
When animation begins playing there is nothing new to discuss, it plays as it always has. When animation reverses direction and reaches the starting position subsequently, animation will always hold value of initial keyframe (first or last keyframe depending on proper animation direction), no matter the value of
FillMode.Animations with INFINITE
IterationCountwill continue backwards past the starting position of the animation. This is to allow changing animation direction without affecting the infinitely repeating nature of the animation.Animations with finite
IterationCountwill be clamped to the starting position and will wait for direction to change back into proper animation direction. When that happens, animation will again start playing normally the same way as it has when animation started the first time (albeit with a potentially different speed). Test Reversing_Direction_Past_Initial_Point_Clamps_To_Initial_Point has been added to check the correctnes of this behavior.Behavior of delays
Between the starting position and ending position of animation, behavior of both
DelayandDelayBetweenIterationsis always the same, no matter how many times any given delay has been reached. This can be though of as delays being "baked-into" the animation. I believe this is what most people would expect and want.The only new delay behavior is with animations where
IterationCountis INFINITE. When animation plays backwards past the starting point, theDelayBetweenIterationswill be played in the opposite part of the iteration time. Here is a quick/crudely drawn illustration of animation timeline to make it clearer:If user wants to always have the same delay between all iterations, then both
DelayandDelayBetweenIterationsneed to be set to the same value.Unhandled case
Changing between
Alternateand non-AlternatePlaybackDirections are not handled. This means that switching between such two modes (fromNormalorReversetoAlternateorAlternateReverseor vice versa) can result in a "jump" of current animation progress depending on when you switch it.This could be handled in a way that kind of "does" make sense, but I don't see a reason why anyone would need to switch between these two modes.
Finally
Please review carefully in case I missed something, I wouldn't want to break animations which are probably considered a core feature.
I should also mention that I didn't do any testing of
FillMode(besides what I mentioned above), I just assumed that if all UnitTests pass, then it works as expected.Checklist
Animation Speedpage to RenderDemo sample for previewBreaking changes
IterationCount.Valuemay no longer be greater thanlong.MaxValue.Obsoletions / Deprecations
NONE
Fixed issues
Fixes #19979