piqnt / planck.js

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

World step error #266

Closed quinton-ashley closed 10 months ago

quinton-ashley commented 10 months ago
Screenshot 2023-12-06 at 22 36 51

I'm not sure what the problem is but I can confirm that it's an issue with planck. If I use the version of planck this user had been using before, v1 alpha 4, there's no problems. But then if v1 beta 16 is used, the error occurs.

I don't know what the issue is, but one exceptional thing about this project is the entire game environment exists at all times, there's no section loading, so it makes the world very long, which I understand is not good for performance or accuracy, but nevertheless that's what this creator did.

I've a attached a zip file so you can test out the program yourself.

sketch1880137 - Dragonz10221.zip

zOadT commented 10 months ago

Hey! Thank you for reporting the issue. I did a quick bisect, the bug was introduced by commit b7ce8be80e6e4c2e71a5d5e3a08aca2676d7c990. The stack trace (at the commit) is

Uncaught TypeError: v is undefined
    transformVec2 Matrix.ts:223
    Distance Distance.ts:173
    testOverlap Distance.ts:749
    update Contact.ts:584
    updateContacts World.ts:977
    step World.ts:809

Stack trace for main:

transformVec2 Matrix.ts:236
    Distance Distance.ts:175
    testOverlap Distance.ts:743
    update Contact.ts:590
    updateContacts World.ts:978
    step World.ts:810

Will take a closer look tomorrow, I guess this should be a relatively easy fix

zOadT commented 10 months ago

The issue is caused by numeric inaccuracy. It can be solved in PolygonShape.ts by replacing

// (A)
matrix.combineVec2(center, 1, center, triangleArea * k_inv3, e1);
matrix.combineVec2(center, 1, center, triangleArea * k_inv3, e2);

with

center.x += triangleArea * k_inv3 * e1.x + triangleArea * k_inv3 * e2.x;
center.y += triangleArea * k_inv3 * e1.y + triangleArea * k_inv3 * e2.y;

It really is just a numeric issue because the issue shows up again if you set

center.x = center.x + triangleArea * k_inv3 * e1.x + triangleArea * k_inv3 * e2.x;
center.y = center.y + triangleArea * k_inv3 * e1.y + triangleArea * k_inv3 * e2.y;

and can be solved again by setting parentheses

center.x = center.x + (triangleArea * k_inv3 * e1.x + triangleArea * k_inv3 * e2.x);
center.y = center.y + (triangleArea * k_inv3 * e1.y + triangleArea * k_inv3 * e2.y);

Could it be that maybe some sizes/masses of bodies are outside of the recommend order of magnitude because they span the entire world?

@shakiba This issue wasn't present in alpha because (A) was

center.addCombine(triangleArea * k_inv3, e1, triangleArea * k_inv3, e2);

In general this seems to me to be numerically more stable because the terms being combined are probably the same order of magnitude. Shall we reintroduce a matrix.addCombineVec2 function and use it where previously addCombine was used in the entire code base? But might actually also have to check why this caused an error.

shakiba commented 10 months ago

Hmm, very interesting—thanks for reporting and investigating this!

How about assigning e1, e2 combination value to a temporary variable and then adding it to center?

shakiba commented 10 months ago

I prefer to have fewer functions in new matrix, but this seems a common use-case so I think adding a new function as you suggest is also a good idea.

shakiba commented 10 months ago

One more option is adding combine3Vec2 function, so that we can also use it here

@zOadT Up to you, feel free to do whatever you prefer

zOadT commented 10 months ago

Oh yeah, combine3Vec2 is also more similar to the other matrix functions. Will open a PR tomorrow!

shakiba commented 10 months ago

Is this resolved now?

zOadT commented 10 months ago

Yes (Sorry, forgot to close it)

shakiba commented 10 months ago

Thanks!

quinton-ashley commented 10 months ago

@zOadT @shakiba Thank you! Could you guys make a new beta version of planck in dist? or are you waiting to release v1?

shakiba commented 10 months ago

Yes, I will in a few days, I also merged a few changes

quinton-ashley commented 9 months ago

@shakiba It worked! Thank you :)