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

Add destroy method to Playable #64

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jrouault
Copy link
Member

@jrouault jrouault commented Aug 2, 2017

Description

As pointed out in #63, whenever a tween is stopped, its update and complete can still trigger in the same tick.
This PR adds a destroy method to Playable which immediately removes the tween. As the tween is removed immediately, its update and complete do not trigger in the same tick.

poke @ronkorving @cainmartin @bchevalier

@bchevalier
Copy link
Contributor

bchevalier commented Aug 2, 2017

Yo!

The problem with immediately removing a tween is that it might translate into some weird behaviors if the tween's onComplete or onStop callback is called while some code is looping over the current active tweens.

Also, instead of duplicating the stop method it might be best to wrap the code into a third method? there are other solutions to avoid the issue but I think logic should not be duplicated.

@jrouault
Copy link
Member Author

jrouault commented Aug 2, 2017

Thanks for the quick answer 😃

As for the onComplete callback, it's the whole point of the destroy method to ensure it will not be called. I don't see which case could result in a weird behaviour.
As for the onStop callback, it is still called at the end of the destroy method but I don't see how it would result in a weird behaviour either.
Could you detail that a bit please?

I will make the changes to avoid the duplicated logic.

return true;
if (immediate) {
playable._handle = this._activePlayables.removeByReference(playable._handle);
return true;

Choose a reason for hiding this comment

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

If true in both cases, maybe more elegant to move the return statement to after the else?

@jrouault
Copy link
Member Author

jrouault commented Aug 9, 2017

Hello,

Is there anything I can do to push this PR forward?
@bchevalier Do you have an example so that I could reproduce the bad behaviour that would be introduced by this feature?

@bchevalier
Copy link
Contributor

Unit tests?
That show that removing tweens immediately do not affect other tweens' updates.

@Tatsujinichi
Copy link

Tatsujinichi commented Oct 25, 2017

@bchevalier I assume this is because doubly list is nulling out the next and previous on the removed node, therefore it may stop iterating? Which, coincidentally, is not included on your 'container-doublylist'

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.

4 participants