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

onComplete does not fire if timeScale is set on the last frame. #551

Closed
Jeaper opened this issue Jul 26, 2023 · 3 comments
Closed

onComplete does not fire if timeScale is set on the last frame. #551

Jeaper opened this issue Jul 26, 2023 · 3 comments

Comments

@Jeaper
Copy link

Jeaper commented Jul 26, 2023

If timeScale is set during the same loop a tween is about to complete
It will Cause it to never call its onComplete event.

I think this only happens if the timescale is called during an event on another tween, but I have not been able to say that for sure, or if on the same RAF is enough.

Recreation:
https://codepen.io/Jesper-Leveau/pen/gOQBpdK

Found during an upgrade of existing project from 1.18.5 to 3.12.2

@jackdoyle
Copy link
Member

Thanks for the minimal demo, @Jeaper

This is because you're altering the timeScale such that the completion would be in the PAST. And when adjusting the startTime position in the parent via a timeScale() change, it suppresses events. I'll look into whether or not there's a clean way to accommodate this edge case and NOT suppress events, perhaps with a parameter or something.

@Jeaper
Copy link
Author

Jeaper commented Aug 29, 2023

Suggestion: @jackdoyle
Why does timeScale affect startTime and progress at all?
Should it not just affect how much dt affects the progress in future updates?

@jackdoyle
Copy link
Member

When you alter the timeScale(), it MUST alter the startTime if progress isn't 0, otherwise it'd get out of sync and the playhead would jump. Remember that a child animation's playhead position is controlled by its parent timeline's playhead (so that everything is perfectly in sync).

For simplicity's sake, let's say you've got a 1-second tween and its startTime is 0 (so it's at the very start of its parent timeline) and then when that tween is at 0.5 (halfway done), you alter the timeScale to be 2 which means the effective duration of that tween becomes 0.5. If we don't update the startTime, then the tween would immediately be at its very end because the parent's playhead is at 0.5 which is where that tween ends if its timeScale is 2 (if it starts at 0 and the native duration is 1 but its timeScale is 2, that means it ends at 0.5).

So the correct thing to do is alter the startTime to be 0.25 so that the overall progress remains the same (the tween would now be positioned from 0.25 -> 0.75 on its parent timeline).

Changing the timeScale does not actually affect progress. The reason you saw that in your demo is because of an order-of-execution thing. The first tween updated first...and called its onComplete, and the second tween would have updated next but your first tween read back the 2nd tween's progress BEFORE it had a chance to update. When you altered its timeScale(), you forced an update.

Make sense?

I think in the next release, I could allow a suppressEvents parameter on timeScale() just like we do for things like seek(), time(), totalTime(), etc. so that you can tell it specifically that you want callbacks to fire in that case.

jackdoyle added a commit that referenced this issue Nov 30, 2023
- IMPROVED: gsap.context() and gsap.matchMedia() functions will get a 2nd argument that we'll call "contextSafe" which is like a wrapper for any function that you'd like to keep in the context. So any GSAP animations/ScrollTriggers/Draggables/Observers that are created during the execution of that function will be added to the context and the selector text will be scoped.

- IMPROVED: added a suppressEvents parameter to Tween/Timeline/Animation .timeScale() method so that you can optionally prevent it from suppressing events when altering the timeScale. See #551

- IMPROVED added an ignoreSpeed [3rd] parameter to ScrollSmoother's offset() method to allow you to specify whether you want to get the value that corresponds to the window's scroll position or the ScrollSmoother's scrollTop value. See https://greensock.com/forums/topic/35108-problems-with-the-scrollsmoother-plug-in-speed-option-in-gsap-3114/

- IMPROVED: Observer now fires an onStop with an onMove (previously it would only fire after a press). See https://greensock.com/forums/topic/38469-observer-misunderstanding-with-onchange-onmove-and-onstop/

- IMPROVED: slight change to TypeScript definitions for EaseString allows arbitrary strings while also activating code hinting for the common ones in more environments. See #556

- IMPROVED: animations/ScrollTriggers created inside a ScrollTrigger's callback like onEnter/onLeave/onToggle/onEnterBack/onLeaveBack will be added to the original Context (if one existed), meaning selector text will be scoped properly. See https://gsap.com/community/forums/topic/38850-reactgsap-why-i-cant-use-class-selector-in-scrolltrigger-inside-gsapcontext/

- IMPROVED: a ScrollTrigger's snap end position is limited to the resolution of the browser's scroll (whole pixels only), thus if you have a scrubbed animation that's supposed to snap to a very specific spot on that animation, it may end slightly off of that but now a correction runs at the end of the snap to ensure that it gets set PRECISELY to that snapped position. See https://gsap.com/community/forums/topic/38937-scrolltrigger-timeline-snapping-with-label-doesnt-snap-precisely-to-label-position/

- IMPROVED: the gsap-trial files will now work on domains that end in ".local" (for testing only please)

