ceramic-engine / ceramic

Cross-platform 2D framework written in Haxe that can export natively to desktop (windows, mac, linux), mobile (ios, android), web (js + webgl) and to unity projects
MIT License
263 stars 22 forks source link

Nape bodies are not removed from the nape space when being destroyed #70

Closed qeshi closed 3 years ago

qeshi commented 3 years ago

Hi!

Only the visual part gets removed if I call destroy() on a Visual instance that I have initNapePhysics() on. So the nape physics objects was still around even thought I destroyed() them.

So I added some code to remove the nape body from space. I don't know if this i the best place for this code though, but it seems to work without crashing. :)

jeremyfa commented 3 years ago

That's right, thanks for pointing that out.

I took a look at nape physics docs (https://joecreates.github.io/napephys/help/manual.html#Constraints) and they suggest to do that to remove the constraints:

while (!body.constraints.empty()) {
        body.constraints.at(0).space = null;
}
body.space = null;

I'm no expert in nape, but it seems safer to me to completely remove the constraint instead of just making it active = false, don't you think so?

qeshi commented 3 years ago

I'm no expert in nape either :) and that seems like a much better solution!

I tried to clear() the body.constraints list first but it was immutable so that is why I only deactivated them.

jeremyfa commented 3 years ago

Can you update your PR with the suggested change above? Then I'll merge it :)

qeshi commented 3 years ago

Yes you are right! I just need to figure out how to update a pull request it on Github. 😊

I also noticed that my code stopped working if removing the constraint all together. So I figured that it should be left to the programmer to clean up the constraints if they need to, depending on how their program is structured.

qeshi commented 3 years ago

Wait did my update work?

jeremyfa commented 3 years ago

The removal of constraints is missing (from what I see here: https://github.com/ceramic-engine/ceramic/pull/70/files)

jeremyfa commented 3 years ago

Ah sorry I didn't see your message about the code stopping to work. Mmmh, I don't know then. Maybe removing the constraints is the responsibility of the developer indeed. I'll merge it like it then for now.

qeshi commented 3 years ago

Ok, thank you!