Skip to content

Conversation

@rh101
Copy link
Contributor

@rh101 rh101 commented Sep 27, 2022

Describe your changes

If a component is added to a parent node after the parent Node::onEnter() has been called, then the Component::onEnter() would never be called.

This fix ensures that if the component is added while the parent node is already running, then the Component::onEnter() is called.

Issue ticket number and link

#866

Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.

@DelinWorks
Copy link
Contributor

DelinWorks commented Sep 27, 2022

maybe remove the Component::onEnter from Node::onEnter since your're calling Component::onEnter anyways when it gets added, that's to guarantee that Component::onEnter doesn't get called twice and bring frustrating bugs

for reference:

void Node::onEnter() {
    if (_componentContainer && !_componentContainer->isEmpty()) {
        _componentContainer->onEnter();
    }
}

portion should be removed.

@rh101
Copy link
Contributor Author

rh101 commented Sep 27, 2022

maybe remove the Component::onEnter from Node::onEnter since your're calling Component::onEnter anyways when it gets added

That code should not be removed, since it is still required. Review the code carefully and you will see why.

The Node::_running flag is false until the end of the Node's onEnter() method, so if a component is added prior to Node::onEnter() being called, the only place Component::onEnter() will be called is in Node::onEnter().

Also, if the Node::onExit() is called, which in turn calls Component::onExit(), and then Node::onEnter() is called later, then the components would still need Component::onEnter() to be called. Node::onEnter() is the only place this can occur.

That's why the code is correct and should not be removed at all.

@DelinWorks
Copy link
Contributor

I see..

@aismann
Copy link
Contributor

aismann commented Sep 27, 2022

Maybe a test (cpp-test) with different scenarios will be usefull too?

@halx99 halx99 merged commit 5c1b2d4 into axmolengine:dev Sep 27, 2022
@rh101
Copy link
Contributor Author

rh101 commented Sep 27, 2022

Maybe a test (cpp-test) with different scenarios will be usefull too?

I don't see any way to test this, as the side effect of the original code is that onEnter() is never called, which would cause undesired behaviour in a component. This fix simply ensures that Component::onEnter() is called regardless of when the component is added to the parent.

The sub-classes that inherit from Component and override Component::onEnter() are Physics3DComponent, NavMeshObstacle and NavMeshAgent, all of which would be buggy if they weren't added to the parent node prior to the parent running Node::onEnter(). The current test cases cover them all, and the changes here have no impact on their operation.

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.

4 participants