Fix failed assertion and memory leak with event listeners

[TyelorD]

Describe your changes

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.

The issue I opened for this is listed below for additional discussion if this is the right solution or not (though I don't personally see much harm in doing this, perhaps this will require many of you to refactor your code that moves a Node from one parent to another).

I did replace the old Cocos2d-x comment here with one that describes the issue and what to do to avoid the cleanup if desired as well to hopefully help anyone who might update who relied on the old behavior.

Issue ticket number and link

1836

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.

[smilediver]

From a first glance this change might have quite big unintended consequences, so I hope this will be thoroughly reviewed.

One of the bigger problems might be is that Node::removeFromParent() calls removeFromParentAndCleanup(true), but it sounds like it shouldn't, cause that with current change might cause issues. But the removeFromParent() method sounds like the one that user is supposed to use if he wants to remove a node from a parent, and I guess a lot of people do. Had a quick glance on our games, and they call removeFromParent() a lot.

[TyelorD]

From a first glance this change might have quite big unintended consequences

I listed the consequences that you brought up in my change description. I also listed why I believe that these consequences only impact some edge cases that aren't likely to be very common use cases anyways. The old logic still works if someone needs it (like we did for moving a Node from one parent to another), just pass false to any Node that is being removed from one parent to another.

It's also worth mentioning that this only impacts Nodes that have event listeners registered to them. For most Nodes this isn't going to be the case, and mainly impacts things like UIWidget classes, as well as any potential classes unique to your project that uses event listeners.

But the removeFromParent() method sounds like the one that user is supposed to use if he wants to remove a node from a parent, and I guess a lot of people do.

This is true but is only an issue if you're removing a Node from a parent so you can add it as a child to another parent. If you're simply removing a Node from it's parent with the removeFromParent() function in order to get rid of that Node, then this change does nothing except cleanup leaked memory for that Node's event listeners.

If you are removing a Node from its parent to add it as a child to another Node, then you simply need to pass in a value of false to the removeFromParent() function call.

Hopefully this addresses your concerns!

Tldr; I still believe that removing memory leaks is better than keeping them in simply for a small handful of users with a small edge case.

[rh101]

This PR introduces the correct behaviour. A minor inconvenience now for developers affected by it is, in my opinion, well worth it. The benefit is not having to remember that the event listeners need to be released separately when a node is removed from the parent, and the removal of code associated with this task.

If this PR is merged in, then the difference in behaviour should be listed in the Cocos2d-x to Axmol migration guide.

[smilediver]

I don't like the way this was handled and that there was no discussion at all to find the best solution for the problem. I understand that the fix is "correct", but the biggest problem is that this fix changes the behavior and breaks the projects silently. I would have preferred a fix that would break the compilation to make you stop and review the usage places.

I also think it's unfortunate that these method names and their combination are a bit misleading. There are 2 methods: removeFromParent() and removeFromParentAndCleanup(bool cleanup). It sounds like removeFromParent() just removes the node from a parent, ie something you'd want to do if you wanted to attach it to another node, while removeFromParentAndCleanup(bool cleanup) sounds like it also does a cleanup, something you would do before destruction.

IMHO a better fix would have been to remove removeFromParentAndCleanup(bool cleanup), since there's no reason to have two different methods for doing the same thing, and change removeFromParent() to removeFromParent(bool cleanup) with an explicit cleanup parameter and adding clear method documentation about the parameter's consequences. This would have forced people to review their code, and also hopefully would make people think when they are removing nodes in the future.

[rh101]

IMHO a better fix would have been to remove removeFromParentAndCleanup(bool cleanup), since there's no reason to have two different methods for doing the same thing, and change removeFromParent() to removeFromParent(bool cleanup) with an explicit cleanup parameter and adding clear method documentation about the parameter's consequences. This would have forced people to review their code, and also hopefully would make people think when they are removing nodes in the future.

Is there any reason to not make this change to removeFromParent? It's a good way to bring attention to that call. I also agree with removing removeFromParentAndCleanup, as modifying removeFromParent and the removal of the event listener in the cleanup is a breaking change anyway.

@smilediver Can you please create a new PR with the exact changes you have suggested?

[TyelorD]

I also agree with @smilediver's proposed change, with the exception that I think cleanup should be implicitly set to "true" by default, so as to provide the correct memory cleanup behavior out of the box for new users. If people need to modify it because they don't cleanup their memory, or manage it another way, then they can just make a small tweak to the source code there and change it to false implicitly (or require it to be set explicitly).

(It's also worth noting the default behavior of Node::removeFromParent simply calls Node::removeFromParentAndCleanup(true); so it was always intended to cleanup the memory when removed from its parent. I also disagree with you that removeFromParent implies that it would be moving from one Node to another, since that is more rare use-case than simply removing an Node to destroy/remove it.)

[TyelorD]

IMHO a better fix would have been to remove removeFromParentAndCleanup(bool cleanup), since there's no reason to have two different methods for doing the same thing

I agree with your general premise, and outlined above why I disagree with a part of it. I've made a pull request with the change you suggested as well as my suggestion for cleanup to be implicitly defaulted to true with #1840

Thanks for your suggestion!