Fix AABB Ray intersection - return inside

[lawnjelly]

• Separates find_intersects from test_intersects for rays, and wraps the former.
• Changes parameter name to "r_intersection_point".
• Fixes broken old version which returned per axis t.
• Returns whether the ray origin is within the AABB.
• Returns intersection point when origin outside.
• Returns "backtracking" intersection point when inside.
• Returns sensible normal when inside.
• Returns valid results on borders.
• Returns robust results dealing with floating point error.
• Added some extra unit tests for borders and inside behaviour.

Alternative to #83138

Notes

Needs looking over but seems to work in tests so far (it is possible there is some case I have missed).

This adds a few extra things that weren't dealt with in @Kimau 's original PR. Am equally happy to have them integrated in her original PR, just easier to make an alternative and add as co-author. Some of these changes will doubtless be bikeshedded too.

On suggestion from @AThousandShips I've separated the find intersection function from the test intersection. (I haven't changed much about the segment test, because it appears to have worked before so it would be more likely to be breaking compatibility in e.g. third party code).

Returns whether the ray origin is within, in combination with returning a "backtracking" intersection point. This has the advantage of allowing the caller either to use the ray origin as the intersection point (which is afaik what happens with #83138 ) but also allows backtracking to a border in the case of float error and slightly overshooting an AABB. This also ensures the normal is sensible for the backtracked intersection point, allowing reflection / sliding etc.

Border behaviour is improved, with correct intersection point / normal (this was broken in #83138).

Intersection point coordinates on the colliding axis are copied directly from the AABB. This ensures robust behaviour and eliminates the possibility of float error on the border axis (as before we are multiplying t by the ray direction and relying on this to line up exactly).

I also noticed and fixed a bug here:
(*r_normal)[axis] = p_dir[axis] ? -1 : 1;
Float when converted to bool returns false only in the case of zero, and returns true for negative values. Whereas here we want to return the opposite normal when the direction is negative:
(*r_normal)[axis] = p_dir[axis] >= 0 ? -1 : 1;

[Kimau]

Thanks for taking the time to address these bits. There is a reason I was using the ray intersect over the segment version and that was performance. Most game engines don't account for the edge cases as Ray v AABB is a low level op which often needs to be as performant as possible.

I made a quick godbolt here to show the amount of extra work even -O2 opt
https://godbolt.org/z/nnK3s6n3e

And a quickbench here will show you the perf difference, about 1.4x difference.
https://quick-bench.com/q/WNphX51fCE7EUpmOTr0IItfHqew

While these micro-optimisations are often pushed back on in a core math lib I do feel its an important place to have this discussion. Maybe making another version of the function if you want to perform additional checks, which is rare I found.

[Kimau]

Apoligies didn't account for nullptr branches. Updated the quickbench: https://quick-bench.com/q/cgLYWatzc4Oy0xWhp9DYg-0eDSQ

Yeah I fully withdraw the perf argument with nullptr on inside it works out to the same code once opt. Though its worth thinking about the gdscript bindings.

[AThousandShips]

(Your benchmark link doesn't include the test used for the image, it misses a case, also not sure if it matters but we compile with c++17)

Also AFAIK on production builds we run O3, and for that setup the new method is faster in the first setup, and equivalent in the second, and on debug builds I'd say the performance here is irrelevant compared to other performance aspects

Correction, with O3 and correctly avoiding optimization of certain values it yields more stable yet less drastic differences:
https://quick-bench.com/q/MXbaOJ_gfWgvWqBJjJqF-_ZbPl8

In this case the version used in GDScript, namely IntersectsRayPos is comparable with the FindIntersectsRayInsidePos, which are the ciritical here (O3 and O2 respectively)

So regardless of the actual importance of performance here, it isn't a critical difference (1.1 times slower for this version in O2, equivalent in O3, that is for the bound versions)

[lawnjelly]

Just to clarify, I'm fully a performance guy (I'm just updating my SIMD library right now). However, we must recognise that Godot in general prioritizes robustness / user friendliness / stability over performance, especially in bound functions. This can be seen throughout the code base. This took me a little time to accept when I first started contributing.

The policy is to make things "correct" first and foremost, and then for bottlenecks to write highly optimized versions which can let conditions slide / less error checking etc (usually local to the calling code, Juan has written some articles on this).

On the other hand often we can deal with robustness / edge cases without sacrificing much in the case of performance. In this particular case for performance, I would be suspecting multiple tests in a single function call could be better, as the overheads of the function call (particularly when bound) will probably be pretty big and helps the compiler, plus the MATH_CHECKS etc. I don't know your particular use case, if you can clarify this may help.

[Kimau]

I updated my branch, you want to fix the conflict on here @lawnjelly, don't have write access to this one

[lawnjelly]

Rebased. 👍

[lawnjelly]

Rebased again, and made minor change, r_inside is now a reference rather than a pointer.

r_inside is pretty cheap to calculate and checking the pointer is extra work, and the dummy inside should hopefully be compiled out when called from intersects_ray().

[AThousandShips]

Haven't tested extensively but the code and tests look good to me and the approach seems good

[clayjohn]

Looks good. Let's move forward with this!

[akien-mga]

Thanks to everyone involved!