-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #70: PooledEntity: entity not fully reset #72
Conversation
@@ -81,4 +81,25 @@ public void entityRemovalListenerOrder() { | |||
|
|||
engine.removeAllEntities(); | |||
} | |||
|
|||
@Test | |||
public void resetEntityCorrectly() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot rely very much on this test because if we add a new member it won't test it gets properly reset.
Can you please see the comments and update the PR please? Thanks for the fix! |
I've updated the test to loop over 10 entities and test all of them although I've hit a snag; should we remove all listeners when a PooledEntity is reset? To my mind, "reset" means return it to the state it would be in if we called The current implementation doesn't reset componentAdded, componentRemoved or familyBits. Is that acceptable? |
You're right, can we please reset those fields too? Thanks a lot. |
update test accordingly.
OK they're now reset with a new removeAllListeners method which I think is reasonable API addition. Test also checks for it. |
Thanks a lot @SgtCoDFish, keep up the good stuff! |
Fixes #70: PooledEntity: entity not fully reset
componentOperationHandler was not set to null when an entity was removed from an engine, which messes with component changes.
I'm not sure what the description in #70 means with the signals not being reset correctly; the test I added seems to confirm that everything works as expected.