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: when yoyo repeats, first frame behaves as if yoyo=false #678

Closed
wants to merge 3 commits into from

Conversation

humodz
Copy link
Contributor

@humodz humodz commented Feb 12, 2024

fix for #677

summary:

The current logic first updates the properties, then run the logic that reverses the start and end values for yoyos. The problem is that if the tween has just started repeating, the update will happen before the reversing, so for a single frame the value will be wrong, as if yoyo=false

The proposed fix is to check if the tween has just started to repeat and, if so, apply the yoyo logic before updating the properties

Also, something that I noticed, but I'm not sure if it's intended or not: when the tween repeats, onEveryStart seem to trigger one update later than I would expect. For example:

new TWEEN.Tween(obj)
    .to({ x: 100 }, 100)
    .repeat(1)
    .yoyo(true)
    .start(0);

TWEEN.update(99); // start everystart update
TWEEN.update(120); // update repeat
TWEEN.update(150); // everystart update
TWEEN.update(199); // update
TWEEN.update(201); // update complete

shouldn't everystart happen at 120ms, since the tween has already repeated at that instant?

@alphabetabc
Copy link

I found that this code can be fixed by restoring it to this state
image

@humodz
Copy link
Contributor Author

humodz commented Apr 19, 2024

I found that this code can be fixed by restoring it to this state image

This has two issues:

If the tween has a start delay, and .update is called with a duration larger than the delay + the tween's duration, elapsed will end up negative

// in the following example, during the delay between repetitons, obj2.x will be negative
import TWEEN from './dist/tween.esm.js';

const obj1 = { x: 0 }
const obj2 = { x: 0 }

const f = (obj) => new TWEEN.Tween(obj)
    .to({ x: 100 }, 100)
    .delay(50)
    .repeat(99)
    .start(0);

const tween1 = f(obj1);

for (let t = 0; t < 500; t += 10) {
    tween1.update(t);

    obj2.x = 0;
    const tween2 = f(obj2);
    tween2.update(t);

    console.log({ t, obj1, obj2 });
}

The second issue is that the value will be incorrect on the first frame after the yoyo loops, as if it gets stuck at the end of the loop for a single frame:

import TWEEN from './dist/tween.esm.js';

const obj1 = { x: 0 }

const tween1 = new TWEEN.Tween(obj1)
    .to({ x: 100 }, 100)
    .repeat(30)
    .yoyo(true)
    .start(0);

// at instant t === 160
// with t += 80 -> obj.x === 100 (wrong)
// with t += 20 -> obj.x === 40  
for (let t = 0; t <= 500; t += 80) {
    tween1.update(t);

    console.log({ t, obj1 });
}

@trusktr
Copy link
Member

trusktr commented May 5, 2024

@humodz thank you for this excellent and well-tested work! For some reason the tests are not running, so I move your code into a local branch and make a new PR with the same commits:

#683

Will close this one in faavor of that one.

@trusktr trusktr closed this May 5, 2024
@trusktr
Copy link
Member

trusktr commented Jul 26, 2024

@humodz for some reason this broke the yoyo example:

so I had to revert the change (which I had originally merged in #683).

Although all tests were passing, something was different with the yoyo example that we will need to account for in the unit tests.

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.

3 participants