playcanvas / engine

JavaScript game engine built on WebGL, WebGPU, WebXR and glTF
https://playcanvas.com
MIT License
9.55k stars 1.34k forks source link

Collision contact normal is inconsistent through re-enabling the 'other' entity. #4547

Open Ri0ee opened 2 years ago

Ri0ee commented 2 years ago

Description

Engine ver 1.55.4

Steps to Reproduce

  1. Explore the https://playcanvas.com/project/970566 project, fork it
  2. Run it and check the console output
LeXXik commented 2 years ago

This is how Ammo reports these normals. It is not a PlayCanvas issue.

Edit: You can use a debug drawer to see the physics world that Ammo sees. If you set the render mode to 8, you should see the contacts data. https://playcanvas.com/project/744419/overview/ammo-debug-draw

mvaligursky commented 2 years ago

I wonder if there's some issue with re-adding the collider to Ammo (for example rotated 180deg), which could something like this perhaps? Maybe something is not converted to correct coordinate system.

LeXXik commented 2 years ago

No, not really. Just Ammo quirks and how it calculates the normal of a contact point between two bodies. It mostly relates to the order that the bodies entered the physics world, which affects the order the solvers solve the collisions. For example, here, you can just change the order of the Floor and Cylinder in the Editor hierarchy to see the effect:

https://playcanvas.com/project/970930/overview/ammo-quirks

There isn't much you can do, I'm afraid. If the normal is needed for generating a force, since we know the position of the body, one can invert it, if it points the other way.

mvaligursky commented 2 years ago

But don't you at least get two colliders that have contact in different order? That would allow you to use that order to invert the normal.

LeXXik commented 2 years ago

@mvaligursky not sure what you meant, but yes, as I mentioned, if the normal is pointing the other way, it is trivial to invert it in user script, before consuming. We don't even need 2 bodies, since we have a contact point and a body position, it should be enough.

Unless, you are suggesting PC should handle tracking the normal orientation? I would suggest not to.

mvaligursky commented 2 years ago

You seem to suggest there possibly is a quirk in Ammo. I'm saying that there is a way this is correct behaviour, without any quirk, in case the contact information specifies two bodies that have contact. It could be that they list these two contacts:

What I mean is that the normal is relative to the order of the colliders?

kungfooman commented 2 years ago

What I mean is that the normal is relative to the order of the colliders?

I just tested that and other is always the floor, since the collider is registered on the box.

After some testing I figured the normal can be calculated from the local collision points: https://playcanvas.com/project/971123/overview/rigidbody-collision-test

Basically:

var Test = pc.createScript('test');
Test.attributes.add('groundEntity', { type: 'entity' });
Test.prototype.initialize = function() {
    this.entity.collision.on('contact', this.onContact, this);
    setTimeout(() => {
        this.groundEntity.enabled = false;
        this.entity.rigidbody.linearVelocity = pc.Vec3.ZERO;
        this.entity.rigidbody.teleport(0, 5, 0);
    }, 3000);
    setTimeout(() => {
        this.groundEntity.enabled = true;
        this.entity.rigidbody.linearVelocity = pc.Vec3.ZERO;
        this.entity.rigidbody.teleport(0, 5, 0);
    }, 6000);
};
Test.prototype.onContact = function(hit) {
    function replacer(key, value) {
        if (typeof value === "number") {
            return value.toFixed(2);
        }
        if (value instanceof pc.Vec3) {
            const {x, z, y} = value;
            return `[${x.toFixed(2)}, ${y.toFixed(2)}, ${z.toFixed(2)}]`;
        }
        return value;
    }
    console.log("this", this);
    console.log('hit.other.name', hit.other.name);
    if (hit.other.name !== 'Floor') {
        debugger;
    }
    this.lastContacts = hit.contacts;
    for (const contact of hit.contacts) {
        console.log(JSON.stringify(contact, replacer, 2));
        //console.log('point.y', contact.point.y.toFixed(2), 'pointOther.y', contact.pointOther.y.toFixed(2), 'normal.y', contact.normal.y);
    }
};
Test.prototype.update = function(dt) {
    if (!this.lastContacts) {
        return;
    }
    for (const contact of this.lastContacts) {
        const normalFromLocalPoints = new pc.Vec3().sub2(contact.localPoint, contact.localPointOther);
        //const localPoint      = this.entity.getPosition().clone().add(contact.localPoint);
        //const localPointOther = this.entity.getPosition().clone().add(contact.localPointOther);
        //this.app.drawWireSphere(localPoint     , 0.1, pc.Color.GREEN , 20, false);
        //this.app.drawWireSphere(localPointOther, 0.1, pc.Color.YELLOW, 20, false);
        // point/pointOther
        const point      = contact.point;
        const pointOther = contact.pointOther;
        this.app.drawWireSphere(point     , 0.1, pc.Color.GREEN , 20, false);
        this.app.drawWireSphere(pointOther, 0.1, pc.Color.YELLOW, 20, false);
        this.app.drawLine(pc.Vec3.ZERO, contact.normal, pc.Color.RED, false);
        this.app.drawLine(pc.Vec3.ZERO, normalFromLocalPoints, pc.Color.CYAN, false);
    }
}

normalFromLocalPoints will render like this:

image

The red line is contact.normal which is first "right" and then "wrong":

image

mvaligursky commented 2 years ago

Nice! Should we create a PR with some helper function here or something similar?

LeXXik commented 2 years ago

Well, potentially we could change the way we construct the normal. Instead of using whatever Ammo is giving us directly, we could add additional test for checking if it needs an inverse:

https://github.com/playcanvas/engine/blob/91811cf100e10b3fb44078673ec4b6c0ed41bda7/src/framework/components/rigid-body/system.js#L577 https://github.com/playcanvas/engine/blob/91811cf100e10b3fb44078673ec4b6c0ed41bda7/src/framework/components/rigid-body/system.js#L594

However, this is a critical code, that runs every frame. The use of a contact normal is not as common and can be easily inversed by user, if need be. Unless, you are proposing some kind of utility function, like the procedural stuff, that user would run manually?

yaustar commented 2 years ago

Maybe it could be part of the getter of normal on the contact object? So it would only do the check and cache the result if the user access contact.normal?

kungfooman commented 2 years ago

The use of a contact normal is not as common and can be easily inversed by user, if need be.

I don't get it, how can you easily tell that a normal is "wrong"?

I guess best case would be someone takes the time to compile Ammo with debug symbols and trying to figure out why the normals don't work as expected in devtools and then fix Ammo itself.