Fix AABB Ray Intersection

[Kimau]

The old function was incorrect returning a per axis t Tested this and it works correctly returning same result as intersects_segment

[AThousandShips]

Style for CI

[lawnjelly]

I wondered how this had not been noticed (if incorrect) and it looks like none of the calls from core in 3.x and 4.x use the returned clip (what's a clip when it's at home? intersection point?) or normal. 🙂

Needs double checking this doesn't alter the test result with the change to t_min (or possibly any places that relied on incorrect previous result). Doesn't look like it was called from a lot of places: TriangleMesh::intersect_ray() and CSGBrushOperation::MeshMerge().

EDIT: Actually on reflection, perhaps the name "clip" indicates that it isn't designed to return the intersection point? And that the function is doing what it designed to do, but the error is in the documentation for the bound function? Especially bearing in mind most of these type of tests will have been copy pasted from elsewhere (really ideally there would be comment links to the source).

Then the question arises whether the "clip" is a useful value anyway, and is sensible to replace with what most of us would expect (intersection point).

I think this is the most likely explanation for the discrepancy, rather than an "error". Given that the "clip" is not used anywhere in core, and the bound documentation refers to intersection point, it does seem to make sense to change.

UPDATE: The previous r_clip looks to be the near value on each axis. I'm not sure it is useful.

[Kimau]

Ammended for whitespace and comment into sentance case with . on end

[lawnjelly]

I'm short on time right now, but can you let us know why this figure should be changed to 0.0 (just in case this introduces float error)? A quick google suggested using -INFINITY, +INFINITY as before:

https://tavianator.com/2011/ray_box.html

Did you write this or copy from a source? It would be helpful to link to the source article in a comment, or at least provide reasoning for the change here.

(If you treat reviewers as knowing little it won't be far from the truth - I have done slab test before but I simply haven't time to refamiliarise myself this week. 😁 )

[Rindbee]

As the case where the component is equal to 0 is handled separately, I think this change makes sense.

case 0	case 1	case 2

Less than 0 means far away from the point (pink line segment), and greater than 0 means close to the point (green line segment).

It is possible for the intersection point to fall on the green line segment (include the parallel segment in the rect), but it is impossible to fall on the pink line segment (include the parallel segment in the rect).

[lawnjelly]

Ah good explanation @Rindbee that makes sense. 👍

However this does brings up another issue, that calling code may in a lot of cases want to handle the start point inside the box as a special case. It would have been neater if the function returned not just whether it intersects, but also whether the start point is inside the box (being a special case), as this now might require a separate test.

Maybe we are stuck with this for backward compatibility.

[Rindbee]

I'm not sure if on aabb box (vertex/edge/face) is a special case (it seems to be considered as inside), but it's special enough if the direction is parallel to some axis.

[Kimau]

It's changed to a zero because you don't want to negativly cast the ray. Worse case your ray origin is inside AABB and you stop. I've written these tests tons of times with more than 15 years in the game industry. The main book I use for ref is Real-Time Collision Detection by Christer Ericson if you need to cross check it.

[lawnjelly]

Again here, if you can clarify why removing the || (far < 0) is needed that would be useful for reviewing.

(isn't this one of the rejection cases? If the far is below 0, then the intersection with both planes is behind the ray origin?)

EDIT: Ah I see, it may be that with near constrained to 0.0, that near > far effectively catches the far < 0 case.

[Kimau]

see above

[lawnjelly]

If tmin is initialized to 0.0, if the start point is on the exact edge, and it moves towards the box, will this if fail, and thus not store the axis for the returned normal? 🤔

Could >= be better?

[Kimau]

no because first case wins and you want to avoid extra ops where possible this code should be VERY performant. As rays vs boxes is pretty common. The nasty case of being on the corner of a box which happens more often than you realise is degenerate and your best to return the first result asap

[lawnjelly]

This code fails with incorrect normal:

	// Test AABB code
	AABB aabb;
	aabb.size = Vector3(1, 1, 1);
	
	Vector3 pt, normal;
	if (aabb.intersects_ray_test(Vector3(0.5, 0.0, 0.5), Vector3(0, 1, 0), &pt, &normal))
	{
		print_line("intersect " + String(Variant(pt)));
		print_line("normal " + String(Variant(normal)));
	}

intersect (0.5, 0, 0.5)
normal (1, 0, 0)

This is likely due to the axis check failing as I indicated above.

Mostly it doesn't look bad though (I'm no expert), I took a look at my old libraries for this and I appear to use something adapted from Pete Shirley's http://psgraphics.blogspot.com/2016/02/new-simple-ray-box-test-from-andrew.html (with 0.0 for near) at some point. But I've usually only used this for intersection testing rather than finding intersection point, as far as I remember.

I'm have to admit I'm usually a fan of copying a standard debugged algorithm because there tend to be so many special cases with intersection tests and also float error. 😁

Ah here's the page I was looking for, always good refs:
https://www.realtimerendering.com/intersections.html

[Kimau]

Again as stated above my goto ref when I need it is Real-Time Collision Detection by Christer Ericson. He is stand up guy who knows his shit. Look this review should be simple extra stuff like missing unit tests aside there was just a bug in that t was being return instead of intersection point.

The collision normal for a ray inside the aabb is a degenerate case. One could argue that in the case that t is zero the normal should be inverse the ray but that might push you back to the middle of the box which is often undersiarble in collision response. Otherwise you could go away from center but in the degenerate case where origin matches center you get into trouble and need to place more checks which again clogs up what is meant to be a pretty simple function.

This is not a comlpex review.

[adamscott]

@Kimau First of all, thanks for your interest in Godot and for your first PR!

I think some context is needed here. This function was pretty much left untouched since the release of the Godot Engine 10 years ago. (hence why I asked @reduz for a review).

@lawnjelly and @Rindbee are doing their best to review your PR and to mitigate any regressions that could be caused by the change.

[...] He is stand up guy who knows his shit. Look this review should be simple extra stuff like missing unit tests aside there was just a bug in that t was being return instead of intersection point.
[...]
This is not a comlpex review.

This is not constructive.

Godot is one of the biggest projects on GitHub as we are receiving more than 800 PRs a month.

Considering the following points, I think your apparent frustration is misplaced:

• the code you intend to replace is 10 years old;
• your PR is less than 10 days old;
• you've received active feedback from two maintainers;
• the milestone is set to 4.2, which means that it will receive extra attention as it is considered for the next release in November.

To cite Godot's Code of conduct's expectations, I highlighted some parts in bold characters:

• Politeness is expected at all times. Be kind and courteous.
• [...]
• Feedback is always welcome but keep your criticism constructive. We encourage you to open discussions, proposals, issues, and bug reports. Use the community platforms to discuss improvements, not to vent out frustration. Similarly, when other users offer you feedback please accept it gracefully.

[Kimau]

• I should have remained kind and courteous. I apologize.

• I understand Godot is a large project, and I did not expect a quick turnaround.

• My annoyance was related to the amount of feedback and constant iteration on a relatively small piece of code, as it was very time-consuming.

• I understand math code can make people nervous, especially when it's 10-year-old code.

• This is quantifiably a small change, as it could have been expressed in one line.

• The code is not called anywhere else in Godot, and the docs clearly state the intended behavior.

As a first-time contributor, the requirement of compiling Godot, learning the unit test system, and doing a large amount of due diligence on a small change like this, while receiving tiny bits of feedback that diverged on minor issues like variable naming, was an extremely discouraging and negative experience.

I will strive to improve my interactions in the future, and I apologize for my communication style that was unkind and unfair. I won't try to excuse it. For now I'm going to just step away from this PR and turn off notifications for a while.

Thanks again,
Claire

[AThousandShips]

First of all I want to apologize for any pressure you might have felt to complete any changes in any deadline, I did not intend and pressure nor did any other team member here I'm sure, but communication in the text medium is hard. So I'm sorry

And I hope you can get some rest and peace

And I'm sorry that the process has been unpleasant for you, it can be hard to estimate the scale of a change or how involved the process is in any new environment you enter, and I'm sorry the process caused you frustration.

However it's important to be aware that you have to communicate if something feels bad or wrong, assuming that your feelings about a situation are known without communicating them is a recipe for clashes

Again, I'm sorry that this experience has been unpleasant for you, and hope that things can get better in the future

[lawnjelly]

This is unfortunately one of those PRs that ended up needing a bit more discussion than first appeared, so wasn't an ideal experience for a first PR.

Looking back at the history this bound function appears to have been broken for at least 10 years. Juan is credited with the binding in 3.x, but he can't remember that far back unsurprisingly 😁 , so we have to use a bit of detective work.

Given that the function operated incorrectly before, we actually have the opportunity to break compatibility at this point - so need to discuss whether the API for the bound function / c++ version was sensible in the first place, particularly regarding ray inside the box or near edge, particularly for tunnelling. This is the kind of thing we often discuss in e.g. PR meetings / team meetings / proposal how best to proceed.

[Kimau]

This is the main fix and the only real line that matters.
The rest is unit tests and naming discussion

[Kimau]

Just resolving merge issue

[akien-mga]

Superseded by #86755. Thanks for your contribution!