Skip to content

Commit

Permalink
Fix failed assertion and memory leak with event listeners (axmolengin…
Browse files Browse the repository at this point in the history
…e#1837)

* Fix failed assertion and memory leak with event listeners

This solves an issue that was fixed for Cocos2d-x v3.16 and then reverted afterwards for "backwards compatibility".

I don't think we need this backwards compatibility anymore, and the benefits to cleaning up this memory outweigh the potential for some developers to need to refactor their code.

Specifically this also solves an ASSERT that fails when quitting the game on a scene which has nodes with Event Listeners that I experienced without this change, and with the Config.h setting AX_NODE_DEBUG_VERIFY_EVENT_LISTENERS enabled.

* Fixed typo
  • Loading branch information
TyelorD authored Apr 19, 2024
1 parent 547b5c0 commit d9a95c6
Showing 1 changed file with 8 additions and 9 deletions.
17 changes: 8 additions & 9 deletions core/2d/Node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -224,15 +224,14 @@ void Node::cleanup()
// timers
this->unscheduleAllCallbacks();

// NOTE: Although it was correct that removing event listeners associated with current node in Node::cleanup.
// But it broke the compatibility to the versions before v3.16 .
// User code may call `node->removeFromParent(true)` which will trigger node's cleanup method, when the node
// is added to scene again, event listeners like EventListenerTouchOneByOne will be lost.
// In fact, user's code should use `node->removeFromParent(false)` in order not to do a cleanup and just remove node
// from its parent. For more discussion about why we revert this change is at
// https://github.com/cocos2d/cocos2d-x/issues/18104. We need to consider more before we want to correct the old and
// wrong logic code. For now, compatiblity is the most important for our users.
// _eventDispatcher->removeEventListenersForTarget(this);
// This will stop Axmol from leaking event listeners on any objects that create them:
//
// Note: If you're moving a Node from one parent to another then you must remember to always call either removeChild(),
// or removeFromParentAndCleanup() with a cleanup bool parameter of false. Otherwise Nodes with listeners (e.g. buttons)
// will stop working when it's removed from it's parent and then added as a child to any Node.
//
// For more details read: https://discuss.cocos2d-x.org/t/note-compatibility-issue-of-node-cleanup-in-cocos2d-x-v3-16
_eventDispatcher->removeEventListenersForTarget(this);

for (const auto& child : _children)
child->cleanup();
Expand Down

0 comments on commit d9a95c6

Please sign in to comment.