piqnt / planck.js

2D JavaScript Physics Engine
http://piqnt.com/planck.js/
MIT License
4.91k stars 236 forks source link

Restitution (bounciness) Error #223

Closed quinton-ashley closed 1 year ago

quinton-ashley commented 2 years ago

Setting Restitution to 1 makes this bouncing ball go higher:

https://p5play.org/demos/index.html?file=fullBounce.js

The ball should just bounce back up to the same height. This is a really bad problem for certain types of jumping games. How could I fix this?

zOadT commented 2 years ago

Hey! Unfortunately I wasn't able to reproduce the bug yet. I tried something like:

const world = new pl.World(Vec2(0, -10));
// ...
var ground = world.createBody();
ground.createFixture(pl.Box(80, 30, Vec2(0, -200)))
// ...
let test = world.createBody({
    type: "dynamic",
    position: planck.Vec2(17, 10),
});
test.createFixture(planck.Circle(8), {density: 3, restitution: 1});

But the simulation also seems like there is some scaling going on, at least I feel like the ball is moving too fast. Could you please tell me what the object sizes in planck are?

quinton-ashley commented 2 years ago

Hmm yeah maybe I set something up wrong. Here's how I'm doing the scaling: https://github.com/molleindustria/p5.play/blob/master/lib/p5.play.js#L27

Am I just making the objects too small by dividing by the plScale which is 60? :0

and here is the relevant section for the world step: https://github.com/molleindustria/p5.play/blob/master/lib/p5.play.js#L3459

zOadT commented 2 years ago

I also thought about scaling but a radius of 8 / 60 ~ 0.13 should actually be fine and also worked in a test I did. But I noticed that the speed seems a bit off compared to the scenes I tried to build and thought that probably the gravity is also scaled by 60? In a test the bounciness indeed seems to become broken, but a bit differently compared to the behavior you are seeing. So maybe its because the ball gets too fast?

quinton-ashley commented 2 years ago

@zOadT Well the speed isn't supposed to be changing at all but yeah with 9.8 y gravity the ball does get faster.

I tried using a y gravity value of 10 like you did in your example and the bouncing was stable! The ball always bounces back to the same position. This must be problem with how planck uses decimal values for gravity.

zOadT commented 2 years ago

What?! Ok, I have to debug it at the weekend šŸ˜„ My current guess would also be that the cause is a floating point error when applying gravity. Still a bit hard to belief and there are some other possibilities, so I have to look closer into it. But thank you very much for sharing!

zOadT commented 2 years ago

Ok, I think I figured out what happens:

console.log('running fullBounce.js#');

let ball, platform;
let gridSize = 32;
let prevVel = 0
let prevEn = 0

function setup() {
    createCanvas(800, 400);
    new World(0, 9.8);

    ball = new Sprite(400, 200, 8);
    ball.bounciness = 1;

//  platform = new Sprite(400, 368, 96, 32, 'static'); // line (A)
}

function draw() {
    background(0);
    const en = ball.fixture.m_body.m_world.getGravity().y * (-ball.fixture.m_body.getPosition().y) + 1/2 * (ball.fixture.m_body.getLinearVelocity().y**2)
    const vel = ball.fixture.m_body.getLinearVelocity().y
    if(prevVel * ball.velocity.y < -1) {
        console.log(ball.position.y, ball.velocity.y)
    //    console.log('-------')
    //    console.log(en - prevEn)
    }
    prevVel = vel;
    prevEn = en;

    // Block (B)
    if (ball.position.y >= 348 && ball.velocity.y > 0) {
        ball.fixture.m_body.getLinearVelocity().y+=1 / 60 * ball.fixture.m_body.m_world.getGravity().y
        ball.fixture.m_body.getLinearVelocity().y*=-1;
    }

    fill(100);
    for (let i = 0; i < width / gridSize; i++) {
        for (let j = 0; j < height / gridSize; j++) {
            rect(i * gridSize, j * gridSize, gridSize, gridSize);
        }
    }
}

