melonjs / melonJS

a fresh, modern & lightweight HTML5 game engine
https://melonjs.org
MIT License
5.9k stars 645 forks source link

me.pool.pull additional class arguments and me.Matrix2d #869

Closed obiot closed 7 years ago

obiot commented 7 years ago

I noticed that yesterday when working on something else,

a visual way to see the bug is to replace the following line (used by the WebGL renderer save/restore methods) and run the platformer : https://github.com/melonjs/melonJS/blob/master/src/math/matrix2.js#L372 by

return me.pool.pull("me.Matrix2d", this);

Now the funny part is that, if I debug this step by step, I can see the extra parameter being passed to me.Matrix2d, and the class internal val property being set accordingly. But when running the whole thing, it does not work .... and it does seems to work with other classes than matrix 2d...

parasyte commented 7 years ago

I think that's because me.Matrix2d.onResetEvent is defined (it reset the matrix to the identity); See https://github.com/melonjs/melonJS/blob/22401642a0e35ddcacfff5a62e5909ae30b4ab3a/src/system/pooling.js#L125-L127

obiot commented 7 years ago

holy moly.... and it reset the matrix actually.... i'm going to remove it then, does not bring anything for a matrix object.

parasyte commented 7 years ago

Make sure it doesn't break any of the cases where it is currently called:

src/math/matrix2.js
372:            return me.pool.pull("me.Matrix2d").copy(this);

src/renderable/renderable.js
43:                 this.currentTransform = me.pool.pull("me.Matrix2d");

It looks like the currentTransform path relies on the matrix being reset when pulled from the pool. Otherwise it's just going to be some random matrix in an undefined state.

parasyte commented 7 years ago

I agree, the onResetEvent isn't needed for Matrix2d. It should just reuse the init logic. FWIW, it was added here: https://github.com/melonjs/melonJS/commit/8b3d5c2d76ee461dc858ea5aa3d0955342142bfb

obiot commented 7 years ago

yes I know, I added it... :( totally useless and it broke other things as always :)

obiot commented 7 years ago

https://github.com/melonjs/melonJS/commit/524b9e328652a1a0f8e678b19a49d4187a139533

obiot commented 7 years ago

the matrix is always resetted if not parameters are passed, so we are fine wrt to the above example

parasyte commented 7 years ago

Looks good to me! :+1: