Tweenable does not seem to be reusable

tfsJoe commented 2 years ago

In the documentation here, there is an example of configuring a Tweenable, and calling .tween(). There is a comment that says:

// tweenable.tween() could be called again later tweenable.tween().then(() => console.log('All done!'))

So, I attempted to reuse a Tweenable, by calling .tween() again. However, the result was, for the duration set in the config, the final value is always used in the render callback. Here is an example - the green div called thing uses a new tween every time, and the red one called thing2 attempts to reuse a tweenable. Based on the comment in the doc, I would expect the red box to go back to y = 0 and animate through y = 500. Instead, after animating from 0 to 500 the first time, the second time it animates from 500 to 500 and just stays put.

<!DOCTYPE html>
<HTML>
    <HEAD>
        <TITLE>Shifty anim lib test</TITLE>
        <SCRIPT type="text/javascript" src="shifty.core.js"></SCRIPT>
        <LINK rel="icon" type="image/png" href="/ui_img/trickyfasticon.png" />
        <STYLE>
#thing {
    position: absolute;
    width: 50px;
    height: 50px;
    background-color: green;
}

#thing2 {
    position: absolute;
    width: 50px;
    height: 50px;
    left: 100px;
    background-color: tomato;
}
        </STYLE>
    </HEAD>
    <BODY class="tight">

        <DIV id="thing" onclick="clickHandler(this).then(()=>console.log('whew'));"> :-) </DIV>

        <DIV id="thing2" onclick="myTweenable.tween();"> :-( </DIV>

        <SCRIPT>
thing = document.getElementById('thing');
thing2 = document.getElementById('thing2');

tween = shifty.tween;
async function clickHandler(element) {
    console.log("ouch!");
    let bounds = element.getBoundingClientRect();
    let myAnim = tween({
        render: ({x}) => {element.style.left = x + 'px'},
        from: {x: bounds.left},
        to: {x: (Math.random() * 600),
        duration: 500,
        easing: 'easeInQuad'}
    });
    await myAnim;
}

myTweenable = new shifty.Tweenable();
myTweenable.setConfig({
    from: {y: 0},
    to: {y: 500},
    duration: 500,
    easing: "easeInQuad",
    render: (state => {
        thing2.style.top = state.y + "px";
        console.log(state.y);
    })
});
        </SCRIPT>
    </BODY>
</HTML>

I've attached a screenshot of the console, with a pink line drawn through the first and second runs:

jeremyckahn commented 2 years ago

Hi @tfsJoe, this is an excellent bug report! Thanks for opening it. I just tried to reproduce this issue in CodePen and am seeing the same behavior: https://codepen.io/jeremyckahn/pen/zYdrNZz?editors=1111

tween doesn't restart a tween from the beginning, although I can see why that would be assumed. Here's the source for that function: https://github.com/jeremyckahn/shifty/blob/520ba431598d3c8e662b9347804b95ddca482134/src/tweenable.js#L334-L352

All it really does it perform some housekeeping around timing and then begin playing with its current config state. In your sample code, setConfig is called once at the beginning, so the initial tween works correctly. However, subsequent calls to myTweenable.tween() don't provide a new config object, so the tween simply uses whatever internal config state it has.

I think you can achieve the behavior you want by changing:

myTweenable.tween();

To something like:

// Assumes newStartingPosition returns updated starting position data
myTweenable.tween({ from: newStartingPosition() });

Shifty's current behavior is perhaps not ideal; it should probably restart completed tweens when a config object is not provided. I can keep this issue open to make that change, but I am pretty tied up with other projects at the moment so I'm not sure when I might be able to work on it (though feel free to make a PR!).

Let me know if this helps!

tfsJoe commented 2 years ago

Thanks for the quick response. I appreciate the workaround, this will work for me. Of course it is up to you as to whether this issue stays open, but if you do decide not to change it I would at least consider this a defect in the documentation - it should probably mention that the configuration needs to be supplied anew.

jeremyckahn commented 2 years ago

@tfsJoe I had a bit of spare time and I think I found a fix for this issue: https://github.com/jeremyckahn/shifty/commit/b3dc299a4bd98094ca1b23186b42894a6f8b5954. It was actually just the removal of some code that I'm not quite sure was for to begin with. :sweat_smile:

I've put up a pre-release version (2.16.0-0): https://unpkg.com/shifty@2.16.0-0/dist/shifty.js

It seems to be working with the sample code you provided: https://codepen.io/jeremyckahn/pen/wvqGWQw Could you give this a look and let me know if it's working the way you expect? If it is, I can release it in 2.16.0.

tfsJoe commented 2 years ago

I pulled it down and used it in my sample, and reviewed the change. With the caveat that I am new to the codebase, this makes sense to me and results in my expected behavior, so it looks good to me!

jeremyckahn commented 2 years ago

Awesome, thanks for checking @tfsJoe! The fix has been released in 2.16.0.

jeremyckahn / shifty

Tweenable does not seem to be reusable #133