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

Fixes transitioning API delay when delayMs is not set #335

Merged
merged 3 commits into from
Nov 12, 2019

Conversation

tomasgcs
Copy link
Contributor

@tomasgcs tomasgcs commented Jul 8, 2019

After implamenting "Slow Transitions" feature an issue was introduced with Transitioning API as you can see here tomasgcs@4ef8b81#diff-a7ea5763fc80cde0b741dca6a5095a18R46

Previously beginTime variable was not set at all which was equal to 0, but after this specific line was added the value is different, and it causes animations to delay even if delayMs value is not set, this happens only when animation is is being executed multiple times.

Not sure why this is happening but most likely that CACurrentMediaTime() introduces delay even if it takes current time. In my case it is very important to run animations as fast as possible so I've sumbitted a fix here.

@tomasgcs
Copy link
Contributor Author

@kmagiera Hello, could you take a look at this PR?

@@ -42,8 +42,10 @@ + (REATransitionAnimation *)transitionWithAnimation:(CAAnimation *)animation

- (void)play
{
if (_delay > 0){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with merging this change in. However, I'd like to have a comment here explaining why this check has been introduced (basically what you explain in the PR description).

I also wonder what is the delay in case we use CACurrentMediaTime(). Is this like 1 frame delay (16ms) or more than that? Also if you set delay to be 60 ms is it going to start after 60ms or after 60ms + 1 frame

@osdnk
Copy link
Contributor

osdnk commented Oct 21, 2019

ping @tomasgcs

@tomasgcs
Copy link
Contributor Author

tomasgcs commented Nov 6, 2019

Apologies for delays, added a comment for the check. Also found an article about CACurrentMediaTime:
https://bendodson.com/weblog/2013/01/29/ca-current-media-time/

as Apple in apple docs

A CFTimeInterval derived by calling mach_absolute_time() and converting the result to seconds.

So I am thinking that it might be rounding issue or something like that.

@tomasgcs
Copy link
Contributor Author

@osdnk @kmagiera

Check my latest commit an new issue was introduced with this commit

50e695f#diff-7ab6d44707ac6cd944a490906c842dbb

backwards compatable animations stopped working.

@osdnk
Copy link
Contributor

osdnk commented Nov 11, 2019

Looks good! Please ping whenever you'll find it mergeable

@tomasgcs
Copy link
Contributor Author

@osdnk

Animations are fixed here:
4d98f4f

And I already added comments for _delay animations issue here:
24401b4

Apologies for not creating a separate PR for backward compatible animations fix. But I can do that if necessary.

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.

None yet

3 participants