HaxeFlixel / flixel-addons

Additional classes for HaxeFlixel
170 stars 136 forks source link

What happens to the old body? Overwritten, or memory leak? #237

Open xerosugar opened 8 years ago

xerosugar commented 8 years ago

https://github.com/HaxeFlixel/flixel-addons/blob/dev/flixel/addons/nape/FlxNapeSprite.hx#L253

Not sure about this, but maybe

FlxNapeSpace.bodies.remove(this.body);

should be added before the

this.body = body;

?

thegoodideaco commented 8 years ago

if anything this adds extra steps. If you look here: https://github.com/HaxeFlixel/flixel-addons/blob/dev/flixel/addons/nape/FlxNapeSprite.hx#L289 FlxNapeSprite removes the body from space if it is part of space to begin with.

JoeCreates commented 8 years ago

Why do we have a setBody rather than just having a body property with a setter?

thegoodideaco commented 8 years ago

Good question. I would prefer that. Shall I implement it?

JoeCreates commented 8 years ago

@thegoodideaco That doesnt remove the old body. It determines whether the current body is in the space or not. Note that it gets called after setting the body in setBody. Looks like unless you remove the old body yourself, you'll have one left hanging around in space?

thegoodideaco commented 8 years ago

@JoeCreates yes you are correct. it looks like it adds or removes it from the space depending on if physicsEnabled is true, which makes sense

JoeCreates commented 8 years ago

Im not sure if it should automatically remove the body from space or not...

thegoodideaco commented 8 years ago

Well it does (for the new body) depending on if physicsEnabled is set to true. however if the old body is already set, and a part of space, then we immediately lose it as soon as we set this.body to another body.

thegoodideaco commented 8 years ago

I do think that this class should be looked at again. I don't like the fact that setPosition focuses on the top-left and the body is center anchored. It seems to me that the body should control all movement of the Sprite, while the sprite controls the graphics. Maybe adding getters/setters to the Sprite itself? But then again that could be a bad idea..

JoeCreates commented 8 years ago

@thegoodideaco The point in the original post is that the original body isn't lost, because the space still has a reference to it, hence the possibility of a memory leak. Do you think it should be automatically removed or left in space?

thegoodideaco commented 8 years ago

@JoeCreates I meant the reference is lost from the FlxNapeSprite. wether we remove it from space or not, it will still be floating around not being used (unless there is some reason to use it).

Another thing to consider is that a body's space can be changed _only _ if that body doesn't belong to a Compound. And you also have to consider any Constraints that it might be apart of, because a Constraint's bodies must both have the same space. setting space to null on a body with a constraint will throw an error

thegoodideaco commented 8 years ago

I don't see a reason for FlxNapeSprite to have more than one body accountable. I would probably want it destroyed since it's really only used by the helper functions for adding a box, circle or premade body. I've never swapped out a body after it's been initated but there might be some use cases I'm not aware of.

There are also other restrictions that we have as well, such as making the BodyType Dynamic right off the bat and immediately adding it to space. If I wanted it to be Kinematic, I would have to remove it from space, change the type, and add it back