[NOTE] Compatibility issue of Node::cleanup in Cocos2d-x v3.16

cocos2d-x
#1

Hello Developers,

I’m James, one of the core developers in Cocos2D-X team. I work for Cocos about 6 years now.

In v3.16, I corrected a wrong logic code in Node::cleanup. I was thinking about that Node::cleanup removes Actions, Schedulers, why wouldn’t it remove EventListeners?
Look at this code in Node::pause and Node::resume:

void Node::resume()
{
    _scheduler->resumeTarget(this);
    _actionManager->resumeTarget(this);
    _eventDispatcher->resumeEventListenersForTarget(this);
}

void Node::pause()
{
    _scheduler->pauseTarget(this);
    _actionManager->pauseTarget(this);
    _eventDispatcher->pauseEventListenersForTarget(this);

So

void Node::cleanup()
{
    ...
    ...
    // actions
    this->stopAllActions();
    // timers
    this->unscheduleAllCallbacks();

    // ??? 
    // Event listeners should not be cleaned up / removed???
    
    for( const auto &child: _children)
        child->cleanup();
}

In v3.16, I thought that it should be fixed by invoking _eventDispatcher->removeEventListenersForTarget(this); in Node::cleanup.

If a node is cleaned up, all its components attached to it should be removed.
And why don’t we remove the associated event listeners?

I didn’t consider the wrong use case of user code:

...
node->retain();
node->removeFromParent(); // or node->removeFromParentAndCleanup(true);
anotherNode->addChild(node);
node->release();

In fact, if developer doesn’t want to to cleanup node, developer should invoke node->removeFromParentAndCleanup(false).
I didn’t consider there are some developers write this wrong code. There even are some code in TableView which uses removeFromParent() in our engine.

Sorry of that I broke the compatibility of Node::cleanup behavior in v3.16.

If you’re using v3.16, writing code like above, and find your node can’t receive touch or other events, you may need to check your code and use removeFromParentAndCleanup(false); instead of removeFromParent().

Or revert that line in Node::cleanup.

I have sent a PR to revert the code :

The issue report is :

The reverted code may be in v3.17 which depends on what you’re thinking about this.

Any advice and thought will be appreciated.

Best Wishes

  • James
1 Like
#2

James,
Thank you for info.

I have few comments:

  1. There is no removeFromParent(true); there is removeFromParentAndCleanup(bool)
    from CCNode.h
/**
 * Removes this node itself from its parent node with a cleanup.
 * If the node orphan, then nothing happens.
 * @see `removeFromParentAndCleanup(bool)`
 */
virtual void removeFromParent();

  1. There is no point to have in public interface removeFromParent(); and removeFromParentAndCleanup(bool). Where behavior of removeFromParent(); == removeFromParentAndCleanup(true).

  2. Function with bool parameter are consider as “code smell”. I think that this is bad practice for public interface
    https://martinfowler.com/bliki/FlagArgument.html
    Better would be to have two functions e.g: removeFromParent() and removeFromParentWithCleanup()

  3. I think that current behavior is better. I found this issue before releasing my game and fixed it.

  4. But unfortunately there were other breaking changes (issues were reported by my customers …)
    https://github.com/cocos2d/cocos2d-x/issues/18465
    https://github.com/cocos2d/cocos2d-x/issues/18384

That fixed one issue:


But without patch in proj.android-studio\app\src\org\cocos2dx\cpp\AppActivity.java creates x10 crashes in my app

protected void onCreate(Bundle savedInstanceState) {
super.setEnableVirtualButton(false);
super.onCreate(savedInstanceState);
// Workaround in https://stackoverflow.com/questions/16283079/re-launch-of-activity-on-home-button-but-only-the-first-time/16447508
if (!isTaskRoot()) {
// Android launched another instance of the root activity into an existing task
// so just quietly finish and go away, dropping the user back into the activity
// at the top of the stack (ie: the last state of this task)
// Don’t need to finish it again since it’s finished in super.onCreate .
return;
}

Can we do something with that?

  1. Maybe if it is possible we can implement breaking change in that way that it will break compilation (hopefully we have C++ not Python/JavaScript :slight_smile: )? Rename function, remove function, change parameter type etc instead changing runtime behavior?

Regards,
Chp

#3

It’s corrected in my thread.

#4

tl;dr for anyone else, if you’ve upgraded to Cocos2d-x v3.16 and you find that your ui::Buttons aren’t clickable anymore, it was a deliberate fix that accidentally broke a (common but incorrect) usecase for it. Seems like commenting out the event removal in Node::cleanup works (at least in mine so far).

Personally I think that there should be some alternate path that correctly removes the listeners, since that what should happen, but I don’t know of a good way to keep older code around without breaking it. My usecase is that I take a scene from Creator/Studio and remove it from the created scene and add it to my existing scene.

It does seem like your solution of passing cleanup false around solves that, but it would be nice to have a way to reactivate the event listeners on addChild then right? Just spitballing though.

I do want to say that I understand how much of a pain this is for you guys, so thanks for trying to open this up to everyone. Love cocos and I appreciate that you guys do this.

#5

@dumganhar I didn’t know this issue existed. usually I just do a

ShopButton::~ShopButton()
{
Director::getInstance()->getEventDispatcher()->removeCustomEventListeners(EVENT_EQUIP);
}

1 Like