Add `near` and `far` check in intersectRay #658

agargaro · 2024-04-14T17:21:45Z

No description provided.

agargaro · 2024-04-14T18:03:15Z

src/core/utils/intersectUtils.js

-	if ( tmax < 0 ) return false;
-
-	return true;
+	return tmin <= far && tmax >= near; // TODO far and near should be scaled here


near and far should be multiplied by scale. In case the scaling is not uniform, we need to have 3 separate values of near and far according to the axes.
So near and far could become a Vector3 or a float32Array (to have a better memory location?) instead of a number.
Do you have any ideas in mind otherwise?

This should be validated further but I believe the math here works out such that if you scale the ray into the local space of the box being intersected against then the "distance" calculated here (tmin, tmax) is scaled based on the length of the ray. Ie tmin and tmax represent the number of ray.direction vectors needed to reach the surface. So this distance should be enough as-is.

Here's a modified version of the three.js intersects box function showing the above:

Code Block

function intersectBox( box, ray ) { let tmin, tmax, tymin, tymax, tzmin, tzmax; const invdirx = 1 / ray.direction.x, invdiry = 1 / ray.direction.y, invdirz = 1 / ray.direction.z; const origin = ray.origin; if ( invdirx >= 0 ) { tmin = ( box.min.x - origin.x ) * invdirx; tmax = ( box.max.x - origin.x ) * invdirx; } else { tmin = ( box.max.x - origin.x ) * invdirx; tmax = ( box.min.x - origin.x ) * invdirx; } if ( invdiry >= 0 ) { tymin = ( box.min.y - origin.y ) * invdiry; tymax = ( box.max.y - origin.y ) * invdiry; } else { tymin = ( box.max.y - origin.y ) * invdiry; tymax = ( box.min.y - origin.y ) * invdiry; } if ( ( tmin > tymax ) || ( tymin > tmax ) ) return null; if ( tymin > tmin || isNaN( tmin ) ) tmin = tymin; if ( tymax < tmax || isNaN( tmax ) ) tmax = tymax; if ( invdirz >= 0 ) { tzmin = ( box.min.z - origin.z ) * invdirz; tzmax = ( box.max.z - origin.z ) * invdirz; } else { tzmin = ( box.max.z - origin.z ) * invdirz; tzmax = ( box.min.z - origin.z ) * invdirz; } if ( ( tmin > tzmax ) || ( tzmin > tmax ) ) return null; if ( tzmin > tmin || tmin !== tmin ) tmin = tzmin; if ( tzmax < tmax || tmax !== tmax ) tmax = tzmax; //return point closest to the ray (positive side) if ( tmax < 0 ) return null; return tmin >= 0 ? tmin : tmax; } box = new THREE.Box3(); box.min.setScalar( - 1 ); box.max.setScalar( 1 ); ray = new THREE.Ray(); ray.origin.set( 0, 0, - 5 ); ray.direction.set( 0, 0, 1 ); console.log( 'distance:', intersectBox( box, ray ) ); // 4 ray.direction.set( 0, 0, 0.5 ); console.log( 'distance:', intersectBox( box, ray ) ); // 8

I have tried but this only works if the scale is uniform.

this.getWorldScale( worldScale ); ray.direction.divide( worldScale );

If worldScale is for example (1, 2, 1), dividing the ray.direction by worldScale would deform the ray, changing direction.

Or am I doing something wrong?

I see now that Ray.direction is supposed to be normalized and the Ray.applyMatrix4 function actually normalizes the ray rather than scaling it based on the matrix so my suggestion won't work.

Instead we can scale the near / far values based on how much the ray length would change which should work universally:

const tmp = new Vector3().copy( ray.direction ).applyMatrix4( tmpInverseMatrix ); const scaleFactor = tmp.length() / ray.direction.length(); raycaster.near *= scaleFactor; raycaster.far *= scaleFactor;

I think this should work. We're basically just scaling the near and far values based on how the ray would scale to the local space. This has turned out to be more complicated than I though. Probably why it wasn't done like this in the first place 😅 Thanks for digging in and checking everything.

There are still some problems... I will investigate later

Edit: Fixed :)

agargaro · 2024-04-14T18:04:12Z

src/core/cast/raycast.template.js

@@ -24,6 +24,8 @@ function _raycast( nodeIndex32, bvh, side, ray, intersects ) {

 		/* @if INDIRECT */

+		// It might happen that a triangle less distant than 'near' is selected here. Should we add the near check here too? is it worth it? It might slow down to handle really rare cases


It might happen that a triangle less distant than 'near' is selected here. Should we add the near check here too? is it worth it? It might slow down to handle really rare cases

The triangle hits will also need to be filtered so that the results returned by the raycast function are accurate. Otherwise its the case that we may have 10 triangles in a large leaf node such that the near hit point falls within the ray range but the triangles within do not.

We could filter them afterward but it probably makes sense to just not add them to the intersections array here.

It sounds like we can remove this line now, too?

gkjohnson · 2024-04-15T14:34:48Z

src/core/cast/raycast.template.js

@@ -24,6 +24,8 @@ function _raycast( nodeIndex32, bvh, side, ray, intersects ) {

 		/* @if INDIRECT */

+		// It might happen that a triangle less distant than 'near' is selected here. Should we add the near check here too? is it worth it? It might slow down to handle really rare cases


The triangle hits will also need to be filtered so that the results returned by the raycast function are accurate. Otherwise its the case that we may have 10 triangles in a large leaf node such that the near hit point falls within the ray range but the triangles within do not.

We could filter them afterward but it probably makes sense to just not add them to the intersections array here.

gkjohnson · 2024-04-15T14:47:14Z

src/core/utils/intersectUtils.js

-	if ( tmax < 0 ) return false;
-
-	return true;
+	return tmin <= far && tmax >= near; // TODO far and near should be scaled here


This should be validated further but I believe the math here works out such that if you scale the ray into the local space of the box being intersected against then the "distance" calculated here (tmin, tmax) is scaled based on the length of the ray. Ie tmin and tmax represent the number of ray.direction vectors needed to reach the surface. So this distance should be enough as-is.

Here's a modified version of the three.js intersects box function showing the above:

Code Block

function intersectBox( box, ray ) { let tmin, tmax, tymin, tymax, tzmin, tzmax; const invdirx = 1 / ray.direction.x, invdiry = 1 / ray.direction.y, invdirz = 1 / ray.direction.z; const origin = ray.origin; if ( invdirx >= 0 ) { tmin = ( box.min.x - origin.x ) * invdirx; tmax = ( box.max.x - origin.x ) * invdirx; } else { tmin = ( box.max.x - origin.x ) * invdirx; tmax = ( box.min.x - origin.x ) * invdirx; } if ( invdiry >= 0 ) { tymin = ( box.min.y - origin.y ) * invdiry; tymax = ( box.max.y - origin.y ) * invdiry; } else { tymin = ( box.max.y - origin.y ) * invdiry; tymax = ( box.min.y - origin.y ) * invdiry; } if ( ( tmin > tymax ) || ( tymin > tmax ) ) return null; if ( tymin > tmin || isNaN( tmin ) ) tmin = tymin; if ( tymax < tmax || isNaN( tmax ) ) tmax = tymax; if ( invdirz >= 0 ) { tzmin = ( box.min.z - origin.z ) * invdirz; tzmax = ( box.max.z - origin.z ) * invdirz; } else { tzmin = ( box.max.z - origin.z ) * invdirz; tzmax = ( box.min.z - origin.z ) * invdirz; } if ( ( tmin > tzmax ) || ( tzmin > tmax ) ) return null; if ( tzmin > tmin || tmin !== tmin ) tmin = tzmin; if ( tzmax < tmax || tmax !== tmax ) tmax = tzmax; //return point closest to the ray (positive side) if ( tmax < 0 ) return null; return tmin >= 0 ? tmin : tmax; } box = new THREE.Box3(); box.min.setScalar( - 1 ); box.max.setScalar( 1 ); ray = new THREE.Ray(); ray.origin.set( 0, 0, - 5 ); ray.direction.set( 0, 0, 1 ); console.log( 'distance:', intersectBox( box, ray ) ); // 4 ray.direction.set( 0, 0, 0.5 ); console.log( 'distance:', intersectBox( box, ray ) ); // 8

src/core/MeshBVH.js

gkjohnson · 2024-04-15T14:54:21Z

src/utils/GeometryRayIntersectUtilities.js

@@ -12,7 +12,7 @@ export function convertRaycastIntersect( hit, object, raycaster ) {
 	hit.distance = hit.point.distanceTo( raycaster.ray.origin );
 	hit.object = object;

-	if ( hit.distance < raycaster.near || hit.distance > raycaster.far ) {
+	if ( hit.distance < raycaster.near || hit.distance > raycaster.far ) { // TODO probably this can be removed


This can be removed if we can guarantee the function only returns triangle hits within the range of the near and far values. I don't recall if there are any yet but if not it would be nice to add a few tests that check to make sure the correct results are returned when the raycaster near and far are modified compared to the three.js results.

Lets go ahead and remove this now that we have tests!

src/core/utils/iterationUtils.template.js

src/core/cast/raycastFirst.template.js


 		/* @else */

-		return intersectClosestTri( bvh, side, ray, offset, count );
+		return intersectClosestTri( bvh, side, ray, offset, count, near, far );


agargaro · 2024-04-16T18:35:10Z

This PR became a little more complicated than expected.

I modified the raycast example, putting a mesh with non-uniform scale, and used it to do my tests.
It seems to working but I would still like to do more testing.

Should I add new tests?

gkjohnson

This looks great - just left a couple comments. It would also be great to add a couple tests to make sure we're returning the same results that three.js is.

Las thing to also decide is whether we should just allow for passing a raycaster in or add the new arguments 🤔. I'm leaning towards passing a raycaster in to avoid adding too many arguments to the raycast function signature.

gkjohnson · 2024-04-20T01:21:47Z

src/utils/ExtensionUtilities.js

+		this.getWorldScale( worldScale );
+		direction.copy( ray.direction ).multiply( worldScale );


The scale axes change based on the rotation of the model so we'll want to make sure we account for the rotations when scaling here, as well. I think this should be correct:

direction.copy( ray.direction ).applyMatrix4( this.matrixWorld ); // we divide by the ray.direction just in case it's non normalized const scaleFactor = direction.length() / ray.direction.length();

This still needs to be addressed, as well.

gkjohnson · 2024-04-20T01:25:07Z

src/core/utils/iterationUtils.template.js

-		intersection = intersectTri( geometry, side, ray, _indirectBuffer ? _indirectBuffer[ i ] : i );
+		intersection = intersectTri( geometry, side, ray, _indirectBuffer ? _indirectBuffer[ i ] : i, undefined, near, far );


nit: lets use null here instead of undefined.

gkjohnson · 2024-04-20T01:27:47Z

example/raycast.js


 			cylinderMesh.rotation.z = Math.PI / 2;

+			hitMesh.visible = res.length > 0;
+			origMesh.visible = res.length > 0;


Lets keep origMesh visible regardless of whether or not we see a hit. It's a little confusing to see those sphere flicker, I think

gkjohnson · 2024-04-20T01:32:56Z

src/core/utils/intersectUtils.js

-	if ( tmax < 0 ) return false;
-
-	return true;
+	return tmin <= far && tmax >= near;


Does this work correctly if the ray origin is within the box? I'm having trouble visualizing the logic here. It may be easiest to understand if we just return a distance here (null if no hit) and check the distance against near / far in the calling function.

Though I see this makes the other code a bit more messy. Maybe this is easier to understand:

const hitDist = tmin >= 0 ? tmin : tmax; return hitDist > near && hitDist < far;

I think this would be a good change to make to make thins more clearly with three.js' version that returns a this.at( tmin >= 0 ? tmin : tmax, target ) as the hit point

src/core/utils/iterationUtils.template.js

@@ -32,11 +32,11 @@
 		let intersection;
 		/* @if INDIRECT */

-		intersection = intersectTri( geometry, side, ray, _indirectBuffer ? _indirectBuffer[ i ] : i );
+		intersection = intersectTri( geometry, side, ray, _indirectBuffer ? _indirectBuffer[ i ] : i, null, near, far );


agargaro · 2024-05-01T08:36:32Z

Las thing to also decide is whether we should just allow for passing a raycaster in or add the new arguments 🤔. I'm leaning towards passing a raycaster in to avoid adding too many arguments to the raycast function signature.

To conform to your code style, which one do you prefer?

if ( rayOrRaycaster.ray !== undefined ) {
    // isRaycaster or isRay don't exist
    raycastFunc( this, i, materialSide, rayOrRaycaster.ray, intersects, rayOrRaycaster.near, rayOrRaycaster.far );
} else {
    raycastFunc( this, i, materialSide, rayOrRaycaster, intersects, 0, Infinity );
}

const isRaycaster = rayOrRaycaster.ray !== undefined;
const ray = isRaycaster ? rayOrRaycaster.ray : rayOrRaycaster;
const near = isRaycaster ? rayOrRaycaster.near : 0;
const far = isRaycaster ? rayOrRaycaster.far : Infinity;

raycastFunc( this, i, materialSide, ray, intersects, near, far );

let ray = rayOrRaycaster;
let near = 0;
let far = Infinity;

if ( rayOrRaycaster.ray !== undefined ) {
    ray = rayOrRaycaster.ray;
    near = rayOrRaycaster.near;
    far = rayOrRaycaster.far;
}

raycastFunc( this, i, materialSide, ray, intersects, near, far );

gkjohnson · 2024-05-03T02:32:07Z

To conform to your code style, which one do you prefer?

After thinking about it for a bit I think I've changed my mind 😅 Adding "near" and "far" to the public API is good for now. Perhaps in the future if we have to add too many arguments we can change it to something like an options object but we'll deal with that later. The API you've added is good, as is:

bvh.raycast( ray, material, near, far );

// and

bvh.raycastFirst( ray, material, near, far );

Once the rest of the feedback is addressed we can get this merged! Sorry for the indecision!

agargaro · 2024-05-03T12:44:34Z

Okay :) so now I'll add some tests.

agargaro · 2024-05-13T13:41:30Z

I added these tests:

describe( 'Random CENTER intersections with near', () => runRandomTests( { strategy: CENTER, near: 6 } ) );
describe( 'Random CENTER intersections with far', () => runRandomTests( { strategy: CENTER, far: 7 } ) );
describe( 'Random CENTER intersections with near and far', () => runRandomTests( { strategy: CENTER, near: 6, far: 7 } ) );

Would you rather have tests for near and far, for all strategies?

gkjohnson

Just looked through it all again - this is a great change! I noted some of the last few things that need to be cleaned up. If you'd like me to help out with some of those final changes I'm happy to make the commits.

Would you rather have tests for near and far, for all strategies?

I think the tests you added look great. Once the few other comments are addressed we can get this merged!

example/raycast.js

gkjohnson · 2024-05-14T10:03:23Z

src/core/cast/raycast.template.js

@@ -24,6 +24,8 @@ function _raycast( nodeIndex32, bvh, side, ray, intersects ) {

 		/* @if INDIRECT */

+		// It might happen that a triangle less distant than 'near' is selected here. Should we add the near check here too? is it worth it? It might slow down to handle really rare cases


It sounds like we can remove this line now, too?

gkjohnson · 2024-05-14T10:14:25Z

src/core/utils/intersectUtils.js

@@ -67,14 +67,12 @@ export function intersectRay( nodeIndex32, array, ray ) {

 	if ( ( tmin > tzmax ) || ( tzmin > tmax ) ) return false;

-	// if ( tzmin > tmin || tmin !== tmin ) tmin = tzmin; // Uncomment this line if add the distance check
+	if ( tzmin > tmin || tmin !== tmin ) tmin = tzmin;


Just noticed that this line is in the condition tmin !== tmin 😅. Likely some C-era method for checking for NaN I'd think. Might be worth fixing in three's implementation for the sake of clarify.

gkjohnson · 2024-05-14T10:16:17Z

src/core/utils/intersectUtils.js

-	if ( tmax < 0 ) return false;
-
-	return true;
+	return tmin <= far && tmax >= near;


I think this would be a good change to make to make thins more clearly with three.js' version that returns a this.at( tmin >= 0 ? tmin : tmax, target ) as the hit point

gkjohnson · 2024-05-14T10:18:46Z

src/utils/ExtensionUtilities.js

+		this.getWorldScale( worldScale );
+		direction.copy( ray.direction ).multiply( worldScale );


This still needs to be addressed, as well.

gkjohnson · 2024-05-14T10:19:03Z

src/utils/GeometryRayIntersectUtilities.js

@@ -12,7 +12,7 @@ export function convertRaycastIntersect( hit, object, raycaster ) {
 	hit.distance = hit.point.distanceTo( raycaster.ray.origin );
 	hit.object = object;

-	if ( hit.distance < raycaster.near || hit.distance > raycaster.far ) {
+	if ( hit.distance < raycaster.near || hit.distance > raycaster.far ) { // TODO probably this can be removed


Lets go ahead and remove this now that we have tests!

gkjohnson · 2024-05-15T10:02:02Z

Fixes #657

agargaro added 3 commits April 14, 2024 11:17

Add near and far check to bvh node ray traverse

0b05e62

Example updated

db3a65d

Add comments to review

3d00e02

agargaro marked this pull request as draft April 14, 2024 17:22

agargaro commented Apr 14, 2024

View reviewed changes

gkjohnson reviewed Apr 15, 2024

View reviewed changes

agargaro added 2 commits April 15, 2024 18:52

Scaled ray

eefa65e

Add scaledFactor and add near and far check for triangles

3ab2009

github-advanced-security bot found potential problems Apr 16, 2024

View reviewed changes

Fix

17b7868

gkjohnson reviewed Apr 20, 2024

View reviewed changes

Replaced 'undefined' with 'null'

36eb4d1

github-advanced-security bot found potential problems Apr 29, 2024

View reviewed changes

Add tests

8758246

gkjohnson reviewed May 14, 2024

View reviewed changes

Removed useless code

62fde15

agargaro marked this pull request as ready for review May 14, 2024 13:09

gkjohnson merged commit aa63f76 into gkjohnson:master May 15, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `near` and `far` check in intersectRay #658

Add `near` and `far` check in intersectRay #658

agargaro commented Apr 14, 2024

agargaro Apr 14, 2024 •

edited

gkjohnson Apr 15, 2024

agargaro Apr 16, 2024 •

edited

gkjohnson Apr 16, 2024

agargaro Apr 16, 2024 •

edited

agargaro Apr 14, 2024

gkjohnson Apr 15, 2024

gkjohnson May 14, 2024

gkjohnson Apr 15, 2024

gkjohnson Apr 15, 2024

gkjohnson Apr 15, 2024

gkjohnson May 14, 2024

agargaro commented Apr 16, 2024 •

edited

gkjohnson left a comment

gkjohnson Apr 20, 2024

gkjohnson May 14, 2024

gkjohnson Apr 20, 2024

gkjohnson Apr 20, 2024

gkjohnson Apr 20, 2024

gkjohnson Apr 20, 2024

gkjohnson May 14, 2024

agargaro commented May 1, 2024 •

edited

gkjohnson commented May 3, 2024 •

edited

agargaro commented May 3, 2024

agargaro commented May 13, 2024

gkjohnson left a comment

gkjohnson May 14, 2024

gkjohnson May 14, 2024

gkjohnson May 14, 2024

gkjohnson May 14, 2024

gkjohnson May 14, 2024

gkjohnson commented May 15, 2024

		@@ -24,6 +24,8 @@ function _raycast( nodeIndex32, bvh, side, ray, intersects ) {

		/* @if INDIRECT */

		// It might happen that a triangle less distant than 'near' is selected here. Should we add the near check here too? is it worth it? It might slow down to handle really rare cases

		this.getWorldScale( worldScale );
		direction.copy( ray.direction ).multiply( worldScale );

		intersection = intersectTri( geometry, side, ray, _indirectBuffer ? _indirectBuffer[ i ] : i );
		intersection = intersectTri( geometry, side, ray, _indirectBuffer ? _indirectBuffer[ i ] : i, undefined, near, far );

Add near and far check in intersectRay #658

Add near and far check in intersectRay #658

Conversation

agargaro commented Apr 14, 2024

agargaro Apr 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agargaro Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agargaro Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agargaro commented Apr 16, 2024 • edited

gkjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agargaro commented May 1, 2024 • edited

gkjohnson commented May 3, 2024 • edited

agargaro commented May 3, 2024

agargaro commented May 13, 2024

gkjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkjohnson commented May 15, 2024

Add `near` and `far` check in intersectRay #658

Add `near` and `far` check in intersectRay #658

agargaro Apr 14, 2024 •

edited

agargaro Apr 16, 2024 •

edited

agargaro Apr 16, 2024 •

edited

agargaro commented Apr 16, 2024 •

edited

agargaro commented May 1, 2024 •

edited

gkjohnson commented May 3, 2024 •

edited