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 debug line drawing with tiny convex hull mesh colliders #69197

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Nov 26, 2022

Godot's convex hulls support lines and planes, but the code to draw debug lines currently disables itself if there are fewer than 4 points. This means that the debug lines are not able to draw valid convex hulls with 2 or 3 points, even though there is no reason the code couldn't do that.

@lawnjelly
Copy link
Member

lawnjelly commented Nov 26, 2022

In terms of a lot of usage, if a convex hull has no planes, it is undefined. So e.g. a point or line segment is not strictly speaking a valid convex hull - there can be infinite planes for both, and they have no volume. So whether these will work in many convex hull routines I don't know (in most hull generation / sliding code I have written in the past this would be an error, but it's just possible the existing godot physics routines might deal with it gracefully). But very dodgy territory.

A tetrahedron should be okay though, it has volume.

@aaronfranke
Copy link
Member Author

aaronfranke commented Nov 26, 2022

@lawnjelly The current Godot physics is able to deal with points and lines gracefully aside from the few places in this PR that I modified. More details: The uint32_t size_to_reserve change is needed to support tris/tetrahedrons, and the editor if (points.size() > 1) { change is a no brainer to keep (if there is physics data, it makes sense to display it).

If we wanted to change the minimum to 3 points, we would remove the if (unlikely(ch.edges.size() < 3)) { bit, and change the check in ConvexPolygonShape3D::set_points to check size() < 3, but then it would be an almost arbitrary limitation since supporting in Godot just needs modifying one check and adding one check.

@aaronfranke aaronfranke force-pushed the fix-tiny-convex branch 2 times, most recently from 1e9483c to a93d10a Compare January 7, 2023 05:45
@aaronfranke
Copy link
Member Author

aaronfranke commented Jan 7, 2023

@lawnjelly Updated the PR. #70403 fixed the worst offender, and I removed the changes I made to support convex hulls with 1 or 2 points. Now it's just a no-brainer update to the editor code to support drawing debug shapes for fewer points (only two points are needed to draw lines in the editor).

@aaronfranke aaronfranke modified the milestones: 4.0, 4.1 Feb 15, 2023
@akien-mga akien-mga requested a review from lawnjelly June 11, 2023 08:47
@lawnjelly
Copy link
Member

lawnjelly commented Jun 11, 2023

This PR fixes a few problems I found with tiny convex hull meshes.

Probably needs a physics guy to decide on, I'm not sure from the title of this class ConvexPolygonShape3D what it is meant to represent, and there are no comments, so it is effectively defined by its usage.

If it is meant to be a convex hull, my earlier comment was to caution that a convex hull is not really valid with less than 4 points (and these must form a tetrahedron with non-zero volume). This is the smallest form that can be changed from the 3 plane intersection form to point form back and forth, and lots of convex hull stuff will fail with an invalid hull. This may be the reason for the previous if (poly_points.size() > 3) checks which are removed by this PR.

On the other hand, there is an argument to support less than 4 points for editing purposes, and let the client code deal with the case of it being invalid (or store a bool in the class to indicate whether or not it is in a valid state, because checking for zero volume is expensive). This is commonly done (I do this in the Portal class for example to allow editing).

It might alternatively be intended to handle both convex hulls and non-convex hulls, but that would need a physics guy to confirm.

So in summary:

  • If a physics guy can confirm that it is intended to work with non-"convex hulls" that would make it ok.
  • If not, at the least I would be tempted to add a valid / invalid flag and check this before use (and reject and possibly flag errors if attempting to use a non-valid hull).
  • If the convex hulls are not meant to be editable, then perhaps the existing method of disallowing non-valid hulls to be set could still be appropriate.

@aaronfranke
Copy link
Member Author

@lawnjelly For the replacement of if (points.size() > 3) { with if (points.size() > 1) {, this is done because this is the minimum requirement for the debug code to draw lines. Regardless of what we want to enforce as the minimum, if some data with only 2 or 3 points gets turned into a convex hull, we should still show it in the debug lines, instead of having it be invisible.

@aaronfranke aaronfranke changed the title Fix errors with tiny convex hull mesh colliders Fix debug line drawing with tiny convex hull mesh colliders Mar 27, 2024
@aaronfranke aaronfranke modified the milestones: 4.3, 4.4 Jul 22, 2024
@aaronfranke aaronfranke requested a review from a team as a code owner October 2, 2024 22:20
@aaronfranke aaronfranke requested a review from a team as a code owner October 30, 2024 07:49
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess as aaron has been so carefully keeping up this PR we should consider trying it, and in the absence of a full time physics maintainer I guess we have to decide as best we can.

As I say, I'm not sure about the logic of being able to have convex hulls with less than 4 points, but if this is already allowed, then I guess displaying it makes sense.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lawnjelly Let's do the best we can!

@Repiteo Repiteo merged commit 7982030 into godotengine:master Nov 1, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 1, 2024

Thanks!

@aaronfranke aaronfranke deleted the fix-tiny-convex branch November 1, 2024 07:43
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.

6 participants