DanielChappuis / reactphysics3d

Open source C++ physics engine library in 3D
http://www.reactphysics3d.com
zlib License
1.54k stars 224 forks source link

SingleFrameAllocator and multiple world instances. #348

Open programingman opened 1 year ago

programingman commented 1 year ago

Hi again!

For a while I've been running in to an issue with seemingly random crashes when allocating or freeing memory. I think I've finally tracked it down to having multiple PhysicsWorld instances sharing a SingleFrameAllocator.

I know multiple worlds probably isn't a common use case, but here's what I've found, and potential solutions.

The problem is related to user code being able to cause allocations from the SingleFrameAllocator outside of PhysicsWorld::Update(). There may be multiple things that can trigger this, but most commonly, I'll use setTransform() and then testOverlap() or testCollision() and this will sometimes cause new elements to be added to mLostContactPairs via computeBroadPhase -> removeNonOverlappingPairs -> addLostContactPair. Usually this is fine, because mLostContactPairs is processed in the next physics update before SingleFrameAllocator is cleared/reallocated. But if another PhysicsWorld::Update() is called before then, the mLostContactPairs will be invalidated.

Here's an example timeline:

WORLD INSTANCE 1:
  -PhysicsWorld::update()
    -add lost contacts, process and clear lost contacts
    -resetFrameAllocator()
  -user code
    -setTransform(), testOverlap(), causes mLostContactPairs.add() <--------------------------------------------------------

WORLD INSTANCE 2:
  -PhysicsWorld::update()
    -add lost contacts, process and clear lost contacts
    -resetFrameAllocator() <--------------------------------------------------------
  -user code
    -move object, add lost contacts

WORLD INSTANCE 1:
  -PhysicsWorld::update()
    -lost contacts added in previous user code are now potentially being overwritten by other allocations, 
      or if the SingleFrameAllocator was resized, they will be in deallocated memory!

Potential solutions:

  1. Always call PhysicsWorld::update() immediately AFTER any user code related to that instance, rather than before which I'm doing.
  2. Have a separateSingleFrameAllocator for each PhysicsWorld instance.
  3. Add a flag to redirect SingleFrameAllocator to a different allocator when outside of PhysicWorld::update()
  4. Remove the resetFrameAllocator() from PhysicWorld::update() and call it manually after all updates are done.

No rush addressing this, I've implemented solution 3 as a workaround. And apologies if this has been addressed already in develop branch, I haven't dug in deep enough to confirm if it's been fixed there.

Thanks again for your time and this excellent library!

DanielChappuis commented 1 year ago

Thanks a lot for taking the time to report this (with all the details). I don't have the time to improve this right now but I will take a look when I have some time.