Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix animation reversing #620

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ChrisFloofyKitsune
Copy link
Contributor

fix animation reversing by removing undocumented and buggy "reverse_adjustment_factor"

@mikke89
Copy link
Owner

mikke89 commented Jun 9, 2024

Hi, and thanks for the PR!

Can you elaborate a bit more. What is the buggy behavior you mention? How does this PR change the current behavior?

Looks like this factor was introduced all the way back in 23bdc17, it's a while ago, but looks like it's related to implementation of the CSS spec reverse shortening factor.

@ChrisFloofyKitsune
Copy link
Contributor Author

Before:

Reverse animation duration is not shortened properly.
Calculations are done, but thrown away due to the incorrect property.
GIF 6-9-2024 8-39-10 AM

After:

The reverse transition's duration is properly shortened based on how far in the animation the previous transition was.
A defined property was being introduced when there was no need of one.

GIF 6-9-2024 8-47-38 AM

Re-reading the spec, I'm certain the reverse shortening factor related stuff was not implemented correctly.

https://www.w3.org/TR/css-transitions-1/#ref-for-running-transition%E2%91%A5

I can't quite parse all the jargon in there, but the "After" animations definitely look better to my eyes.

Seems that there should also be a check that makes sure that the new transition's ending state matches the old transition's starting state.

@mikke89
Copy link
Owner

mikke89 commented Jun 9, 2024

Right, thanks for elaborating on the motivation and the screenshots.

Yeah, I see this change here is essentially equivalent to setting the factor to 1.0, while before the default was set to 0.0. I don't remember why I left it like that in the end, could be I just never got back to it, but I'm a bit worried there was some issue with it.

One issue seems to be that we don't keep track of the factor, thus leading the duration to be compressed for each successive reversal. This actually seems to be a visible issue in practice. E.g. try a hover transition to around 50% completion, then quickly move in and out of it, then let it complete. It looks quite buggy. I see that the spec tries to account for this, I think we should do the same.

Another thing, I notice the factor can go outside the [0, 1] range (e.g. with exponential-out interpolation). Just like in the spec, we should clamp it to the [0, 1] range. This should be a quick change though.

@ChrisFloofyKitsune
Copy link
Contributor Author

The spec is making sense the more I let my brain sit on it and all those changes sound like good ideas.

I'm already planning out how to add all that stuff in.

@ChrisFloofyKitsune ChrisFloofyKitsune marked this pull request as draft June 13, 2024 04:06
@ChrisFloofyKitsune ChrisFloofyKitsune marked this pull request as ready for review June 19, 2024 07:30
@mikke89
Copy link
Owner

mikke89 commented Jun 23, 2024

First of all, thanks a lot! This looks like a great effort, and it's nice if we can follow the standard a bit better in some of these corner cases.

Can you tell a bit more about the observable changes we get from this?

We are currently in a stabilization phase towards RmlUi 6.0. There are quite a few changes here, and some issues can easily creep in from experience. So we will have to merge this one after release, I'll see if I can find some time to review it in the meantime.

@ChrisFloofyKitsune
Copy link
Contributor Author

Sorry for the late response.

Can you tell a bit more about the observable changes we get from this?

The main thing is that animation reversing (and reversing the reversing repeatedly, and reversing that..., etc) does not result in strange behaviours / visual glitches.

In general, I've just been trying to follow the CSS spec as much as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants