Pass indices to trimesh collision shapes; optimize CSG collision

[rburing]

In Godot Physics the trimesh collision shape (ConcavePolygonShape3D) always had the ability to store triangles by storing vertices and (triples of) indices (into the list of vertices), but this ability was only ever used like this:

godot/servers/physics_3d/godot_shape_3d.cpp

Lines 1631 to 1633 in a05670c

	facesw[i].indices[0] = i * 3 + 0;
	facesw[i].indices[1] = i * 3 + 1;
	facesw[i].indices[2] = i * 3 + 2;

By adding the ability to pass custom indices, we also gain the ability to run the mesh optimizer (via the static method SurfaceTool::strip_mesh_arrays) in the editor (on import, or when generating a collision shape from a mesh). This has the positive effect of eliminating degenerate triangles, which are known to cause collision issues, see e.g. #47214 (comment). This PR applies that optimization to CSG collision shapes; it could also be exposed (as an option) to meshes and the mesh importer.

This also paves the way to fixing #69683 (for ConcavePolygonShape3D) via the algorithm described in Contact generation for meshes by Pierre Terdiman.

Breaks compatibility very slightly, only on the physics server level: the format of shape_get_data / shape_set_data changed from a dictionary with a "faces" key (PackedVector3Array value) to a dictionary with a "vertices" key (PackedVector3Array value) and an "indices" key (PackedInt32Array value).

Supersedes #47214.

Production edit: closes godotengine/internal-team-priorities#54

[rburing]

It may be desirable to keep the index data the same as in the original mesh, so I could remove the automatic optimization and add a warning for degenerate triangles instead, as suggested in #47214 (review).

[rburing]

We could also make indices optional; that would be less compatibility-breaking.

Edit: But this requires distinguishing "no indices" from "no triangles" somehow, which seems annoying, considering the ordering of setter calls (e.g. when loading resources). If you set the vertices, should it start creating faces assuming there are no indices? That would be a waste of work if there are indices that will be set shortly after. The alternative of requiring to set indices first would be a weird API.

[Calinou]

we also gain the ability to run the mesh optimizer (via the static method SurfaceTool::strip_mesh_arrays) in the editor (on import, or when generating a collision shape from a mesh).

This will help implement godotengine/godot-proposals#3603 🙂

meshoptimizer is available in exported projects if I'm not mistaken, so this should be exposable as a function you can call at runtime too.

[rburing]

I've removed the calls to SurfaceTool::strip_mesh_arrays from Mesh::create_trimesh_shape() and ImporterMesh::create_trimesh_shape() to respect the principle of least surprise. It could be made optional or exposed differently (or I could put the calls back if desired).

The internal trimesh collision shape generated for CSG shapes is still optimized using SurfaceTool::strip_mesh_arrays.

[rburing]

@mihe @fabriceci Could you comment on this API for trimesh collision shapes?

In the current version I have the indices always being saved (e.g. to disk), even if they are [0, 1, 2, 3, 4, 5, ..., n]. I'd like to avoid this (to avoid wasting space), but it's not so easy given the two separate setters for vertices and indices.

[mihe]

By "this API" I assume you're referring to the notion of using indexed triangles for ConcavePolygonShape3D?

If so, I'm all for it. This is something Jolt more or less requires for its MeshShape, which I use as the backing shape for ConcavePolygonShape3D in Godot Jolt. In fact, Jolt currently spends time "indexifying"/deduplicating the vertices, as seen here, since I'm not passing any indices to it.

[clayjohn]

Is there any way for this to maintain compatibility? Or at least provide an intuitive error for users?

Perhaps the returned dictionary could provide a faces key, but hidden behind a DISABLED_DEPRECATED flag?

[rburing]

@mihe I was referring more to the implementation detail mentioned in the second half of my comment. I'd really like to avoid saving indices = [0, 1, 2, 3, 4, 5, ..., n] in .tscn files (and in memory), and let indices = [] signify this instead. It's tricky given the possible orders of calls to setters for vertices and indices. Do you have an idea?

It is interesting that Godot-Jolt does automatic deduplication and welding of vertices. It's definitely good for performance, and can prevent bugs in some physics algorithms (at least in Godot Physics). In Godot itself this could be done in Mesh::create_trimesh_shape() and ImporterMesh::create_trimesh_shape(). In the first version of this PR I did add deduplication+"indexifying" (via SurfaceTool::strip_mesh_arrays) there, but I thought there could be a use case for the collision shape's vertex/face indices matching the original mesh's vertex/face indices exactly. (Face indices are planned to be exposed as collision data in #71233.) For that reason I thought it might be better to make it optional (perhaps enabled by default). What do you think?

@clayjohn For compatibility (for those users using PhysicsServer3D.shape_get_data) I could indeed add the (redundant) faces key back to the shape's data dictionary, hidden behind #ifndef DISABLE_DEPRECATED (at a performance cost, for those users using the function). That's the only way not to break compatibility.

[mihe]

I'd really like to avoid saving indices = [0, 1, 2, 3, 4, 5, ..., n] in .tscn files (and in memory)

Judging by your list of indices and the name of strip_mesh_arrays I'm guessing this list of indices is meant to represent triangle strips? If so, then this will not work with Jolt, as it expects a regular triangle list. From a quick look at PhysX and Bullet that seems to hold true there as well.

Assuming I'm not misunderstand this, and you're willing to consider triangle lists instead, then any notion of optimizing away the indices becomes moot I guess, since triangle lists would be much less predictable in anything but the simplest of cases. Although, would it even have made sense with triangle strips? Would the meshes really be a single continuous strip? I must admit I haven't used triangle strips much ever.

I thought there could be a use case for the collision shape's vertex/face indices matching the original mesh's vertex/face indices exactly. [...] For that reason I thought it might be better to make it optional (perhaps enabled by default).

Through what mechanism would you toggle such a setting? Things like the "Create Trimesh Static Body" menu option don't seem to offer much in terms of configuration. I guess the ConcavePolygonShape3D could store the non-optimized vertices/indices and this would instead happen at runtime, but that seems a bit wasteful/expensive.

[mihe]

If so, then this will not work with Jolt, as it expects a regular triangle list.

Thinking about it some more, I guess converting from strips to lists would be fairly straight-forward, and strips would lead to more space-efficient indices. You'd likely need to marshal the indices/vertices over to whatever third-party engine one-by-one anyway, due to potential differences in size or winding order, so doing a slight conversion along with it isn't all that bad I guess.

I'm assuming then that this optimization for the case of [0, 1, 2, 3, 4, 5, ..., n] would only apply when there aren't any breaks (presumably using degenerate triangles) in the strip? If so, I can't say I have any good answers for the problem of the setter call ordering, sadly. I don't find it particularly upsetting to just write/store the indices as-is though, even if they're perfectly sequential.