search

CesiumGS / cesium-native

Apache License 2.0
432 stars 212 forks source link

Add triangle/ray and gltf/ray intersect function with tests #777

Closed joseph-kaile closed 3 months ago

joseph-kaile commented 10 months ago

Added two new utility functions, rayTriangle and intersectRayGltfModel. They have unit tests to verify their correctness.

This PR can really be thought of as two distinct efforts:

javagl commented 8 months ago
csciguy8 commented 4 months ago

Opening this up for review (non-draft).

All previous comments should have been addressed and any known major functionality is done.

I may add some more commits from self-review later on, but it's ready for some eyes on it.

joseph-kaile commented 4 months ago

Opening this up for review (non-draft).

All previous comments should have been addressed and any known major functionality is done.

I may add some more commits from self-review later on, but it's ready for some eyes on it.

Hello team,

I (and ChatGPT) noticed an edge case not handled. In the ray-OBB code, halfAxes could be singular. See the comment in the OBB constructor. I would recommend rewriting it to not use halfAxis.

Thanks

kring commented 4 months ago

Hi @joseph-kaile, good to hear from you, hope you're doing well! Thanks for the note about the edge case. Are you referring to IntersectionTests::rayOBBParametric specifically?

joseph-kaile commented 4 months ago

Hi @joseph-kaile, good to hear from you, hope you're doing well! Thanks for the note about the edge case. Are you referring to IntersectionTests::rayOBBParametric specifically?

I’m doing well, thanks. Yes, I’m referring to that function.

I found this out recently after intersecting the large root tiles of CWT.

kring commented 4 months ago

I found this out recently after intersecting the large root tiles of CWT.

Ok, cool. It's not obvious to me why the CWT root tile OBB half axes would be singular. But we should probably handle that possibility in the intersect function in any case.

javagl commented 4 months ago

The question about the handling of (near) singular matrices is essentially leaked through from an old, unaddressed TODO. My understanding is that this is undefined behavior and a crash waiting to happen.

But... one could argue whether this should be addressed in this PR or ... more holistically. Yes, the glm::inverse function brutally divides by (potentially) zero. The fact that theOrientedBoundingBox constructor is declared noexcept therefore raises a bunch of questions, somewhat related to https://github.com/CesiumGS/cesium-native/issues/348 and the comment in the glm docs that I already linked to. Still, the code in this PR just relied on existing functionality. Nobody can expect someone who uses the OrientedBoundingBox to look into the constructor code and see whether there might be an open TODO (of which nobody knows how to resolve it).

(But of course, all that is not on me to decide...)

csciguy8 commented 3 months ago

Still, the code in this PR just relied on existing functionality. Nobody can expect someone who uses the OrientedBoundingBox to look into the constructor code and see whether there might be an open TODO (of which nobody knows how to resolve it).

I'm thinking we should make this a separate issue, and not handle it for this PR.

I do have opinions of course (like letting the OBB constructor code throw), but this feels like something to be discussed outside of ray intersection tests.

csciguy8 commented 3 months ago

@kring, ready for another look. All comments have been resolved or commented on.

csciguy8 commented 3 months ago

@kring Review changes committed. Ready again for another look.

csciguy8 commented 3 months ago

@kring Ready for one more pass!

kring commented 3 months ago

Looks good, thanks! I'll merge as soon as CI passes.