Problem with cancelTween()

letmaik commented 9 years ago

I think I found a bug in the Tween component. I'm using the latest nightly.

What happens is:

I start a tween of say 1 sec.
I cancel the tween at 0.5sec.
I start another tween at 0.7sec.
The first tween isn't fully cancelled! (e.g. still in .tweens array)

It seems the full cancellation happens only after the time the tween would have ended originally. This effect however screws up my game logic since I use tween loops and rely on correct order of these things.

Unfortunately I don't have a small self-contained example, but maybe it is easy to find the bug without it.

starwed commented 9 years ago

The first tween isn't fully cancelled! (e.g. still in .tweens array)

I think the way it's written, you'd expect that there would still be an object in the tweens array, but the props object on it would have no properties remaining.

This effect however screws up my game logic since I use tween loops and rely on correct order of these things.

If the cancelled tween properties are still progressing, that would definitely be a bug. Is that what you're seeing?

letmaik commented 8 years ago

Sorry I'm currently a bit busy, I'll try to come up with an example in 1-2 weeks.

airza commented 8 years ago

Let me see if I can reproduce.

airza commented 8 years ago

square = Crafty.e("Color,2D,DOM,Tween").color("Black").attr({
    x:10,
    y:10,
    h:50,
    w:55
}).tween({
    x:100,
    y:100,
    h:100,
    w:100,
},2000);
window.setTimeout(()=>{
    square.cancelTween({x:true});
debugger;
//should just have the y,h,w tween group
    square.tween({
    x:1000,
    y:0
    },500);
    debugger;
//x/y should be in one group and w/h still in the original group..
},1000);

The behavior after cancel and after the new tween looks correct in both cases. Notably something insane happens if the value of duration isn't specified, but I don't think that's what's being referenced here.

What part of the tween internals are you relying on? That area of code has been wonky for a while and I don't know if counting on its internal API is a good idea.

letmaik commented 8 years ago

My problem is when using the TweenEnd event:

<!doctype html>
<html>
<body>

<div id="game"></div>

<script src="https://rawgit.com/craftyjs/Crafty-Distro/nightlies/crafty.js"></script>
<script>
Crafty.init(500,500, document.getElementById('game'));

var t0 = new Date()
square = Crafty.e("Color,2D,DOM,Tween").color("Black").attr({
    x:10,
    y:10,
    h:50,
    w:55
}).bind('TweenEnd', function() {
    console.log("end at " + (new Date()-t0))
}).tween({
    x:100,
    y:100,
    h:100,
    w:100,
},2000);
window.setTimeout(()=>{
    square.cancelTween({x:true});

//should just have the y,h,w tween group
    square.tween({
    x:1000,
    y:0
    },500);

//x/y should be in one group and w/h still in the original group..
},1000);
</script>

</body>
</html>

The console output is:

end at 1496
end at 1995

Whereas it should be (ignoring millisecond accuracy):

end at 1000
end at 1500

The TweenEnd of the first tween only gets fired after its original duration passes, not as soon as it is cancelled. I don't think this makes sense, and this is the actual problem which messes up my logic.

airza commented 8 years ago

The first tween isn't cancelled. Only its x component is cancelled. When you bind the x,y after that it makes it so there are two tweens: the original one which is still tweening h and w and the new one which is tweening x and y.

Additionally it seems personally more intuitive to me that cancelled tweens don't fire TweenEnd because they don't actually complete. Both ways sort of have their caveats if multiple competing components are trying to tween an item, though.

letmaik commented 8 years ago

Ok but even if I cancel all components there's no change in behaviour.

I think the best way would be that there are two types of events: TweenEnd and TweenCancel. Only one of both is ever fired for a tween.

My problem is: I listen to TweenEnd, and then launch a new tween. A loop, and I have a function that can stop and start the loop. But that gets screwed up when TweenEnd is not fired when the tween is cancelled. Currently it's just impossible for me to handle such cases.

200sc commented 8 years ago

Putting aside whether it is appropriate for the future of the engine to associate an event with calling tweenCancel explicitly, for your specific problem, is there a reason you can't call your own "TweenCancel" event after performing the cancel?

As far as what events need to be defined by the engine and not by the user, an event where the user always knows when it should occur (right after they call cancelTween, and never anywhere else) seems more bloaty than useful. Compare this to the end of a tween, as we define it now, which would require a delay call to explicitly call that event on the user side when appropriate.

airza commented 8 years ago

Ah yeah, I see - it has an orphaned tween in the tweens array that doesn't attach to any properties but still fires TweenEnd.

The best way to fix this would be to fix the weird way that it stores references to tweens- the fastest way would be to use this instead for the tween's _endTween function. Note that i tested this on my own game for about 30 seconds, but it seems to make sense - it just explicitly prevents property-less tweens from firing the TweenEnd event.

  _endTween: function(properties){
    for (var propname in properties){
      delete this.tweenGroup[propname];
    }
    if (Object.keys(properties).length){
        this.trigger("TweenEnd", properties);
    }
  }

letmaik commented 8 years ago

@Sythe2o0 I could manually fire TweenCancel, but then TweenEnd would still be called and I don't see an easy way to handle that in my game.

@airza The solution you propose makes sense to me. I tested your quick fix since I have a component for my tween sequence anyway. It seems to override the built-in method properly and now it works. Thanks for that!

starwed commented 8 years ago

It looks to me like this is a bug that airza's code would fix, so tagging it as such.

craftyjs / Crafty

Problem with cancelTween() #966