search

schteppe / p2.js

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

World.clear() sets options to default #165

Closed jakobmulvad closed 8 years ago

jakobmulvad commented 9 years ago

This is because it does a World.apply(this); on line 1175 which re-creates the world with default options. At the very least it should be documented that gravity etc. will be reset after clear() but ideally world options should be preserved.

I'm not sure what problem you are solving by re-calling the constructor here. Is it to reset all the timings? If so maybe this could be isolated in a separate function that is called by both the constructor and clear(). Or if you really prefer to call the constructor you could save a reference to the options used to create the world.

Thanks for a great engine!

schteppe commented 9 years ago

You are right. Not sure why I left it like that. If you'd want to call the constructor and wipe everything, you could just as well create a new World instance.

PR's are welcome! Otherwise, I could fix it

englercj commented 9 years ago

Related to photonstorm/phaser#1176 by the way @photonstorm. I know you fixed this by caching config in Phaser, but if this issue is fixed in P2 you may not have to do that anymore.

jakobmulvad commented 9 years ago

Wow there were a lot of typos in my issue description. I apologize.

I just tested removing the constructor call on a local branch. Everything seems to work and all tests pass.

However I (we?) need to understand why this was added in the first place before we can remove it. Perhaps this was a quick fix to solve some greater problem I can't see right now.

It seems to be related to serialization of the world. It was added in this commit https://github.com/schteppe/p2.js/commit/b769c3cb608933ea0196a321847f61921cfc8091 which is about adding the to/fromJSON functions. Maybe this rings a bell @schteppe ?

schteppe commented 9 years ago

Thanks for looking deeper into this @jakobmulvad!

It was indeed used together with the serialization methods, which are now removed (actually, I remade them from scratch and started PhysicsToy). It is also currently used in the Renderer class for the demos, to start or restart a scene without having to re-add world listeners and stuff. The Renderer and demos have to be tested carefully too. Maybe the time related properties of the world have to be reset manually when restarting a demo.

When looking for the removed World.prototype.toJSON method I noticed that the .clone method is broken because it depends on it. Good find :)

schteppe commented 8 years ago

Removed World.apply(this) from the method. Options should be cached by the World and provided to the constructor when it's run, or the method should not run the constructor at all. I settled for the latter :)

jakobmulvad commented 8 years ago

This was my approach as well :)