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

Options to clean/simplify convex hull generated from mesh #50262

Merged
merged 1 commit into from
Jul 12, 2021

Conversation

pouleyKetchoupp
Copy link
Contributor

Clean: remove duplicate and interior vertices (uses Bullet algorithm)
Simplify: modify the geometry for further simplification (uses VHACD algorithm)

In the editor, single convex hull generation now uses the cleaning option.
This PR also adds a new editor entry to create a simplified convex hull, can be useful for creating convex hull from highly tessellated triangle meshes, like in #48468 (from 40K vertices to 56 vertices, compared to 291 with just cleaning).

Note: It would be interesting to have a proper convex hull generation menu with options for the maximum number of shapes and the maximum number of vertices per shape, but that's out of scope for this PR and would require a proposal.

Fixes #41522
Fixes #48468
Supersedes #48907

Clean: remove duplicate and interior vertices (uses Bullet algorithm)
Simplify: modify the geometry for further simplification (uses VHACD
algorithm)

In the editor, single convex hull now uses the clean option.
Added a new editor entry to create a simplified convex hull, can be
useful for creating convex hull from highly tessellated triangle meshes.
@fire
Copy link
Member

fire commented Jul 7, 2021

Can you show a photo or a demo of the differences? Alternatively give a test project.

@pouleyKetchoupp
Copy link
Contributor Author

@fire With the MRP from #48468:

Screenshot Vertex count
Model phone_model 40K
Single Convex (current) phone_convex1 40K
Single Convex (cleaned) phone_convex2 291
Single Convex (simplified) phone_convex3 56

In this case, the mesh is highly tessellated so the simplified version helps a lot. You did actually get the same result from selecting multiple convex hull shapes, but it's just lucky because the original shape is convex.

In more simple cases, like a cube from a BoxMesh, cleaned and simplified give the same result, they just remove duplicate vertices.

@jitspoe
Copy link
Contributor

jitspoe commented Jul 8, 2021

Looks good. Have you tried this with an imported mesh using "convcolonly" in the name? That's how I most frequently use convex hull generation. I would expect that to use the single convex cleaned version, since I'm explicitly creating geometry for collision.

@lawnjelly
Copy link
Member

Preliminary this looks good to me. 👍

I haven't yet had time to try it out rigorously and look at the meshes produced (hopefully I will be able to in the next few days). I'm assuming they should be good as they are defaults of the libraries, so it is more the user friendliness aspects that others can confirm or not by trying. I agree having a measure of control of simplification per mesh would be good, but this is good start to get the ball rolling and have some of this functionality available.

@pouleyKetchoupp
Copy link
Contributor Author

pouleyKetchoupp commented Jul 9, 2021

@jitspoe "convcolonly" uses decomposition into multiple convex hulls, so this PR doesn't affect it. But after this is merged, I wouldn't mind adding more keywords to support the other modes: maybe something like "singleconvcolonly" for the cleaned version and "simpleconvcolonly" for the simplified version? I would keep decomposition as default to avoid unnecessary compatibility breaking.

Edit: See issue #29971 and related discussion in #40622 about this topic. At the time of that PR, @reduz had some reservation because of the risk to create overly complex geometries from QuickHull, but now that we have a better algorithm to clean/simplify the convex hull it makes more sense.

@lawnjelly
Copy link
Member

lawnjelly commented Jul 10, 2021

Just looking over again, one difference is that the VHACD convex_decompose() seems to return a list of faces presumably with duplicate points, and the other convex_hull function presumably lists non-duplicate points that are references by indices. Does this matter? Should we be removing duplicates as a post process after convex_decompose()?

EDIT: It looks like convex_decompose in vhacd register_types.cpp has access to the indices, but for some reason the convention to pass the data it is sent longhand as faces with repeated vertices. Rather than attempt to de-duplicate on the other side, could it be an idea to change the interface for this convex_compose function so that it passes vertices and indices?

Maybe it is to try and deal with there being possibly multiple meshes returned and possibly multiple vertex lists (this would involve returning e.g. some structure per decomposed mesh, with the vertices and indices). Even if this is the case an alternative might be to put in an extra function to deal with the single mesh case.

@pouleyKetchoupp
Copy link
Contributor Author

@lawnjelly Good point! convex_decompose() actually already deals with duplicate vertices here:

for (int i = 0; i < decomposed.size(); i++) {
Set<Vector3> points;
for (int j = 0; j < decomposed[i].size(); j++) {
points.insert(decomposed[i][j].vertex[0]);
points.insert(decomposed[i][j].vertex[1]);
points.insert(decomposed[i][j].vertex[2]);
}

But that doesn't seem the best way to handle the process indeed. Passing vertices and indices in the vhacd interface like you suggest sounds great and it seems to work well. I don't actually see a difference in the results for the cases I've tested, so it looks like the current process is doing the job to clean things up at least for common cases. On the other hand your solution would be more optimized as we could avoid the conversion from faces to vertices before vhacd, and from vertices to faces to vertices again after vhacd (since we just need points in the end).

So it does seem like the way to go. I'll open a separate PR after this one is merged to avoid mixing things together though.

@akien-mga akien-mga merged commit fc00a83 into godotengine:master Jul 12, 2021
@akien-mga
Copy link
Member

Thanks!

@pouleyKetchoupp pouleyKetchoupp deleted the convex-hull-simplification branch July 12, 2021 22:21
@jitspoe
Copy link
Contributor

jitspoe commented Jul 13, 2021

@jitspoe "convcolonly" uses decomposition into multiple convex hulls, so this PR doesn't affect it. But after this is merged, I wouldn't mind adding more keywords to support the other modes: maybe something like "singleconvcolonly" for the cleaned version and "simpleconvcolonly" for the simplified version? I would keep decomposition as default to avoid unnecessary compatibility breaking.

Edit: See issue #29971 and related discussion in #40622 about this topic. At the time of that PR, @reduz had some reservation because of the risk to create overly complex geometries from QuickHull, but now that we have a better algorithm to clean/simplify the convex hull it makes more sense.

Ah, right, it USED to use quickhull (up until 3.2?). I actually modified my branch to use quickhull again, because VHACD messed up my collisions and was very problematic for a precision platformer. I still feel that if I'm explicitly making my own collision objects in blender, they should be transferred 1:1 to the engine.

@lawnjelly
Copy link
Member

Ah, right, it USED to use quickhull (up until 3.2?). I actually modified my branch to use quickhull again, because VHACD messed up my collisions and was very problematic for a precision platformer.

I totally get this - it is one of the nuances of convex hulls, small differences using different algorithms, or duplicate vertices can produce vastly different results.

I still feel that if I'm explicitly making my own collision objects in blender, they should be transferred 1:1 to the engine.

In order to transform results 1:1 from blender we would need to export something more akin to our Geometry::MeshData structure, rather than just a mesh (although even our MeshData structure isn't ideal because it doesn't specify triangulation for faces).

I.e. The data should contain a list of planes. As is, we have to rely on back calculating from a mesh, and the triangulation may be vague. See here for an example of the types of problems this can introduce - it's a minefield:
#50282 (comment)

So we basically have to run a second convex hull algorithm on whatever we get from blender at the moment. This is nearly always 'lossy' (like jpg), especially where planes meet at glancing angles.

And VHACD seems especially lossy, it seems to sometimes bear little resemblance to the input mesh. It probably be improved quite a bit, but I guess the authors gave up because of the compounded difficulties of dealing with accuracy AND decomposition.

For the single mesh case VHACD can be improved quite a bit, probably even simply by combining the results of quick hull and VHACD (i.e. getting the strengths of each method but not the weakness). VHACD has a low threshold so is good for optimizing curves, but in turn it seems to suffer on corners.

I've written my own quickhull like library that does simplification and deals with both these problems (in a single mesh case) but I need a bit more time to iron out all the problems before we could consider using this. Alternatively the bullet library may well have options for simplification that we are simply not using, perhaps we are just using defaults.

@jitspoe
Copy link
Contributor

jitspoe commented Jul 14, 2021

In order to transform results 1:1 from blender we would need to export something more akin to our Geometry::MeshData structure, rather than just a mesh (although even our MeshData structure isn't ideal because it doesn't specify triangulation for faces).

All you need is the vertices. What I'm saying is, if I make a mesh, then create another simplified mesh (ex: box) with the name "Whatever-convcolonly", I would expect the collision in-engine to match that convex hull exactly. It's only lossy if you run something like VHACD, which is designed to simplify the mesh. It's fine to have that as an option, but I think the default behavior should be to run a non-lossy quickhull algothim. The only thing that should be lost is concave areas, if there are any.

@lawnjelly
Copy link
Member

All you need is the vertices.

In theory yes, but in practice, not so much, because the systems that actually use convex hulls, don't operate on vertices, they operate on planes (rooms and physics). So we need to get the planes from the vertices, which is one of the things that quickhull / bullet / VHACD do for us. All these algorithms are lossy in some way, afaik, due to tolerances and epsilons. (That is not to say you may get a lossless situation on some meshes.)

For instance in rooms & portals I made the tolerance for Quickhull passable as a defaulted parameter (default is the same as at the moment). This is used as one of the mechanisms to simplify meshes. The existing default value is usually good but be aware it will collapse some vertices that are not far from an existing plane. This works because Quickhull algorithm recursively splits faces of the hull to make it more refined. If a vertex is less than the tolerance from an existing face plane, it no longer splits. So having a large tolerance results in a more approximate hull, and a lower tolerance a more 'accurate' hull, but with more faces.

@jitspoe
Copy link
Contributor

jitspoe commented Jul 15, 2021

I guess the point I'm trying to make is that it shouldn't use VHACD by default because that's SUPER lossy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants