Remove Node::removeFromParentAndCleanup and Refactor Node::removeFromParent

[TyelorD]

Describe your changes

This change removes the Node::removeFromParentAndCleanup function, and instead gives Node::removeFromParent and optional bool for if the node should be cleaned up or not.

This defaults to true, since the original intention was that it would cleanup the memory by default, and only not cleanup the memory when told to do so by the programmer with the old Node::removeFromParentAndCleanup function.

I've also added missing licenses for the Inspector.cpp/.h files, and the other C++ and header files I had to modify that were using the old function.

Now that I'm thinking about it, this will probably cause some tests to fail to be compiled. This PR is likely going to need some additional work, but I don't have time this week to work on this issue more. If anyone else wants to take it upon themselves to fix any broken tests, then feel free! Otherwise, I might have time next week to fix any broken tests caused by this refactor.

Issue ticket number and link

#1837 Specifically the suggestion that @smilediver had, well mostly anyways. I've described why I believe defaulting it to true is better than requiring the programmer to manually add a bool for cleanup on that thread.

Checklist before requesting a review

For each PR

• [x] Add Copyright if it missed:
  - "Copyright (c) 2019-present Axmol Engine contributors (see AUTHORS.md)."

• [x] I have performed a self-review of my code.

  Optional:

  • [ ] I have checked readme and add important infos to this PR.
  • [ ] I have added/adapted some tests too.

For core/new feature PR

• [ ] I have checked readme and add important infos to this PR.
• [ ] I have added thorough tests.

[rh101]

Sorry, I feel like I got ahead of myself with regards to the changes to removeFromParent and removeFromParentAndCleanup, including the changes made in PR #1837.

Going through the code more thoroughly, it seems that Node::~Node() calls the following:

    stopAllActions();
    unscheduleAllCallbacks();
    AX_SAFE_RELEASE_NULL(_actionManager);
    AX_SAFE_RELEASE_NULL(_scheduler);

    _eventDispatcher->removeEventListenersForTarget(this);

#if AX_NODE_DEBUG_VERIFY_EVENT_LISTENERS && _AX_DEBUG > 0
    _eventDispatcher->debugCheckNodeHasNoEventListenersOnDestruction(this);
#endif

So, on destruction, the event listeners are in fact removed correctly.

With removeFromParent, it is a bit of a loaded method; all actions are stopped, callbacks unscheduled, and now, event listeners would be removed.

If a node is being moved to another parent, then removeFromParent would never be used, and instead the developer would have used removeFromParentAndCleanup(false), assuming they require that actions, callbacks and (now) events persist.

If a node is released, then the actions, callbacks, and event listeners are correctly released in the Node::~Node() destructor.

So, what seems to be the typical use-case in existing code:

Node::removeFromParent() is called, and the node is allowed to auto-release, or is manually released, meaning the Node::~Node() is called. This would be doubling up on the calls to removing the actions, callbacks, and event listeners.

Perhaps the correct behaviour is to just have removeFromParent just do what the name implies, and that is to remove the node from the parent, and that is all. Do not clean-up here. If a node is released, then the clean-up would be taken care of automatically in the destructor, so no more doubling up on the calls to remove actions, callbacks and event listeners.

In this case, Node::removeFromParentAndCleanup(bool) would still be required, and it would be doing what the name implies, which is to remove from the parent and clean up (if the bool is true). This is the method that would (or should) currently be used throughout the code for the purpose of re-parenting a node.

So, would this be a reasonable change in behaviour? Simply changing removeFromParent from this:

void Node::removeFromParent()
{
    this->removeFromParentAndCleanup(true);
}

to this:

void Node::removeFromParent()
{
    if (_parent != nullptr)
    {
        _parent->removeChild(this, false); // No clean-up
    }
}

@halx99 @smilediver @TyelorD and anyone else that wants to chime in, what are your opinions on this?

Also, what I'm not clear on is why setting AX_NODE_DEBUG_VERIFY_EVENT_LISTENERS would be causing an assert to trigger, as the events are correctly being released on destruction of a node. @TyelorD Did you ever look into why the assert is triggering for you?

[halx99]

Maybe revert https://github.com/axmolengine/axmol/pull/1837 first. EDIT: a revert pr #1841 was created

[rh101]

Maybe revert #1837 first.

Yes, that would be the best way forward until the best solution can be decided on.

[smilediver]

I had a look through our first game we're porting to Axmol to gather some stats. In our SDK shared between games we have ~20 calls to removeFromParent() and 0 calls to removeFromParentAndCleanup(). In the game we have ~80 calls to removeFromParent(): ~60% for destroying the node, ~30% for reattaching and ~10% needs a deeper investigation. It also has ~30 calls to removeFromParentAndCleanup() with like 5 calling with true and the rest with false.

I think it shows that not all devs understand the implications of calling removeFromParent(). It probably was a mistake to set cleanup value to any default value.

In perfect circumstances I would also vote for removeFromParent() to be a simple detach, but I just realized it's not one method, there's a whole group of these:

    virtual void removeFromParent();
    virtual void removeFromParentAndCleanup(bool cleanup);
    virtual void removeChild(Node* child, bool cleanup = true);
    virtual void removeChildByTag(int tag, bool cleanup = true);
    virtual void removeChildByName(std::string_view name, bool cleanup = true);
    virtual void removeAllChildren();
    virtual void removeAllChildrenWithCleanup(bool cleanup);

So changing just removeFromParent() would break consistency here, and having in mind that this also silently changes the behavior in even bigger way sound like a bad idea now. I'm starting to think that maybe that previous fix of cleaning up events is a lesser of two evils.

But I would probably cleanup this group interface to be a bit more consistent by deprecating/removing cleanup methods and adding cleanup parameter, something like this:

    virtual void removeFromParent(bool cleanup = true); // <-- add cleanup
    virtual void removeFromParentAndCleanup(bool cleanup); // <-- deprecate or remove
    virtual void removeChild(Node* child, bool cleanup = true);
    virtual void removeChildByTag(int tag, bool cleanup = true); // <-- I think this was already deprecated by Cocos
    virtual void removeChildByName(std::string_view name, bool cleanup = true);
    virtual void removeAllChildren(bool cleanup = true); // <-- add cleanup
    virtual void removeAllChildrenWithCleanup(bool cleanup); // <-- deprecate or remove

Just not sure if cleanup parameter should be left with default value or it should be removed, but I would lean towards it being an explicit parameter, so that devs need to think what to pass in there.

[TyelorD]

So changing just removeFromParent() would break consistency here, and having in mind that this also silently changes the behavior in even bigger way sound like a bad idea now. I'm starting to think that maybe that previous fix of cleaning up events is a lesser of two evils.

This was my main reasoning for the change that I had made here: I felt it was the lesser of two evils.

But I would probably cleanup this group interface to be a bit more consistent by deprecating/removing cleanup methods and adding cleanup parameter, something like this:

I agree, I overlooked the fact there were duplicate methods there. All of those should be cleaned up imo, to remove the AndCleanup/WithCleanup variants with the cleanup parameter. Though I still lean towards cleanup being implicitly set to true, then the programmer themselves can manually set it to false when needed. Even your own code shows that it's mostly used to remove a node without reattaching it, so the most commonly used case should be the implicit one. I can see some benefit to having it explicit, but also don't see the point really: aside throwing errors that the programmer needs to fix after upgrading Axmol (which has pros and cons).

Also, what I'm not clear on is why setting AX_NODE_DEBUG_VERIFY_EVENT_LISTENERS would be causing an assert to trigger, as the events are correctly being released on destruction of a node. @TyelorD Did you ever look into why the assert is triggering for you?

We have a LayerColor that acts as the "Scene", based on the older Cocos2d-x paradigm here, and this LayerColor stores all the nodes for the scene, and itself is stored in a Scene object. That LayerColor class, as well as many nodes in that LayerColor, have custom event listeners as well as several standard event listeners. When we exit the game on a desktop build, with that macro enabled, the assertion is hit because the custom events are never cleaned up implicitly by Axmol without the change I made in this PR. With the change these custom event listeners are cleaned up properly and the AXASSERT is never hit.

So my assumption is that this AXASSERT and cleanup check is happening before the node is destroyed when ending the scene. But by forcing it to cleanup when removed from its parent, it doesn't need to wait for its destructor call (which we have little control over when exactly the system will fire the dtor, since afaik it usually is but isn't guaranteed to be called the moment the object is deleted).

Sorry I haven't had time to cleanup this PR this week, I've been really busy with the update I'm working on. I'm hoping to have time to address this PR sometime next week though.

[smilediver]

Though I still lean towards cleanup being implicitly set to true, then the programmer themselves can manually set it to false when needed.

I would agree in an ideal world, but in the real world everyone makes assumptions and quick judgements and people miss important information especially when they assume that defaults are good. My biggest gripe is that removeFromParent() is doing a lot more than the name suggests, and in our codebase ~30% calls to removeFromParent() are probably wrong.

Even your own code shows that it's mostly used to remove a node without reattaching it, so the most commonly used case should be the implicit one.

You probably misread, but including all remove from parent related calls it reattaches the node in half of the cases.

I can see some benefit to having it explicit, but also don't see the point really: aside throwing errors that the programmer needs to fix after upgrading Axmol (which has pros and cons).

But upgrading the project would silently start removing node's listeners, I imagine not a very fun thing to debug. :) That's why I'm inclined to break the code and mention this in the upgrade guide. I hit a similar silent change in RenderTexture and wasted two days trying to understand why it's not rendering. :(

[TyelorD]

You probably misread, but including all remove from parent related calls it reattaches the node in half of the cases.

But removeFromParent already calls removeFromParentAndCleanup(true), so implicitly defaulting it to true is technically the same behavior as there is now. The not cleaning up event listeners is technically a bug, since it should be cleaning up all the data based on its underlying implementation.

and in our codebase ~30% calls to removeFromParent() are probably wrong.

I'm not sure we should implement things solely to help users who were misusing a function/feature though.

But upgrading the project would silently start removing node's listeners, I imagine not a very fun thing to debug. :)

I suppose that would be true for people who were misusing removeFromParent(), rather than removeFromParentAndCleanup(false) (which I will admit was even us before I made this change in our source code in 2022). There really isn't any harm in forcing an explicit parameter for removeFromParent after this cleanup refactor PR, other than potentially requiring a large number of search and replacing to add in a boolean to any removeFromParent calls. But perhaps that is still the lesser of two evils to help anyone 'misusing' removeFromParent, since previously it wouldn't have cleaned up event listeners with that call commented out from the Cocos2d-x days.

[rh101]

One thing to keep in mind is while Axmol was initially based on Cocos2d-x v4, the goal is not to keep 100% backwards compatibility with it. Axmol has diverged from Cocos2d-x quite a lot, so there is no need to put restrictions on fixing things that need to be fixed for the sake of compatibility with Cocos2d-x.

To make it easier for developers to port over from Cocos2d-x to Axmol, then we should ensure that all the differences (ie. breaking changes) between the two engines are documented; that is really the only requirement.

[halx99]

Maybe don't do anything Node::cleanup which already does in Node destructor, remove xxxAndCleanp, don't need add parameter cleanup to removeFromParent

[rh101]

Maybe don't do anything Node::cleanup which already does in Node destructor, remove xxxAndCleanp, don't need add parameter cleanup to removeFromParent

What happens in the scenario where a node is removed from the parent, but not released, for example, just stored in a ax::Vector etc. The developer may require that the actions and callbacks should not be active, and events should be paused.

One option is for the developer to do this:

node->removeFromParent();
node->pause(); // this pauses actions, scheduler and event listeners

That makes the intention clear, which is to remove from the parent, and pause the node.

If the actions and callbacks should be completely removed for the node:

node->removeFromParent();
node->stopAllActions();
node->unscheduleAllCallbacks();

and if event listeners are also no longer required as well:

node->getEventDispatcher()->removeEventListenersForTarget(node);

While I'm personally not against having to call all those methods to do what is required, and can add them in a utility method somewhere, it would be nice to have all of those in a single method within the Node, especially if this is a common use-case for developers. So, the question is, should a single convenience method be added to handle this scenario?

Perhaps something like this:

Node::cleanup(bool actions, bool callbacks, bool eventListeners)
{
   if (actions) 
       stopAllActions();

    if (callbacks)
        unscheduleAllCallbacks();

    if (eventListeners)
        _eventDispatcher->removeEventListenersForTarget(this);
}

So it would be:

node->removeFromParent();
node->cleanup(true, true, true); // etc. etc.

This just reduces the code required, and still relatively clear as to the intention.

[halx99]

pause node automatically in removeFromParent function?

[rh101]

pause node automatically in removeFromParent function?

If that is implemented, then it would need to be resumed when it is attached to a parent, assuming the parent is not paused.

[smilediver]

I like the Node::cleanup(bool actions, bool callbacks, bool eventListeners) addition (regardless of what other changes would be there). But 50% of the calls are for destroying the node and would end up calling cleanup anyway (at least in our case), so I'm tempted to leave the cleanup parameter also. But I would make it an explicit one, so user is forced to make a decision and specify why he is removing the node, and that would avoid wrong usage.

pause node automatically in removeFromParent function?

This might interfere with user's logic calling pause() and resume(). I agree that detached node probably should become inactive automatically, but if that's implemented I hope it's done through other mechanism than just calling pause(). Probably there should be two flags, one paused by user for pause() and resume() calls and another paused by system managed internally by Axmol (e.g. set by removeFromParent()).

[rh101]

But 50% of the calls are for destroying the node and would end up calling cleanup anyway (at least in our case), so I'm tempted to leave the cleanup parameter also.

That's the thing that is a little strange, because @TyelorD mentioned that there were dangling event listeners that had not been cleared on destruction, but if you check Node::~Node(), you'll see this (extra stuff removed for readability):

Node::~Node()
{
...
    stopAllActions();
    unscheduleAllCallbacks();
    AX_SAFE_RELEASE_NULL(_actionManager);
    AX_SAFE_RELEASE_NULL(_scheduler);

    _eventDispatcher->removeEventListenersForTarget(this);

#if AX_NODE_DEBUG_VERIFY_EVENT_LISTENERS && _AX_DEBUG > 0
    _eventDispatcher->debugCheckNodeHasNoEventListenersOnDestruction(this);
#endif
...
}

It is in fact clearing everything, so it is strange that there would be any dangling event listeners when a node is destroyed. The call to cleanup should never be required if the node is being freed.

[smilediver]

It is in fact clearing everything, so it is strange that there would be any dangling event listeners when a node is destroyed.

I agree, that debugCheckNodeHasNoEventListenersOnDestruction() should never trigger any assert. I think there might be another bug and I think original #1836 issue should be investigated independently from this PR.

[TyelorD]

It is in fact clearing everything, so it is strange that there would be any dangling event listeners when a node is destroyed.

The current Scene object (which inherits from Node) isn't going to be destroyed, and thus cleaned up, until the program exits. So any listeners on that Scene won't be cleaned up, thus triggering the AXASSERT.

By making sure that Node::cleanup properly cleans up event listeners, as in #1837, this solves this AXASSERT being hit, since now the event listeners will be cleaned up when the Scene is removed by Axmol upon exiting the program, rather than when the Scene's destructor is called at the program termination and the Scene fully goes out of scope.

[smilediver]

The current Scene object (which inherits from Node) isn't going to be destroyed, and thus cleaned up, until the program exits. So any listeners on that Scene won't be cleaned up, thus triggering the AXASSERT.

Right before the assert check, node's destructor calls the very same function that #1837 PR enables:

https://github.com/axmolengine/axmol/blob/d50b7aad0b928125dccd2dc5459a63b20475c010/core/2d/Node.cpp#L189-L193

So I think either event removal has a bug or the assert check has. Is that scene's destructor is being called in an event listener by any chance? It might be that the listener's removal is delayed during the event due to this:

https://github.com/axmolengine/axmol/blob/d50b7aad0b928125dccd2dc5459a63b20475c010/core/base/EventDispatcher.cpp#L688-L696

[TyelorD]

So I think either event removal has a bug or the assert check has. Is that scene's destructor is being called in an event listener by any chance? It might be that the listener's removal is delayed during the event due to this:

Indeed it is, specifically a touch event on the button to close the game. This must be the underlying issue I was seeing then, since as you pointed out the event listener removal is delayed when called from within an event listener (which once I thought about it, does make sense).

[rh101]

Indeed it is, specifically a touch event on the button to close the game. This must be the underlying issue I was seeing then, since as you pointed out the event listener removal is delayed when called from within an event listener (which once I thought about it, does make sense).

There are a few ways to deal with this, the simplest being a once off scheduled call that exits on the next update loop, at which point it's no longer in the event listener.

void onTouchEventXYZ()
{
    scheduleOnce([](float) {
        // Call whatever it is you need to call to exit the app
    }, 0.f, "Exit");
}

[halx99]

A reproduce demo will be helpful, I can't reproduce this issue.

[TyelorD]

I'm going to close this PR until I have time to finish it properly and change the unit tests so that the checks succeed. I apologize, I would've gotten to it earlier: I've just been really strapped for time these past few weeks.

If someone else wants to take over this PR for me, then please feel free to re-open it. If not, then when I have time to finish this PR I'll reopen it.