-
Notifications
You must be signed in to change notification settings - Fork 193
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
Add triangle/ray and gltf/ray intersect function with tests #777
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just one comment. There are also some GCC / Clang warnings that need to be fixed before merging.
* backfaces or not. Defaults to true. | ||
* @param return The intersection point along the ray, if any. | ||
*/ | ||
static std::optional<glm::dvec3> intersectRayGltfModel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return more information here? It would be useful to know the mesh, primitive, and triangle that was hit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iff (if and only iff) the barycentric coordinates fall out of the intersection test, that could also be useful.
(If not, they can be computed post-hoc where necessary, but ... if my dusty memories are not misleading me, I think that this is something that comes "(nearly) for free" for some implementations of ray/triangle intersection tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case that I thought was useful was if you have a hit, and needed traverse back into the model to do more.
For example, I hit something, and have a hit point, but want to know the entire triangle I hit because I want to show it to the user. Similarly, you hit something, and want to show the whole mesh that was hit.
I've made changes to this function to return more info (HitResult
struct) and modified the tests to check that we can dive into the model with this info after the fact.
|
Still more work to do in the implementation...
Also add the source python that created them
…ices, for clarity
@@ -971,6 +970,11 @@ void intersectRayScenePrimitive( | |||
if (!pPositionAccessor) | |||
return; | |||
|
|||
// Ignore non-float positions, these are unsupported | |||
// Although valid from the glTF spec, they aren't generally useful here | |||
if (pPositionAccessor->componentType != Accessor::ComponentType::FLOAT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the table at https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#meshes-overview, the POSITION
accessor must use VEC3/float. But 1. it can still make sense to have this check here (and maybe even log this as an error instead of "doing nothing"), and 2. maybe there are extensions that relax this constraint (like the (pending!) extension at KhronosGroup/glTF#2397 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! I missed that part of the spec. I'll update the comments with your notes.
It could be nice to have some warnings logged somewhere when traversing a gltf with bogus values. There are other checks like this that "do nothing" and try our best when encountering bad data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... unless the glTF is using KHR_mesh_quantization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll add a note about that extension, and see how hard it would be to extend this PR to cover it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lilleyse That was one reason why I mentioned extensions (although I might have been more explicit)
(I would have to refresh my memory about the AccessorView
- I don't know whether this already decodes quantized data - i.e. where exactly the quantization extension is handled - but being aware of that point might be enough...)
…uld produce incorrect results Also unify the ::intersectRayGltfModel and ::intersectRayGltfModelParametric functions. We don't really need two anymore
Added two new utility functions,
rayTriangle
andintersectRayGltfModel
. They have unit tests to verify their correctness.