bepu / bepuphysics2

Pure C# 3D real time physics simulation library, now with a higher version number.
Apache License 2.0
2.25k stars 261 forks source link

Handle versioning #308

Open RossNordby opened 4 months ago

RossNordby commented 4 months ago

It's been on the to-do list for a long time, so it deserves an issue!

Handles aren't versioned. A handle associated with a removed body or constraint could later refer to a different body or constraint.

This opens the door to a lot of use-after-free style bugs. Functions like ConstraintExists are also confusing as a result.

Using a simple low bitcount counter would suffice to catch many cases of misuse (even a single bit would help), but it would not be good enough to make ConstraintExists reliable without expanding the bittedness. A large, long running simulation could easily use more than 16 bits of versioning.

In other words, if you add 2^N constraints over the course of a simulation, you need 2^N unique handles, and that requires N bits. A simulation that creates 1024 new constraints per frame (which is well within realistic limits) would have less than a day of uptime before things like ConstraintExists could potentially fail with 32 bit handles.

Bumping up to 64 bit handles trivially lets every handle value have 32 bits of versioning. That's probably enough for nonpathological use cases, but one could still add/remove billions of times fairly quickly. Going even further out to 64 bits of versioning would make even pathological uses a non-issue, but makes the user-held handles awkwardly sized. Doing something like having a single counter for all constraint handles rather than using separate versioning bits would share the space well enough to stay in 64 bits, but we need low magnitude handles for a lot of the direct mapping stuff. You could still have that, it would just require some kind of additional work (e.g. conceptually, a hash map would suffice).

Having a low-bittedness counter that is allowed to roll over is a lot simpler and keeps the weight of handles down. If we stay with that strategy, things like ConstraintExists should probably be documented/named better to avoid the (reasonable) assumption that handles are not vulnerable to reuse.