RandyGaul/qu3e

q3BroadPhase::InsertBox shouldn't call BufferMove for static bodies

doron-feinstein opened this issue · 1 comments

Hi there
Thank you very much for sharing this project - its a great way to learn how physics simulations work!
Just a quick optimization suggestion - it appears that calling BufferMove in q3BroadPhase::InsertBox results in q3BroadPhase::UpdatePairs adding pairs where both sides belong to static bodies.
Maybe it would be a good idea to add an argument to q3BroadPhase::InsertBox that indicates if the box is moving and set it to false when the box belongs to a static body?

That could definitely work, however adding another parameter to the user-facing API just to support a slight optimization is probably not a very good way to go about it. The API is usually of utmost priority. Instead, internally, the kind of body can be read, and if static perhaps an optimization could be made.

However, this optimization may not actually save any time simply due to the extra cost of checking the flag.

Additionally, modifying any of the code is in and of itself a risk, since new bugs can potentially be introduced.

Not to say your suggestion is a bad idea at all; you're correct that a potential optimization here exist. It would just need to be qualitatively decided whether or not adding such a modification outweighs the costs. Costs namely being:

  • Changing APIs and adding parameters should be viewed as extremely costly. APIs are of utmost importance, since they are user facing and directly affect user experience.
  • Is the change likely to introduce new bugs?
  • Is the optimization going to really be any faster than the cost of extra if-statements?
  • Is this optimization going to be solving any bottlenecks? If this optimization does not address a bottle-neck, the performance benefit (if any) may be impossible to realized in practice.

In any case, I'd be willing to take a look at a pull request :)

Closing this for now, but feel free to comment or ask more questions here. We can re-open it if we need to.