Remove duplicate code between intersects_ray and intersects_segment

[Flarkk]

In AABB, Plane, Geometry3D [Edit : and TriangleMesh] functions ray_intersects_* and segment_intersects_* involve almost identical code. In some cases both flavors even diverged over time which complexifies maintenance.

This PR :

• extends ray_intersects_* with an additional parameter distance that clamps the raycast in the same fashion as segment_intersects_*. distance is defaulted to 0.0 which enables infinite raycasting
• makes segment_intersects_* wrap calls to ray_intersects_*. It avoids duplication and opens the way to eventually deprecating intersects_segment_* in the future, as they suffer from numerical precision issues (see related issue Fix BVH::ray_query() numerical precision #100475)
• adds the Geometry3D::ray_intersects_* counterparts to segment_intersects_sphere, _cylinder and _convex
• enhances tests of AABB and Geometry3D to validate the above changes

[Flarkk]

@lawnjelly here the old functions now wrap a call to the new ones as you suggested in #100475. I believe it's helpful here.

[Flarkk]

Resolved requested changes. Thanks @AThousandShips for your torough review.

[lawnjelly]

Are we now paying the price for this check even when not used? (p_max_distance defaulted to 0?)

I can see the p_max_distance > 0 should be optimized out but does the second term tmin > p_max_distance?

(It's a while since I looked at this so can't remember. If it doesn't BTW, then templating the function can be handy for things like that, pass a bool template parameter of whether to do the extra check, and it will compile 2 separate versions with and without the check.)

[Flarkk]

Are we now paying the price for this check even when not used? (p_max_distance defaulted to 0?)

I can see the p_max_distance > 0 should be optimized out but does the second term tmin > p_max_distance?

Yes we are. I believe nothing can be optimized out right now because the code is in the .cpp.
Will consider templating the function, unless just moving it to the header file allows the compiler to optimize it down.

[lawnjelly]

Afaik compiler can optimize out functions in the cpp if called from the same translation unit (i.e. it has the opportunity to inline different versions of called functions), putting stuff in the header allows stuff to be optimized from a different translation unit. (Link time optimization can theoretically also remove this limit but may not be as smart as in the same translation unit.)

Afaik usually this should mean wrapper functions in the same header / cpp will aggressively optimize.

Always worth checking the disassembly to see what is going on, or e.g. profile release version and watch the hot spots.

[lawnjelly]

In some cases both flavors even diverged over time which complexifies maintenance.

Agree, but it does mean that any changes to behaviour could cause knock on bugs. I haven't looked in detail but are there any changes to behaviour here, or is it purely moving code around?

[Flarkk]

Added TriangleMesh to the list of simplifications.

[Flarkk]

Pushed some small changes in relation to the below.

Agree, but it does mean that any changes to behaviour could cause knock on bugs. I haven't looked in detail but are there any changes to behaviour here, or is it purely moving code around?

Generally speaking the intention is to preserve the exact original behavior.
Most changes just consist in wrapping calls and replacing all to - from by direction and distance, except some subtleties with edge cases.

TLDR :

• AABB : some differences in edge cases
• Plane : same behavior
• Geometry3D : same behavior except 1 difference with Sphere intersection
• TriangleMesh : same behavior

Unfold details

---

AABB

Original segment and ray intersection implementations are based on distinct algorithms.
With this PR, segment intersection now runs on ray's code, which has identical behavior except on few edge cases :

• Ray starting point inside the AABB : ray intersection returns the entry point before the ray starting point when the latter is located inside the box, where segment intersection returns the ray starting point itself. A consequence of it is segment returns unspecified normals for ray starting points inside the box.
  ℹ️ This PR preserves these differences in the segment version, except the normal is now set to Vector3() when the starting point is inside the box
• Zero-length segments : when ray start and end are strictly identical segment version behaves like a point intersection (true if inside, false otherwise).
  ✔️ This PR preserves this behavior by falling back to has_point() when necessary.
• Exact intersection coordinates : ray version ensures returned intersection coordinates exactly lie on the box edge to compensate for numerical precision roundings, when segment doesn't.
  ℹ️ This PR harmonizes the behavior and applies this correction in all cases
• AABBs larger than 1e20 : original ray version have indefined results with such very large boxes whereas segment manages them correctly.
  ✔️ This PR fixes ray version behavior such that very large boxes are managed correctly in all cases

Plane

No edge case. Strictly indentical implementation.

Geometry3D

Triangle intersection

No edge case. Strictly indentical implementation.

Sphere intersection

• Zero-length segments : original segment implementation consider start and end points identical and return false below EPSILON.
  ℹ️ This PR changes this for an exact 0 length check to keep it harmonized with all other intersection functions.

Cylinder and Convex intersection

A subpart of the Cylinder's algorithm has been rewritten to get rid of (to - from), however the new behavior is strictly identical.

• Objects larger than 1e20 : original segment implementations of both Cylinder and Convex have indefined results with very large objects.
  ✔️ This PR fixes it to support any size

TriangleMesh

• Mesh with vertices farther than 1e10 : both original ray and segment implementations have indefined results with such very large meshes.
  ✔️ This PR fixes it to support any mesh size

---

But overall I agree this kind of change can have unintended side effects if not toroughly tested.
Unit tests are good but usually not enough.

I'd like to set up a demo project or something visual to allow stress-testing these changes.
It's not that simple because it's core classes and they're not directly accessible from scripts.
I didn't think through it yet but if you have any idea, let me know.

[Flarkk]

Also, I realized that defaulting distance to 0.0 to signify infinite raycasting doesn't allow testing against a point (i.e. a zero-length segment). It's yet another edge case, but the from / to versions do support it.

I'm not fully clear on whether it's a real problem. Any views welcome.

Alternatives could be :

• to default to -1.0 instead
• to keep it as is but add is_point_inside() functions in all classes to allow testing against points too
• in all cases this is better documented : what's the standard practice (except commenting on the code) to document internal classes ?