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

bullet: fix memory leaks related to Bullet #52

Closed
wants to merge 2 commits into
base: master
from

Conversation

2 participants
@xqms
Copy link
Contributor

xqms commented Jan 22, 2019

The Bullet API assumes all allocations by the user must also be freed by the user (source: https://pybullet.org/Bullet/phpBB3/viewtopic.php?t=1660). I think that's a good policy and we should follow it in the Bullet example to show users how to use the API correctly.

I found at least three separate leaks in the example:

  • The bullet world is never deleted
  • The injected dependencies (dispatcher, resolver, ...) are not deleted
  • All rigid bodies that are created are never deleted

The last one is also growing over time, as the user shoots more boxes.

I fixed this by reducing dynamic allocations to a minimum and using Corrade::Pointer in the other cases. Still, we require some custom deletion code in the destructor, because a btRigidBody should be explicitly removed from the btCollisionWorld before it is deleted - if someone has a better idea please tell me ;-)

I have to mention that this changes the behavior of the example slightly. If you shoot a new object, the old one is deleted first. If this is undesirable, we should use a growing list of shot items...

@xqms

This comment has been minimized.

Copy link
Contributor Author

xqms commented Jan 22, 2019

BTW, I tested this in valgrind --leak-check=full and the only remaining leaks are somewhere in SDL2 - not sure if these are our fault or SDL2's.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Jan 23, 2019

Oh, thank you very much! This code was contributed and I was always very suspicious about how the memory is managed there :) Recently I tried to reproduce the leaks in the web version, but the Emscripten fixed-size heap didn't blow up even after a while of "playing", so I put this aside and didn't look further. It's quite bad also because this code was taken as an examples "how to do things" by many students and beginners, suggesting bad practices :/

+1 for using the new Pointer class :)

Still, we require some custom deletion code in the destructor, because a btRigidBody should be explicitly removed from the btCollisionWorld before it is deleted.

Since every rigid body is attached to an Object instance, what about creating an Object subclass which would own the rigid body and take care of its removal on destruction? That way you wouldn't need the application destructor at all, I think, and also the objects vector wouldn't be needed.

If you shoot a new object, the old one is deleted first. If this is undesirable, we should use a growing list of shot items...

That... takes a bit of the fun out, since you can't do carpet bombing anymore :) A growing list of shot items would be better, yes. Or, if you do the Object subclass, then this would be also handled implicitly by the SceneGraph parent/child memory management.

To avoid unbounded memory growth, I was thinking about iterating through all objects on every cube shoot and throwing away the ones that are too far from the origin. For example iterate on _scene.children() and then call delete on objects where obj.transformation()->translation().dot() > 100 or something. I'm a bit torn between "this handling is not needed at such small scale" and "the example should suggest good practices and so this should be here", though :)

the only remaining leaks are somewhere in SDL2

Could it be possibly the same place as described here? #23 (comment) Interesting if it still didn't get fixed.

@mosra mosra added this to the 2019.01 milestone Jan 23, 2019

@mosra mosra added this to TODO in Project management via automation Jan 23, 2019

@xqms

This comment has been minimized.

Copy link
Contributor Author

xqms commented Jan 23, 2019

Since every rigid body is attached to an Object instance, what about creating an Object subclass which would own the rigid body and take care of its removal on destruction? That way you wouldn't need the application destructor at all, I think, and also the objects vector wouldn't be needed.

Okay, sounds like a good proposal. Or would it make sense to offer the btRigidBody as a SceneGraph Feature? That way we could offer this logic in magnum-integration, the same way the MotionState integration is implemented. In any case we need to carry an additional pointer to the bullet world around, since a btRigidBody does not know which world is attached to, but we need that information on destruction to properly unregister it.

That... takes a bit of the fun out, since you can't do carpet bombing anymore :)

Yep, I can definitely understand that ;-) I'll implement a growing list with deletion of far away objects for completeness sake. As you said, people (myself included) look at these examples as the "gold standard" in terms of API usage and best practices. Ideally, the presented solutions should not break when you scale them to real-world problems...

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Jan 23, 2019

would it make sense to offer the btRigidBody as a SceneGraph Feature?

It would only handle correct detaching from the bullet world at the end, and I think that's not enough to warrant creating a whole new feature for it :) Besides that, there are soft bodies and other things, so for completeness we would need to wrap all of them. Since people need to interact with Bullet anyway (and handle all that manual memory management themselves), I think the less magnum obscures it, the better.

Ideally, the presented solutions should not break when you scale them to real-world problems...

Yup, exactly :)

@xqms

This comment has been minimized.

Copy link
Contributor Author

xqms commented Jan 25, 2019

Carpet bombing restored as ordered. The new approach exposes two new gotchas:

  • The Bullet world needs to live longer than the scene (for the removeRigidBody() call). This is now guaranteed by the member variable order, but could be unexpected.
  • We need to manually sync the magnum/bullet poses since we now create the btRigidBody first and then move the object.

I'd say both points are okay and do not require further changes to the example.

Feel free to squash commits on a later merge ;-)

void BulletExample::drawEvent() {
GL::defaultFramebuffer.clear(GL::FramebufferClear::Color|GL::FramebufferClear::Depth);

/* Housekeeping: remove any objects which are far away from the origin */
for(Object3D* obj = _scene.children().first(); obj; )

This comment has been minimized.

@xqms

xqms Jan 25, 2019

Author Contributor

Side-note: Do you have a nicer way of iterating & deleting children than what I did here?

This comment has been minimized.

@mosra

mosra Jan 25, 2019

Owner

I thought about while(Object3D* obj = _scene.children().first()), but that would be an infinite loop I'm afraid, since obj = next at the end wouldn't do anything.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Jan 25, 2019

Carpet bombing restored as ordered

😂 👍

The Bullet world needs to live longer than the scene

This is okay, but I'll put a comment there to make this clearer.

Thank you! Will merge this later today / tomorrow, I'm in the middle of doing some unique_ptr -> Pointer cleanup, so it might take a while :)

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Jan 27, 2019

Merged in c715253 with minor modifications (adapting the style to the surrounding code).

Thanks a lot! 👍

@mosra mosra closed this Jan 27, 2019

Project management automation moved this from TODO to Done Jan 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment