search

schteppe / p2.js

JavaScript 2D physics library
Other
2.64k stars 330 forks source link

Consider not re-using event objects #233

Closed AndrewRayCode closed 5 years ago

AndrewRayCode commented 8 years ago

This is the same comment as https://github.com/schteppe/cannon.js/issues/249#issuecomment-166770288. It bit me again recently and this is a request to re-visit it.

I'm programming a game in a more immutable functional style, meaning that every frame of my game has a unique state object. If I get a reference to an object, anywhere in my code, I know that object will always be the same even if I want to use it later, in a setTimeout, etc. I know no one will "pull the rug" out from under me.

I've had a long running bug in my game where the player can jump forever, and I finally tracked down why. I track what objects the player is touching by storing data from the beingContact and endContact events. However, the time when I process the events into the playerContact object is variable. Because p2.js re-uses the event object, I would often not be processing the event I thought I would be. The rug was being pulled out from under me.

A solution for the user is to just make a shallow copy of the object, but I think there's more to consider than that.

Here's the considerations for not re-using event objects:

  1. I've come to appreciate the concept of "micro-optimizations" or "premature optimizations", where optimizations are made even though there isn't a bottleneck. In your reply to my comment you say it's less work for the GC. I'm impressed by your work on both this library and on Cannon.js, but have you seen this issue appear in a real world scenario? Where the garbage collector hangs the page while cleaning up event handlers? I create a unique state object every frame of my game (30-60fps) and haven't ran into any garbage collector issues.
  2. Any user who encounters this problem will have a very hard time debugging it. Because the old event is mutated into a new (still valid) event, when it's processed it won't look like anything is wrong. The fact that the user has entirely lost the reference to the old event will be impossible to find without digging through the source code or opening an issue. The difficulty of debugging is a serious problem for users. I've spent about 4 hours today tracking it down, even though I filed the original ticket!

Still very happy with this library. Mutating objects after handing them to third parties is dangerous. Most users probably won't ever see this bug, but I worry the ones that do have to go through unnecessary pain for a potential micro-optimization.

schteppe commented 8 years ago

I'm totally with you on all the points.

Ideally p2 would use the same API style as Box2D - the collision callbacks would get a Contact object sent into them, instead of an event. This way it would still mutate the objects, but since it's not an event object it would be more clear what's happening. The current event objects are too similar to the Event objects you get in DOM event listeners, which is a bit confusing.

Regarding the GC - I want to minimize it as far as possible, but also make the API as beautiful and easy to use as possible.