Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix separating axes for 3D cylinder-face collisions #89960

Merged

Conversation

Mknsri
Copy link
Contributor

@Mknsri Mknsri commented Mar 27, 2024

I'm not a 100% this is the correct fix, I would appreciate if someone who knows the SAT equations or the physics system better would look it over. However as far as I can see this fixes the issue and does not cause any additional ones (as far as I can see :) )

I think the issue is because when checking for lateral surfaces of a cylinder against the points on a face, the axis projection does not remove the cylinder position. This results in the axis pointing to the wrong direction and reports collisions when there shouldn't be.

I included a test scene to exhibit the issue and fix

  • sandbox scene contains only the cylinder, that gets stuck without this commit
  • sandbox_all scene contains a bunch of shapes rubbing against corners to make sure nothing else is broken
    sandbox.zip

Physics squad edit:

@Mknsri Mknsri requested a review from a team as a code owner March 27, 2024 23:00
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 28, 2024
@rburing
Copy link
Member

rburing commented Apr 1, 2024

Great find! This fixes cylinder-trimesh collisions (cylinder against ConcavePolygonShape3D and cylinder against CSG shapes with collision enabled); in your PR's description I've added a link to an issue that this PR fixes.

The issue #57476 that you originally linked (which is about cylinder-cylinder) is not fixed as far as I can tell; probably you were referring to some other cases mentioned in the comments on that issue.

Here is an explanation why your fix works, for completeness: When testing for collisions between a cylinder and a triangle face, the SAT solver constructs several axes in 3D space, and for each axis it checks if it is a separating axis. Three of the axes that are supposed to be constructed are those pointing specifically from the cylinder's tubular surface to the points of the triangle, in the normal direction of the tubular surface. If a point P belongs to the plane that (i) passes through the cylinder's center and (ii) is normal to the cylinder's axis, then the axis-to-test is given by the direction vector from the cylinder's center to the point P (which will be perpendicular to the cylinder's tubular surface). If you move the point P along the direction of the cylinder's axis, then the axis-to-test (normal to the tubular surface, pointing at P) does not change, so in general you can project P to the aforementioned plane, and take the direction vector from the center of the cylinder to the projection of P on the plane. The issue was that the plane was constructed in coordinates centered at the cylinder's origin, so the point should be constructed in the same way, as is done in this PR (or alternatively, the plane should be constructed in coordinates centered at the world origin, but your approach is probably better).

Please apply the change suggested by @AThousandShips (and squash your commits), and then we can happily merge this. Edit: Please also change the first line of the commit message to the current title of the PR.

The same issue seems to affect box-cylinder here:

const Vector3 &point = vertices_A[i];
Vector3 axis = Plane(cyl_axis).project(point).normalized();

As well as box-capsule here:

Vector3 point = p_transform_a.origin;
for (int l = 0; l < 3; l++) {
point += p_transform_a.basis.get_column(l) * he[l];
}
//Vector3 axis = (point - cyl_axis * cyl_axis.dot(point)).normalized();
Vector3 axis = Plane(cyl_axis).project(point).normalized();

Please try testing and fixing those in the same way, if you have time (in this PR or another, with me as coauthor if you like).

@rburing rburing changed the title Fix issue with 3D physics SAT collision_face resolution Fix separating axes for 3D cylinder-face collisions Apr 1, 2024
@Mknsri Mknsri force-pushed the fix/cylinder_face_edge_point_collision branch 2 times, most recently from 2754083 to 3788ddf Compare April 1, 2024 18:47
When checking for lateral surfaces of a cylinder against the points on a
face, the axis projection does not remove the cylinder position. This
results in the axis pointing to the wrong direction and reports
collisions when there shouldn't be.
@Mknsri Mknsri force-pushed the fix/cylinder_face_edge_point_collision branch from 3788ddf to 3f69af9 Compare April 1, 2024 18:48
@Mknsri
Copy link
Contributor Author

Mknsri commented Apr 1, 2024

Thanks for the thorough explanation @rburing. I've made the changes per your suggestion. I will have a look a the other cases as well. I need some time to setup a test scene to reproduce the bugs so I will open a new PR if I'll fix them.

@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 4, 2024
@akien-mga akien-mga merged commit d604899 into godotengine:master Apr 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Mknsri
Copy link
Contributor Author

Mknsri commented Apr 4, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

Thanks, happy to help 🥳

@Mknsri Mknsri deleted the fix/cylinder_face_edge_point_collision branch April 4, 2024 16:40
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cylinder gets stuck on seams of static body while sliding against it.
4 participants