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

Juggler: Fixed pooled tweens not properly returned in pool when purging the juggler #1101

Closed
wants to merge 1 commit into from

Conversation

Adolio
Copy link
Contributor

@Adolio Adolio commented Sep 11, 2022

Hey Daniel,

Here is small fix regarding pooled tweens & juggler purging. Since the purge method was removing the objects without passing through an event, the pooled tweens weren't able to be released back to the pool.

Here I first try to remove the objects through events and then remove the remaining objects directly. This give a chance to the tween to get recycled.

I have also changed an event handler's name for better clarity.

Thanks in advance for your review & test.

Aurélien


Proposal for another PR

I would suggest to create another event Event.REMOVED_FROM_JUGGLER to indicate when an object is actually removed from the juggler (following the later refactoring, it's fairly simple to achieve). This would maybe simplify the situation. The pooled tweens could listen to this event instead. Here listening to Event.REMOVE_FROM_JUGGLER is a sort of override of a request for removal which is not super intuitive in my sense. This would also give a bit more flexibility for the users as well.

@PrimaryFeather
Copy link
Contributor

PrimaryFeather commented Sep 12, 2022

Hey Aurélien,
you're right, the purge method, in its original form, breaks pooling. That's a problem I hadn't noticed before!
The same is true for the remove... methods.

However, I'm not particularly happy about the additional dispatch of REMOVE_FROM_JUGGLER. While it is — as you correctly noted — a request for the object to be removed from the juggler, it's currently only ever dispatched when the tween / delayed call is complete. Which means that people could rely on that behavior, and we might break code if we change that.

However, your additional suggestion would solve that problem nicely, and there wouldn't be a potential for breaking existing code.

Would you like to give that a try?

Thanks a lot for those suggestions, btw!! 👍

@Adolio
Copy link
Contributor Author

Adolio commented Sep 14, 2022

I was hopping for this answer 😁!

Here it is: #1103

@Adolio Adolio closed this Sep 21, 2022
@Adolio Adolio deleted the juggler-purge-fix branch January 3, 2023 13:38
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.

None yet

2 participants