- FIXED: for an Observer, if you pressed, started dragging but released within 3 pixels of the original press, it wouldn't fire the onDragEnd. See https://greensock.com/forums/topic/37510-the-problem-that-ondragend-is-not-called-in-observer/

- FIXED: if you revert() a context/matchMedia that has a reversed animation, it may not render things in the proper order, potentially leaving inline styles when it shouldn't. See https://greensock.com/forums/topic/37432-issues-with-horizontalloop-helper/

- FIXED: if Draggable is applied to an <object> where the document isn't defined initially, an error could be thrown. See #549

- FIXED: if you apply a speed to a ScrollSmoother (other than 1), the offset() method would return a value that corresponds to the window's scroll position rather than the ScrollSmoother's scrollTop (which is affected by speed). See https://greensock.com/forums/topic/35108-problems-with-the-scrollsmoother-plug-in-speed-option-in-gsap-3114/

- FIXED: if you apply an onUpdate to a .fromTo() tween, the scope ("this") would be linked to the zero-duration "from" tween instead of the actual tween instance you'd expect, thus its progress() would be 1 instead of 0 at the very start.

- FIXED: if you define position: absolute on a SplitText and then revert(), the width/height inline styles could remain instead of getting cleared out. See https://greensock.com/forums/topic/38391-responsive-behavior-using-splittext/

- FIXED: if you create a ScrollTrigger with a "snap" and pin inside a gsap.matchMedia(), it could lead to incorrect positioning of the pinned element after a resize that makes it no longer match.

- FIXED: if you set a CSS value to "auto" (like height or width) via a GSAP tween and then you revert() it, the original computed value could be left as an inline style instead of cleared out.

- FIXED: if you call kill() or revert() on a MotionPathHelper in certain specific conditions, it may delete the original <path> itself.

- FIXED: worked around a very rare scenario where document.createElement() returns undefined and would consequently throw an error in CSSPlugin. See #553

- FIXED: if you create a staggered animation inside a gsap.context() or gsap.matchMedia() that affects the same element(s) as a gsap.set() that happened before that staggered animation and then the context/matchMedia gets reverted, the initial value may not get reverted properly.

- FIXED: if you animate a percentage-based width/height of an element whose parent has padding and/or is flexbox/grid, and the target element doesn't have that property already set inline, it may miscalculate the starting value of the tween. Related to https://gsap.com/community/forums/topic/38599-how-to-animate-object-fit/

- FIXED: if you create multiple SplitText instances on the same element inside a gsap.context() and then revert() that Context, it may not fully revert the element to its original state. See https://gsap.com/community/forums/topic/38734-splittext-innerwrap-renders-twice-in-strictmode/

- FIXED: if you set allowNestedScroll: true in the ScrollTrigger.normalizeScroll() feature, touch-scrolling on a link on a mobile device could result in a click event firing on that link. See https://gsap.com/community/forums/topic/38770-why-does-the-burger-menu-scroll-along-with-the-content-on-mobile/#comment-193009

- FIXED: if you set end: "max" or clamp() the end of a ScrollTrigger that has a pin, and the pinSpacing was extending the page taller, the dynamic adjustment of the end value would also affect the pinSpacing, reducing the maximum scroll area which then wouldn't be accurately reflected in the final end value.

- FIXED: if you define a stagger with grid: "auto" on an Array of elements that don't wrap at all (not really a grid), the last element's timing wouldn't be correct. See https://gsap.com/community/forums/topic/38536-when-scrolling-down-a-batch-the-stagger-doesnt-seem-to-work-correctly/

- FIXED: if you clamp() a ScrollTrigger's start value and it would naturally (without clamping) resolve to beyond the maximum scroll position, it wouldn't get clamped. Starting values were only clamped such that they weren't allowed to be negative (focused on the top of the page only, not the bottom too).

- FIXED: if you dynamically added/created a ScrollTrigger while ScrollSmoother was mid-scrub, it could lead to the scroll jumping. See https://gsap.com/community/forums/topic/37515-dynamic-scrolltrigger-with-pin-inside-a-scrollsmoother/

- FIXED: if you run SplitText on text that has words separated by non-breaking spaces (&nbsp;), it wouldn't recognize that as a word delimiter. See https://gsap.com/community/forums/topic/37271-why-does-the-splitted-node-from-splittext-includes-extra-whitespace-note-only-the-node-which-is-next-tag/

- FIXED: Flip.fit() is now gsap.context()-aware so that it'll revert inline styles when the context is reverted.

- FIXED: regression in 3.11 that could cause transformOrigin to lose its "z" portion if you apply it in a .from() or .fromTo() tween or a context that gets reverted. See https://gsap.com/community/forums/topic/38958-rotatey-not-working-in-nuxt/

- FIXED: if you call gsap.registerPlugin(Observer) multiple times, it could throw an error.
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

No branches or pull requests

2 participants