This code will print 347.49 -7.023333333333326 for the first bounce (at least on my machine). But when you swap out block (B) with line (A) this will be logged: 340.4666666666667 -7.023333333333326. Note that the velocity is the same but the collision system moves the ball up, therefore increasing the total energy in the system!

quinton-ashley commented 2 years ago

I think I kind of understand. Will it be possible for you to fix the collision resolution so it doesn't move the ball too far up? Initially thought this was just a floating point error but now I think you will have to solve that by calculating the energy at impact and then recalculating what the velocity should be at the ball's new position instead of how I assume the collision system currently works by just inverting the velocity value it has when it's technically below the top edge of the platform (adding energy to the system)? That would mean gravity of 10 works because only because it matches my units of 1 perfectly so the bottom of the ball aligns with the top of the platform?

zOadT commented 2 years ago

The problem is that I think we don't want to deviate from box2d's implementation. And though I think it would lead to a more accurate (but still in general not perfect) solution I think it would also lead to at least some confusing behavior in some other edge cases.

So currently I would prefer if the user manually adjusts the velocity if accuracy is required. ball.fixture.m_body.m_world.getGravity().y * (-ball.fixture.m_body.getPosition().y) + 1/2 * ((ball.fixture.m_body.getLinearVelocity().y + 1 / 120 * ball.fixture.m_body.m_world.getGravity().y)**2) should be constant (It is just E = E_pot + E_kin = m * g * h + 1/2 v^2 * m divided by m where v is adjusted for discrete timesteps) Using that formular you can calculate the correct velocity after a collision.

Still, the idea of adjusting the velocity is somewhat interesting. I'm contemplating about whether we could maybe implement it behind a flag or something like that, but I wouldn't assume this to be implemented any time soon.

quinton-ashley commented 2 years ago

It'd be great if you could implement it behind a flag. For now I'll just suggest that users should set gravity to a whole number.

quinton-ashley commented 2 years ago

eh I found out setting gravity to whole numbers doesn't necessarily work either. For example this code works, ball bounces at the same height.

let ball, floor;

function setup() {
    createCanvas(200, 200);
    world.gravity.y = 10;

    ball = new Sprite();
    ball.diameter = 50;
    ball.y = 30;
    ball.bounciness = 1;

    floor = new Sprite();
    floor.collider = 'static';
    floor.y = 190;
    floor.w = 400;
    floor.h = 5;
}

function draw() {
    clear();
}

But setting the floor's height to 10 makes the ball bounce higher and higher!

let ball, floor;

function setup() {
    createCanvas(200, 200);
    world.gravity.y = 10;

    ball = new Sprite();
    ball.diameter = 50;
    ball.y = 30;
    ball.bounciness = 1;

    floor = new Sprite();
    floor.collider = 'static';
    floor.y = 190;
    floor.w = 400;
    floor.h = 10;
}

function draw() {
    clear();
}

You can try out this code on my website: https://molleindustria.github.io/p5.play/ref/sprite.html

quinton-ashley commented 2 years ago

Another problem I found was that setting bounciness to .9 makes the ball bounce lose momentum but then it just keeps bouncing at a low momentum.

ball.bounciness = 0.9;
quinton-ashley commented 2 years ago

The reason this issue is important to me is because one of the lessons in my curriculum is how to make a game like Papi Jump and Doodle Jump. Can't work without bounce collision resolutions being dependable. Also it's confusing for students if it doesn't work. In what edge cases do you think it would be a problem if we made the proposed changes to increase bounce accuracy? I could try working on this myself if you could point me in the right direction of where this happens in the code.

zOadT commented 2 years ago

Hey! I'm still not really sure whether it is a good idea to implement it in planck, so I would prefer to not implement it currently. But I think I have an idea of how you can implement it as a user of the library: By using the begin-contact event on the world you should be able to recieve all the necessary information to apply the corrections at the end-contact event. You can use the equations above to apply the correct velocity at the end of the contact (maybe you can also just apply a velocity change based on the change of the position). I think we wouldn't do something much different if we would implement it directly in planck. So maybe that solves the problem? Sadly can't look more into it currently.

quinton-ashley commented 2 years ago

hmm okay thank you, I might try this later

quinton-ashley commented 1 year ago

@zOadT I get that you wouldn't want to change planck so that it's not in line with box2d anymore, but this is still a major bug. My students always comment on it and it's a bit embarrassing for me that a ball can't bounce on a flat surface correctly in p5.play without a custom implementation that ignores the physics engine. Since something that seems so basic is so broken, it makes the whole the whole physics engine seem jank even though the rest of it is super good.

Would you be able to fix this directly in planck but make it an option that has to be turned on? Correcting the bounce response of rotated physics bodies is a bit outside my abilities. I'm sure that you could do it much more easily than I could. I could even pay you for your trouble! šŸ™

zOadT commented 1 year ago

Really dont want to be the gamebreaker here but I think this shouldn't be in the library (also not behind a flag)

I think this could potentially be solved in user land but I have to experiment beforehand (if it isn't solvable in user land but we manage to find a solution on Planck's side I can also share it) Also no guarantee that this doesn't lead to other problems down the line šŸ˜…

Could you share an example of this again (the example links are broken and all my attempts don't show this behavior šŸ˜„) Best here so I can use it directly on my local testbed https://piqnt.com/space/

quinton-ashley commented 1 year ago

Okay I will!

quinton-ashley commented 1 year ago

@zOadT Here is the updated link: https://p5play.org/demos/index.html?file=fullBounce.js

quinton-ashley commented 1 year ago

Side note, I've also never been able to get the space editor? to work. I can't tell if it's supposed to be an editor or just a viewer. I can edit the code but it has no effect if I stop and restart it, even if I erase everything the simulation stays the same. I can even make a copy like this https://piqnt.com/space/rbALxRN9y and still it has no effect. I think if it's just supposed to be for viewing maybe don't let users edit the code, you can set it to read only.

zOadT commented 1 year ago

Hey! Here is a somewhat working version running, I hope this helps you:

const pl = planck;

const world = new pl.World(new pl.Vec2(0, 9));

var ground = world.createBody({
    position: new planck.Vec2(17 / 60, 6.133333333333334),
});
ground.createFixture(new pl.Box(80 / 60, 0.266))

let ball = world.createBody({
    type: "dynamic",
    position: new planck.Vec2(17 / 60, 3.3333333333333335),
});
ball.createFixture(new planck.Circle(0.066), {density:5, restitution: 1});

const step = 1 / 60

// TODO don't be lacy, save it weakmap or somewhere else...
let oldB = 0;
let oldE = 0;
let oldV = 0;

this.world.on('pre-solve', (contact, oldManifold) => {
    oldB = contact.getFixtureB().getBody().getPosition().y;
    oldE = world.getGravity().y * (-ball.getPosition().y)
        + 1/2 * ((ball.getLinearVelocity().y + step / 2 * world.getGravity().y) ** 2);

    oldV = ball.getLinearVelocity().y;
});

this.world.on('post-solve', (contact, oldManifold) => {
    const bodyB = contact.getFixtureB().getBody();
    const newB = bodyB.getPosition().y;
    const newE = world.getGravity().y * (-bodyB.getPosition().y)
        + 1/2 * ((bodyB.getLinearVelocity().y + step / 2 * world.getGravity().y) ** 2)

    const C = step / 2 * world.getGravity().y;

    const energyPotentialDiff = world.getGravity().y * (-(newB-oldB))
    // solves `energyProtentialDiff = energyVelocityV - energyVelocityPre`
    const v1 = -C + Math.sqrt(C ** 2 - 2 * energyPotentialDiff + oldV ** 2 + 2 * oldV * C)
    const v2 = -C - Math.sqrt(C ** 2 - 2 * energyPotentialDiff + oldV ** 2 + 2 * oldV * C)

    // TODO pick the correct solution
    bodyB.setLinearVelocity(new Vec2(0, v2));

    const correctedE = world.getGravity().y * (-bodyB.getPosition().y)
        + 1/2 * ((bodyB.getLinearVelocity().y + step / 2 * world.getGravity().y) ** 2)

    console.log(correctedE - oldE);

    // TODO Do the same for A...
});

This currently assumes the gravity to direct in y direction, to fix that you have to calculate using the scalar product. You can probably choose between v1 and v2 based on the sign, but I'm not sure about that (I am a bit confused about their absolute values, probably because of that step / 2 * world.getGravity().y term...)

Concerning Rotation: I am not sure whether Planck rotates bodies in the solve step or just changes their position. If it does not change their rotation this solution should be correct, otherwise I don't think it will make too much of a difference.

NOTE: This will probably add some unwanted velocity on e.g. moving platforms and maybe some collisions in general. Also this does not work if you manually apply gravity by applying a force every step.

quinton-ashley commented 1 year ago

Thank you! But I tried it and it doesn't seem to work. I noticed that you never used the newE variable, correctedE looks the same. What is the todo? // TODO pick the correct solution Is that for which one is correct v1 or v2? How can you tell?

_preSolve(contact, oldManifold) {
    const b = contact.getFixtureB().getBody();

    this.oldB = b.getPosition().y;
    this.oldE =
        world.getGravity().y * -b.getPosition().y +
        (1 / 2) * (b.getLinearVelocity().y + (this.timeStep / 2) * world.getGravity().y) ** 2;
    this.oldV = b.getLinearVelocity().y;
}

_postSolve(contact, oldManifold) {
    const bodyB = contact.getFixtureB().getBody();
    const newB = bodyB.getPosition().y;
    const newE =
        world.getGravity().y * -bodyB.getPosition().y +
        (1 / 2) * (bodyB.getLinearVelocity().y + (this.timeStep / 2) * world.getGravity().y) ** 2;

    const C = (this.timeStep / 2) * world.getGravity().y;

    const energyPotentialDiff = world.getGravity().y * -(newB - this.oldB);
    // solves `energyProtentialDiff = energyVelocityV - energyVelocityPre`
    const v1 = -C + Math.sqrt(C ** 2 - 2 * energyPotentialDiff + this.oldV ** 2 + 2 * this.oldV * C);
    const v2 = -C - Math.sqrt(C ** 2 - 2 * energyPotentialDiff + this.oldV ** 2 + 2 * this.oldV * C);

    // TODO pick the correct solution
    bodyB.setLinearVelocity(new pl.Vec2(0, v2));

    const correctedE =
        world.getGravity().y * -bodyB.getPosition().y +
        (1 / 2) * (bodyB.getLinearVelocity().y + (this.timeStep / 2) * world.getGravity().y) ** 2;

    console.log(correctedE - this.oldE);
}
quinton-ashley commented 1 year ago

Here's the whole file: https://github.com/quinton-ashley/p5play-web/blob/main/v3/p5.play-beta.js

zOadT commented 1 year ago

Hey! Let me try to explain what is the actual reasoning behind these calculations is, I think this will answer most of your questions:

As already mentioned in a comment above: Every body has a potential energy (gravity * height * mass, this is the world.getGravity().y * [y position]) and the kinetic energy (1/2 * (velocity ** 2) * m, these are the (1 / 2) * (bodyB.getLinearVelocity().y + C) ** 2 terms (ignore the details about the C part, this just has to do with the numeric of stepwise approximation)).

Now, when the ball hits the ground, the velocity will be inverted and adapted such that the kinetic energy of the ball stays the same. But additionally, to remove the overlapping, Planck will move the ball up a bit. This increases the potential energy, therefore the total energy of the ball and therefore let the ball jump higher. This is the reason why you are facing the problem.

In the code above, energyPotentialDiff is the amount of energy that was added to the body because of the change in position. Our goal is to adjust the velocity, such that we lower the kinetic energy by the amount we gained in potential energy (resulting in the total energy staying the same).

v1 and v2 are the two solution you can assign to the velocity of the ball to keep the correct total energy (there are two solutions because a ball at height h with velocity v has the same total energy as a ball at height h with velocity -v because v gets squared when you calculate the kinetic energy. Again, C makes this unnecessarily more complicated, just ignore it). Now which solutions should we choose? I think the velocity being closer the bodies velocity after the solve should be used (otherwise the velocity of the ball would show towards the ground (again))

Now, what is the difference between newE and correctedE? Well, correctedE is calculated after we adjusted the velocity. And in my tests correctedE - this.oldE was something around 10^-15, i.e. the energy basically stayed the same. If you log newE - this.oldE the difference is significantly higher (I think something around 10^-2.

BTW: I just implemented this for bodyB because it just so happens that this was the ball in my tests, you have to do the same with bodyA every time. Second: In bodyB.setLinearVelocity(new pl.Vec2(0, v2)); I just set velocity.x to 0, you probably want to just use whatever velocity.x currently is.

One important note: I explained everything in the case of a ball with bounciness 1. But our solution is more general, because we just corrected the energyPotentialDiff. This also works for non perfect collisions. The fact that we did not assume the energy to stay the same throughout the whole collision we also did not use oldE in our calculations (we just used the shift in position).

I hope this helps!

quinton-ashley commented 1 year ago

I tried to make it work but I don't understand what's wrong. Could you edit the file for me and send a pull request?

zOadT commented 1 year ago

Could you try to replace

// TODO pick the correct solution
bodyB.setLinearVelocity(new pl.Vec2(0, v2));

by

const v = bodyB.getLinearVelocity().y
bodyB.setLinearVelocity(new pl.Vec2(0, Math.abs(v - v1) < Math.abs(v - v2) ? v1 : v2));

And also duplicate the code for bodyA? Otherwise I would need a better description of the current behavior

quinton-ashley commented 1 year ago
Screenshot 2023-01-11 at 14 52 17
quinton-ashley commented 1 year ago

When I add that line the ball oscillates between increasing and decreasing in absolute velocity.

zOadT commented 1 year ago

I just checked, in my test I get

2.1316282072803006e-14
1.4210854715202004e-14
1.4210854715202004e-14
2.1316282072803006e-14
1.4210854715202004e-14
7.105427357601002e-15
2.1316282072803006e-14
-7.105427357601002e-15

You are also using gravity.y > 0, aren't you? If not you could try to negate the energyPotentialDiff value. Otherwise I currently also don't see the difference between the implementations unfortunately

quinton-ashley commented 1 year ago
Screenshot 2023-01-11 at 15 09 25
quinton-ashley commented 1 year ago

Here I'm printing the velocity as well as comparing the oldE and correctedE.

https://github.com/quinton-ashley/p5play-web/blob/main/v3/p5.play-beta.js

They are the same each time, but somehow get lower while total velocity gradually gets higher still.

This is my test code, the demo I sent eariler:

let ball, platform;
let gridSize = 32;

function setup() {
    new Canvas(800, 400);
    // error can be fixed by setting gravity to 10
    // but sometimes it still breaks
    // depending on the size of the ball and platform
    // https://github.com/shakiba/planck.js/issues/223#issuecomment-1215211279
    world.gravity.y = 9;

    ball = new Sprite(400, 200, 8);
    ball.bounciness = 1;

    platform = new Sprite(400, 368, 96, 32, 'static');
}

function draw() {
    background(0);

    fill(100);
    for (let i = 0; i < width / gridSize; i++) {
        for (let j = 0; j < height / gridSize; j++) {
            rect(i * gridSize, j * gridSize, gridSize, gridSize);
        }
    }
}
zOadT commented 1 year ago

I will try to take another look at the weekend

quinton-ashley commented 1 year ago

thank you! šŸ™šŸ» Do you think it's a problem with the implementation and our assumption of what the problem is was correct? or maybe it's something else entirely that's adding energy?

zOadT commented 1 year ago

I'm currently pretty confident that our assumption of what the problem is is accurate (I hope this doesn't age bad šŸ˜…). You can probably confirm this by comparing the kinetic energy pre/post-solve. If they are the same only potential energy can increase (there is the possibility of the whole energy calculation is wrong somewhere, but they work perfectly outside of the collision) Also the solution just worked nearly perfectly for me without any tinkering (were you able to reproduce my logs with my Planck code directly?)

So I assume it is the implementation. It could also be that to derive the formulas I used some assumption that only applies to my test case but not yours (but our setups seem so similar, that I don't know what it could be)

quinton-ashley commented 1 year ago

No I didn't test it with planck because I can't seem to use the Space editor for some reason. The goal is to make it work generally with p5.play, not just in a specific case. I trust you that it worked so no need for me to re-test it, it just doesn't work with p5.play.

How can the energy be solved as the same but then the velocity still increases? Is it possible that the energy calculation is wrong?

zOadT commented 1 year ago

Hey! I took another look and finally understood your problem (I only checked energy pre/post-solve and didn't realize that you were talking about the energy throughout the whole simulation šŸ˜…).

To see were the this energy comes from (the energy we are logging is negative, it is increasing, not decreasing) I logged the energy at every step and also in the begin-contact event:

// a lot of times basically the same value +/- epsilon
-29.979187499999966
-29.97918749999996
-29.979187499999956
begin-contact -29.876576731984787
pre-solve -29.876576731984787
-29.87657673198479
-29.876576731984795
-29.876576731984798
// a lot of times basically the same value +/- epsilon

What you see is that we already gained energy when begin-contact gets called! Before our pre/post-solve handlers the energy would have increased even more after post-resolve). I am surprised that something of interest happens before begin-contact gets called, but I haven't checked the code yet.

The problem is, that we still need the oldB and oldV values for each body and there is no pre-begin-contact were we can read these values before the energy changes.

So I went for the trivial solution of saving the values before each step

// somewhere in my update handler
// not in pre-solve anymore
this.oldB = this.ball.getPosition().y;
this.oldV = this.ball.getLinearVelocity().y;

this.world.step(delta);

Now the energy and the ball keeps the same all the time! Unfortunately I don't know any better spot to save these values. Maybe we can find one when we know what happens before begin-contact.

-29.99718749999997
-29.997187499999963
-29.99718749999996
begin-contact -29.736799913180217 // <- value is still wrong but gets corrected in post-solve
pre-solve -29.736799913180217
-29.99718749999996
-29.99718749999997
-29.997187499999974
quinton-ashley commented 1 year ago

Glad you found the fix! I will try implementing it. Yet, it doesn't seem like a very efficient solution. Do you still think this way is better than fixing the bug in planck, even though the bug occurs before the userland "begin-contact" function is called?

quinton-ashley commented 1 year ago

@zOadT I got it to work for bounciness (restitution) values of 1!

But how could other bounciness values such as 0.8 be factored into this?

Also if I rotate the platform it doesn't work. I could just keep the x velocity instead of setting it to zero but I think that doesn't really work but maybe it doesn't matter that much since the goal was to have it work with a non-rotated platform, where the bug was obvious but then I'd have to disable these calculations in any other case.

Check out my code: https://github.com/quinton-ashley/p5play-web/blob/main/v3/p5.play-beta.js#L4916

zOadT commented 1 year ago

Whoops, yeah made a mistake and assumed it would work with bounciness < 1 (I forgot that we just use the energy before the bounce. Therefore the solution is probably unnecessary complex currently) Unfortunately I don|t have time currently to look into it, will come back to that later

quinton-ashley commented 1 year ago

@zOadT A general solution to this problem that factors in x and y gravity, any additional forces on the collider, bounciness, and other properties like friction. I might as well replace planck entirely if solving this bug requires reimplementing the entire physics collision resolution algorithm in p5.play, which I don't want to do.

I don't want you to waste your time improving on that approach because it's clear to me that it isn't going to be useful for p5.play.

I hope in the future you can solve the bug in planck, under a flag if you prefer.

shakiba commented 1 year ago

@quinton-ashley

For implementing the games that you mentioned, how about setting the velocity to zero and applying impulse when the character hits a platform? Here is an example: https://piqnt.dev/space/hlliG9_Ce

shakiba commented 1 year ago

Also I tried to follow the discussion, but most of the links to code are not available anymore. Could someone help by providing testbed code to reproduce this issue?

shakiba commented 1 year ago

Iā€™m closing this since most of the links to reproduce the issue are not available. Please feel free to reopen with some code to reproduce the issue, preferably only physics or testbed code.