schteppe / p2.js

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

Phaser "body.removeShape" triggers error of undefined #208

Closed Weedshaker closed 8 years ago

Weedshaker commented 8 years ago

This is just a fast fix but fixed my issue of a shape which gets temporarely added to a body. I guess it triggers this issue (si.position of undefined) when removed but still in loop. Anyhow, there probably is a much better fix but I don't have time to dig deeper. Hope you can have a look at this issue!

Thanks

schteppe commented 8 years ago

Why would this happen in the first place? Can you provide some (phaser) code?

Weedshaker commented 8 years ago

I do add a shape to the body as followed:

this.hitShape = this.entity.body.addCircle(this.hitSize[0], this.entity.scale.x * this.hitSize[1], this.hitSize[2]);

and then by occasion remove it:

this.entity.body.removeShape(this.hitShape);

the error is triggered when any shape of the body gets a collision while the above shape has been being removed.

schteppe commented 8 years ago

Okay.. But I still don't get why it would happen. The removeBody method in Phaser seems legit. There must be some other piece of code modifying the .shapes array directly, somewhere...

Weedshaker commented 8 years ago

nope (not modifying any in the shapes array)! I would give you my code but its just spread through too many files and objects that it would give a good example, I may should reproduce the issue with a simple example. Though can't find the time yet. But all I do is to add that shape and remove the shape. I use a 100% the phaser api and don't hack around inside p2.js directly.

schteppe commented 8 years ago

If you could provide a simple example it would be super awesome

Weedshaker commented 8 years ago

I looked deeper into the issue and tried to fix it inside my code (which worked with a tiny timeout on removing the shape). Essential to trigger the error is that the main body removes the shape (triggered by onBeginContact / Narrowphase) at the same frame as the object impact occurs. Below a screen shot regarding the issue:

screenshot from 2016-02-12 10 47 32 [fyi => the outer red circle is a sensor]

1) Here you can see the main body (boy), swinging the axe (holding axe), which gets added as an extra shape (applied to the boy's body right seen as a greenish circle).

2) This constellation has an impact body (flying axe) or as well as the explosion. Ether one of this outer body impacts trigger the error. You are having both in the screen shot above, but the error occurs on ether one of them combined or separate.

3) On flying axe impact or explosion impact the boy removes the holding axe shape.

Hope this helps.

schteppe commented 8 years ago

Thanks. Will dig deeper into this when I find some time.

One idea: You remove the shape in the beginContact event? Try not doing that... Because the Narrowphase or World may be in the middle of processing the shapes. Instead, you can make a queue of shapes that should be removed in the next update() of your game loop. See https://github.com/schteppe/p2.js/wiki#events-fired-during-step

Weedshaker commented 8 years ago
World.prototype.removeBody = function(body){
    if(this.stepping){
        this.bodiesToBeRemoved.push(body);
    } else {
        body.world = null;
        var idx = this.bodies.indexOf(body);
        if(idx!==-1){
            Utils.splice(this.bodies,idx,1);
            this.removeBodyEvent.body = body;
            body.resetConstraintVelocity();
            this.emit(this.removeBodyEvent);
            this.removeBodyEvent.body = null;
        }
    }
};

Seems as Phaser does this correct for Bodies (bodiesToBeRemoved - array). But not for shapes... as far as I have seen on first glance. I am gonna dig a bit deeper to make sure, if there isn't something like a "shapesToBeRemoved" array. Anyway, this looks like I have to open a Pull request at Phaser. Sorry, for wasting your time!!!

Thanks for your kind support and hunting down my issue. I really should have red through the p2.js wiki and not only rely on Phasers api.

schteppe commented 8 years ago

Oh, yeah. Shapes to be removed... Hm. If we keep allowing things to be removed during a physics tick, then we need to add arrays for constraints, springs and everything in the World. And yea, we should add arrays for not only removal, but adding as well. Ideally, we would have a queue for all methods that shouldn't be used during the loop as well... I'm starting to see why Bullet don't allow removal at all, during the step.

Weedshaker commented 8 years ago

I got it fixed properly on my side:

this.game.time.events.add(10, () => {
   this.entity.body.removeShape(this.hitShape);
   this.hitShape = null;
});

I just didn't feel well with hacking around inside p2.js. Please, drop the PR... also, I don't think that you should add those arrays you mentioned above, this would just take more cpu juice. I may look into phaser to add this change but most likely lazer. LOL!

P2.js - ROCKS!!!

Again, thank you! Danke schön!

schteppe commented 8 years ago

Thanks for using the lib and for your help :D

Weedshaker commented 8 years ago

I was thinking that it may would be nice making a p2.js branch for beginners. Which handles the above mentioned constraint issue, the issue of this pull request, etc...

Then people can decide on their own, if they prefer more performance but errors thrown if they are not tidy (with the existing master branch) or an error proof "dummy" branch with a bit less performance, since making a few extra checks.

Just an idea... but I guess it could boost popularity with making the entry to p2.js easier. Cheers