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

Ray-casts do not support face_index #702

Closed
mihe opened this issue Dec 1, 2023 · 17 comments · Fixed by #955
Closed

Ray-casts do not support face_index #702

mihe opened this issue Dec 1, 2023 · 17 comments · Fixed by #955
Labels
enhancement New feature or request topic:runtime Concerning runtime behavior (or its source code)

Comments

@mihe
Copy link
Contributor

mihe commented Dec 1, 2023

This extension does not currently support the face_index field found in the result dictionary of intersect_ray, as seen in its documentation.

This was something introduced in Godot 4.2 (godotengine/godot#71233) which lets you get the index of any face/triangle of a ConcavePolygonShape3D that the ray-cast hit.

While Jolt does have support for getting the vertices of a particular face, there's no public interface for getting the actual triangle index of it. Even if there was, the process of "indexifying" the triangles that happens as part of creating Jolt's JPH::MeshShape means that any index you return from Jolt won't map to the actual list of faces originally set on the ConcavePolygonShape3D anyway, which this face_index thing relies entirely upon.

This is unlikely to ever be supported by this extension in its current form.

@github-actions github-actions bot added the needs triage Something that needs investigation label Dec 1, 2023
@mihe mihe added enhancement New feature or request topic:runtime Concerning runtime behavior (or its source code) external Involving an external party and removed needs triage Something that needs investigation labels Dec 1, 2023
@MarioBossReal
Copy link

MarioBossReal commented Apr 7, 2024

I'm wondering if this feature is still not planned to be supported, since this is an important feature to have especially if you have a surface property index per mesh triangle.

Even if there was, the process of "indexifying" the triangles that happens as part of creating Jolt's JPH::MeshShape means that any index you return from Jolt won't map to the actual list of faces originally set on the ConcavePolygonShape3D anyway, which this face_index thing relies entirely upon.

Assuming that the actual vertex count/positions remain the same, would it not be possible to create a mapping between the original vertices and the indexed vertices? I don't really see how the input mesh could differ from the (optimised?) mesh apart from re-indexed vertices.

The triangle index of a godot physics mesh is (roughly, this function probably has issues) int(index/3), since the whole mesh is just a 1D buffer of vertices. I think the easiest solution would be to create a new buffer alongside the vertex buffer (when it gets indexified) that stores the original face index (or vertex index if its easier) from the godot mesh, indexed ofcourse by the same index buffer. Then getting the original face would be as simple as FaceBuffer[index].

Actually that probably wouldnt work since the vertex buffer could be any size. Wouldn't the offset value of the current index correspond to the original index though? As far as I understand, if a mesh has n vertices (before indexing), there has to be n indices after it gets indexified. Even though the value stored at Index[i] corresponds to a position from a shrunken vertex buffer, the actual position [i] should match the original vertex's index so long as no weird swapping/reordering etc occurs?

@mihe
Copy link
Contributor Author

mihe commented Apr 7, 2024

so long as no weird swapping/reordering etc occurs?

While I believe Jolt's indexing currently preserves the same order as the input list of triangles (which is an assumption I would be hesitant about relying on since it could presumably change at any time) the thing that does throw a wrench into this is the fact that Jolt culls degenerate faces, which will obviously offset all subsequent indices.

It might be that I could do the culling of degenerate faces on my end, and hang on to their indices in order to undo any such offset, but that's frankly making a lot of assumptions about Jolt's indexing, which again could presumably change at any point.

In any case, this is moot so long as Jolt doesn't hang on to the actual triangle index in its MeshShape, which to me seems quite fundamental to its design not to do. Its SubShapeID (which is provided as part of its RayCastResult) is already meant to serve the purpose of getting things like the face normal as well as any per-face material index, but this ID doesn't really map to the input geometry in any way.

The only fix I can see, without changing how this works on the Godot side of things, would be to fork/change Jolt and either repurpose/extend its existing 5-bit Triangle::mMaterialIndex (which is unused by this extension) to hold the triangle index instead, or adding another index alongside that's similarly preserved through the indexing process. Either one strikes me as quite a scary (and potentially costly) change though, and is not something I'm eager to do right now.

The change I've had in mind is to instead make a PR to Godot to add the face vertices as part of its ray-cast results. While this would largely make face_index redundant for the purposes it was originally meant for, it obviously wouldn't do much to help your situation if you're keeping some list of per-face materials separately on your end.

@MarioBossReal
Copy link

Its SubShapeID (which is provided as part of its RayCastResult) is already meant to serve the purpose of getting things like the face normal as well as any per-face material index, but this ID doesn't really map to the input geometry in any way.

Would it make sense or be possible to have custom data channels within the physics mesh that get baked alongside the indexing process, similar to how ArrayMeshes have various buffers like position, color, custom0/1/2 etc? This might be more of a general Godot PR type deal rather than jolt specifically. Are face normals for physics meshes baked into a buffer or are they calculated at runtime?

The change I've had in mind is to instead make a PR to Godot to add the face vertices as part of its ray-cast results. While this would largely make face_index redundant for the purposes it was originally meant for, it obviously wouldn't do much to help your situation if you're keeping some list of per-face materials separately on your end.

Do you mean only returning the position values of the vertices, or the actual indices themselves which could be used to index into various buffers? There's a bit of a gap here because as far as I know Godot physics meshes aren't indexed and don't really store any data besides position data. Maybe behind the scenes Godot bakes some data like face normals etc but I'm not sure.

I think what this problem is bringing up however is a solid reason to be able to store different channels in physics meshes just as you would in a render mesh. If returning a face index which corresponds to the input mesh isn't possible, I think that's okay as long as there is a way to add in more data, which is something that Godot itself will have to support.

@mihe
Copy link
Contributor Author

mihe commented Apr 8, 2024

Would it make sense or be possible to have custom data channels within the physics mesh that get baked alongside the indexing process, similar to how ArrayMeshes have various buffers like position, color, custom0/1/2 etc? This might be more of a general Godot PR type deal rather than jolt specifically.

It doesn't really matter what you add on the Godot side of things. The only per-face properties I can provide as part of the ray-cast results is the normal, vertex positions and material index (which again is not used by this extension). Anything else I will have no choice but to leave at its default value.

Are face normals for physics meshes baked into a buffer or are they calculated at runtime?

They're calculated at runtime.

(triangle_idx seen there is not in fact the triangle index, but an index into the acceleration structure used by Jolt.)

Do you mean only returning the position values of the vertices, or the actual indices themselves which could be used to index into various buffers?

Only the position values, yes.

There's a bit of a gap here because as far as I know Godot physics meshes aren't indexed and don't really store any data besides position data. Maybe behind the scenes Godot bakes some data like face normals etc but I'm not sure.

They do calculate it behind the scenes, yes.

@MarioBossReal
Copy link

Thanks for your replies. I guess it's probably much more effort than it's worth at this stage to support getting (godot) face indices. For now I've decided to just split up my multi-material physics meshes into separate meshes by collecting all tris with the same surface property index and building a new mesh shape out of them. It seems to work well, and might even improve performance a little by cutting up very large map meshes into smaller ones.

@jrouwe
Copy link
Contributor

jrouwe commented Aug 18, 2024

@mihe Since we're working through the list of differences between godot physics and godot jolt now, I think there's something we can do about this:

  • Add a uint32 mUserData to Triangle
  • In JoltConcavePolygonShapeImpl3D::_build you would set Triangle::mUserData = face_index
  • Indexify will copy mUserData to IndexedTriangle which will also get uint32 mUserData
  • MeshShapeSettings gets a new property Array<std::pair<SubShapeID, uint32>> *mSubShapeIDToUserData = nullptr
  • In MeshShape::MeshShape when mSubShapeIDToUserData != nullptr we will push all sub shape ID/user data pairs into the array
  • Degenerate triangles that have been removed will not show up in this map
  • You would need to keep a separate map in JoltConcavePolygonShapeImpl3D to run-time map from SubShapeID to face_index

What do you say?

@mihe
Copy link
Contributor Author

mihe commented Aug 18, 2024

  • MeshShapeSettings gets a new property Array<std::pair<SubShapeID, uint32>> *mSubShapeIDToUserData = nullptr
  • In MeshShape::MeshShape when mSubShapeIDToUserData != nullptr we will push all sub shape ID/user data pairs into the array

I guess that would all largely become opt-in then, even from a memory standpoint, meaning it could potentially be put behind a setting for anyone not wanting to pay the cost of this?

@jrouwe
Copy link
Contributor

jrouwe commented Aug 18, 2024

Yes, it would be opt-in. The resulting MeshShape won't change.

@mihe
Copy link
Contributor Author

mihe commented Aug 19, 2024

I think that sounds pretty reasonable.

@jrouwe
Copy link
Contributor

jrouwe commented Aug 21, 2024

I changed my mind a little:

https://github.com/jrouwe/JoltPhysics/tree/feature/per_triangle_user_data

This simply provides a MeshShape::GetTriangleUserData(SubShapeID) when you set MeshShapeSettings::mPerTriangleUserData = true.

What do you think?

(note code untested)

@mihe
Copy link
Contributor Author

mihe commented Aug 21, 2024

I would say that looks much nicer from an API perspective, and obviously lines up better with the existing methods, like GetSurfaceNormal and whatnot.

I'm surprised/happy to see you were able to still make it opt-in without too much hassle.

@jrouwe
Copy link
Contributor

jrouwe commented Aug 22, 2024

I think this should be it:

jrouwe/JoltPhysics#1225

Beware that it costs up to 25% extra memory per mesh, so you may want to put this behind another project flag.

@mihe
Copy link
Contributor Author

mihe commented Aug 22, 2024

Awesome. I'm working on it as we speak.

Beware that it costs up to 25% extra memory per mesh, so you may want to put this behind another project flag.

Yes, most definitely.

@mihe
Copy link
Contributor Author

mihe commented Aug 22, 2024

Looks to be working well enough (#955) but I've only done some very basic testing so far.

The whole GetLeafShape dance and dealing with SubShapeID remainders does feel a little bit clunky, now that I'm actually using it. I'm not sure what a better alternative would look like though. In my mind it was more akin to Body::GetWorldSpaceSurfaceNormal, where it's just some method on the body that I give a SubShapeID and get a uint32 back, but obviously GetWorldSpaceSurfaceNormal is applicable to all shapes, whereas this only makes sense on MeshShape.

Would it be too out-of-place to have some Body::GetTriangleUserData that deals with all the GetShape()->GetLeafShape(...) and remainders and whatnot, and takes a uint32 &outUserData and returns a bool as to whether it was able to actually find a MeshShape?

It's not a big deal or anything, but I figured you might appreciate the feedback.

@jrouwe
Copy link
Contributor

jrouwe commented Aug 23, 2024

Thanks for the feedback! I think having a Body::GetTriangleUserData is a bit too specific. This new GetLeafShape function has more uses than just finding the user data of the triangle. Godot for example currently doesn't seem to return a face_index for GodotHeightMapShape3D, but with this you can detect that the SubShapeID points to a HeightFieldShape and return e.g. face_index = 2 * (x + width * y) + triangle_in_quad_index (I'll expose HeightFieldShape::DecodeSubShapeID). Same if you were to create a custom VoxelShape and wanted to know which voxel was hit. I think there are more use cases where you'd actually want to get to the leaf shape of the body you hit (maybe you want to know the radius of the sphere you hit).

@mihe
Copy link
Contributor Author

mihe commented Aug 23, 2024

Fair enough! That all sounds reasonable to me.

@mihe mihe removed the external Involving an external party label Aug 23, 2024
@mihe mihe closed this as completed in #955 Aug 24, 2024
@mihe
Copy link
Contributor Author

mihe commented Nov 3, 2024

To anyone who might still be subscribing to this issue, support for face_index can now be found in the newly released 0.14.0-stable release.

Note that the release on Godot Asset Library is still pending approval, so you might need to download it from GitHub for the time being.

Also note that you have to explicitly enable support for it, by enabling the physics/jolt_3d/queries/enable_ray_cast_face_index project setting, since it comes at a non-trivial memory cost, as talked about above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request topic:runtime Concerning runtime behavior (or its source code)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants