gkjohnson / three-mesh-bvh

A BVH implementation to speed up raycasting and enable spatial queries against three.js meshes.
https://gkjohnson.github.io/three-mesh-bvh/example/bundle/raycast.html
MIT License
2.38k stars 247 forks source link

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

Closed agargaro closed 1 month ago

agargaro commented 2 months ago

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?

agargaro commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

agargaro commented 1 month ago

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 commented 1 month ago

Fixes #657