matter-js icon indicating copy to clipboard operation
matter-js copied to clipboard

v0.18.0 introduces major bugs with collisions with sleeping bodies spamming collisionEnd indefinitely

Open fishmongr opened this issue 2 years ago • 20 comments

Upgraded from v0.17.1 to latest to hopefully see some performance upgrades. Verified performance is pretty much the same but there is a nasty bug introduced if you have Engine enableSleeping true. Bodies that collide with sleeping bodies will have Matter.Events.on collisionEnd spammed indefinitely until you restart the server. It's pretty easy to reproduce.

This issue will likely happen more frequently than expected to anyone using Engine enableSleeping true as it's quite difficult to manually manage sleeping bodies when you want and not have the lib auto sleep everything including static bodies, this is due to sleepThreshold=-1 being so difficult to set in a manner that sticks, but that is an unrelated older usability issue.

fishmongr avatar Jan 10 '22 22:01 fishmongr

Hey thanks for reporting, I'll look into this but if you happen to have a minimal example please do share.

Is there anything visible other than just that event getting too many calls?

liabru avatar Jan 10 '22 22:01 liabru

Unfortunately I don't have a minimal example to share at the moment but if you have any difficulty reproducing the issue I can try to elaborate or generate an example. My implementation is running in Node as the back-end for a Phaser game. As soon as I move the player over a static asset (created for example with Bodies.rectangle(x, y, width, height, { label: 'wall', isStatic: true }); the onCollisionStart fires like normal and then on exiting the overlap the onCollisionEnd triggers at 60fps indefinitely until moving back onto the item. It starts again when exiting the overlap. The problem goes away when rolling back to 0.17.1.

I created an implementation level fix in our game by ensuring our static bodies do not unintentionally get converted to sleeping by the global Engine config, by setting newBody.sleepThreshold = -1 and found it avoids the issue for static game elements like walls but for elements that need to be destroyed on collision the issue occurs again. This is because we recycle destroyed physic bodies into a recommended reuse Pool with sleep enabled (to avoid game stutter with MatterJS having to rebuild index internally every time an object is added or removed). This causes the onCollisionEnd spam to start again for those items as they are removed on onCollisionStart and put to sleep.

fishmongr avatar Jan 10 '22 23:01 fishmongr

@liabru if you have any consulting hours to spare we'd be happy to pay an hourly rate for you to review the integration of MatterJS in our game as we're working on performance optimizations for it. We have it set up in docker so pretty easy to get set up locally with it.

fishmongr avatar Jan 11 '22 15:01 fishmongr

@liabru I have also been seeing this bug the past few days. In my instance I have a game where when a body collides with another particular static body, the first body will be set to sleeping then removed after a set amount of time. What I am noticing is that the collide end event will continue to fire even after the body is removed from the world. I set up a code pen to demonstrate:

https://codepen.io/tropicalpudding/pen/LYzgaXK

tropicalpudding avatar Jan 11 '22 16:01 tropicalpudding

Thanks for the info both and @tropicalpudding for providing that example.

I've a preliminary fix for it here if you'd like to try it out and help review, that would be much appreciated: https://github.com/liabru/matter-js/pull/1079

liabru avatar Jan 12 '22 18:01 liabru

@liabru thanks for the quick patch! I tested it just now and unfortunately am still seeing the collisionEnd event getting triggered indefinitely after a collision exit. I also noticed CPU load for our test scenario, 10,000 completely static sleeping bodies and 1 active body goes from 20-30% load on v17.0.1 to average 40% load with this patch version.

fishmongr avatar Jan 12 '22 18:01 fishmongr

@fishmongr is it possible the collisionStart event is also firing the same way for your case? If so that might mean you have some bodies rapidly going in and out of collision?

As for using 10,000 bodies it sounds like you may really need to start looking into your own broadphase (it would be a case of swapping out Matter.Detector with your own). It really depends on the setup, but you may also consider merging them into compound bodies (basically creating islands / regions).

liabru avatar Jan 12 '22 19:01 liabru

@liabru I just retested with some additional logging on collisionStart and verified it is only getting triggered the one time as expected. Both collisionStart and collisionEnd occur only once as expected with v0.17.1 with the same code.

Thanks for the feedback on our approach. We are in the early stages of looking at options like that. We were surprised to find that loading 10,000 completely static walls and obstacles in our physics world would sustain such high CPU load (let alone any CPU load at all) when no active player bodies were added to the physics world to interact with them. Is that behavior you would expect? We were assuming our matterJS collision groups configuration was off but it appears to be up to what is outlined in the documentation.

fishmongr avatar Jan 12 '22 19:01 fishmongr

@fishmongr in the example provided by @tropicalpudding (which the PR seems to solve, but still testing through) the problem is specifically caused by bodies being removed (I think).

I've been trying out collision events with sleeping enabled without body removal and can't seem to trigger the end event being called like that so far. Can you try narrow down a test case for what you are seeing?

At the moment the default broadphase is much more optimised for dynamic bodies, but statics shouldn't be all that slow unless you're hitting some sort of n^2 test scenario. Usually I'd expect that sort of massive scaling optimisations for statics coming from game engine implementors rather than the physics engine core, but down the line I might see what it would take.

liabru avatar Jan 12 '22 20:01 liabru

@liabru I created a version of @tropicalpudding example that more matches our use case here: https://codepen.io/fishmongr/pen/mdBQaPP?editors=0011 The good news is that I was able to verify the patch stopped the collisionEnd spamming bug. The bad news is that now it triggers no collisionEnd events at all. I believe that is a bug with the patch.

I found I originally wasn't testing the patch when I thought I was because I was pulling it in via yarn add https://github.com/liabru/matter-js#fix/sleeping-collision-end but it turns out that you generated a separate version of the lib in the build directory called matter.alpha.min.js with the patch and the original matter.min.js included in that directory was unpatched. Explicitly testing the patched version with our game lead to the same results as the codepen I provided above, instead of spamming collisionEnd events it dispatches no collisionEnd events at all.

Thanks again for the additional insight on the current broadphase implementation being oriented towards dynamic bodies. I'm hoping we can make some easy tweaks in our own version of Matter.Detector or some small lib modifications in a fork to allow the lib to meet our needs in terms of static body scale. If you have hourly consulting bandwidth available our company would be happy to pay for your time on updating the lib to handle static bodies much more efficiently. This wouldn't need to be anything custom for us, but just improvements to the library itself. Let me know if that is something you'd have the bandwidth and interest in doing at this time. Thanks!

fishmongr avatar Jan 12 '22 22:01 fishmongr

After digging in it turns out a more complete fixed required a few more edge cases. I've updated the alpha build on #1079 if you want to try it.

@fishmongr thanks for providing the additional example it looks like the above update is working on it, take a look if you can and let me know.

I'll likely look to improve static scaling later on but for now other improvements have priority unless I see that it's affecting a lot of users (sorry not looking to consult on this at the moment though but appreciate the suggestion).

liabru avatar Jan 13 '22 12:01 liabru

@liabru fix is confirmed! Thank you. I have updated the CodePen above that shows the single collisionEnd is dispatching now.

No prob on the priorities. If your bandwidth or priorities with the lib change our offer should stand for some time, you can give me a ping at [email protected]. Unfortunately we'll likely need to stick to v0.17.1 for our use case due to the performance impact of v0.18.0 for our use cases.

fishmongr avatar Jan 13 '22 21:01 fishmongr

Great thanks for checking it out and confirming, I'll get a release out once I've finished testing through.

Yeah 0.18.0 is only the first release of the new broadphase, initial work was to focus on dynamics first being most important for a physics engine, but will revisit later for scaling statics (and sleeping). Keep in mind 10k bodies is still a lot in this context, so still worth looking into your own scaling solutions specific to your use case.

liabru avatar Jan 14 '22 10:01 liabru

Just thought that I should mention that I got this problem (collisionStart and collisionEnd is spammed) on v0.18.0 myself, but not in regards of sleeping bodies. But when I have a collision between a body in engine.world and bodies placed in a composite in engine.world. If I remove all of the bodies from the composite and add it to the engine.world directly instead, I dont get this bug. But this is probably something that you're already aware of? (I have tested revertig to v0.17.1, and there it works fine)

(Also since this this is my first comment on this project: MatterJS is awesome, you are awseome(!!!), and thank you so much for your work on this project 🙌🙌🙌🙌 )

erex1 avatar Jan 14 '22 11:01 erex1

@erex1 that sounds slightly different actually but maybe related, would you mind throwing an example of that like ones above? Also you can try the fix alpha build to see if that resolves it already.

liabru avatar Jan 14 '22 12:01 liabru

@liabru Ok, I may have jumped to conclusions. Even though adding the items to a composite triggered the bug in my project, it must be something more to it. Because when I try to have a composite and a body in a world in a simple codepen, it works fine. I'll need to do some more testing to find out what other factors that combine to reproduce the issue. Also I will test the alpha build when i get home to my computer 👍

erex1 avatar Jan 14 '22 13:01 erex1

@liabru Well this is awkward! I managed to reproduce it. I was adding bodies to the composite in a for loop, but I also managed to add the composite to the world in the for loop and not outside it. So basically if you add the same composite to the world twice it causes the infinite calls to collision start and end. So the weird thing is probably that it works perfectly fine on v0.17.1, because I would guess that adding the same composite twice is not something that should/needs to be supported?

Not sure if it is of interest anymore 😅 , but I created this codepen that reproduces this issue (by adding the composite twice): https://codepen.io/erex1/pen/yLzGwJe

erex1 avatar Jan 14 '22 13:01 erex1

Yeah 0.18.0 is only the first release of the new broadphase, initial work was to focus on dynamics first being most important for a physics engine, but will revisit later for scaling statics (and sleeping). Keep in mind 10k bodies is still a lot in this context, so still worth looking into your own scaling solutions specific to your use case.

Sounds good, thanks. BTW we forked v0.17.1 and commented out some functionality we didn't need and found something like a 600% improvement in CPU performance with those 10,000 static bodies in the world while our active bodies still worked the same. Could be a good reference for things that could be removed from your internal loops for static and sleeping bodies: https://github.com/aavegotchi/matter-js/pull/1/commits/3ca14034f511eb48c3a2f59692a8e2e8c23d7daa

10,000 is just a performance test to understand the boundaries of the physics system. As this engine is very popular with Phaser and Phaser is used to build large MMPORG game worlds I completely agree that the game logic itself should be optimized for the use case and shouldn't completely lean on the physics engine to do that, but the lib does seem to be unusually unoptimized for the static / sleeping elements of larger more serious game projects trying to leverage it. We plan on scaling the engine by splitting our huge maps into separate physics zones but the number of separate physics engine zones required depends most significantly on the performance of the engine with X number of bodies of all types.

fishmongr avatar Jan 14 '22 18:01 fishmongr

I wanted to report that I has similar issues with sleeping collisions when I switched to 0.18 I just tried the alpha build from here and the problem went away. So I can also confirm the fix, thanks!

(the issue for my game occurred when you use an ability that freezes time for everyone but the player called time dilation, https://landgreen.github.io/sidescroller/ )

landgreen avatar Jun 28 '22 02:06 landgreen

@landgreen thanks for the confirmation, I had been unsure about that fix but that's a great sign if it's working out in your game (nice updates too!)

liabru avatar Jul 27 '22 13:07 liabru