Cast rays towards specific instances of an instanced mesh.

[mikesol]

Description

Raycasting against instanced meshes is currently an expensive operation when there are many instances but we only want to check for a few intersections. This adds the ability to raycast against a specific set of instances.

[Mugen87]

A InstancedMesh specific method in Raycaster seems not right to me. Raycaster should not be aware about specific 3D objects.

[mikesol]

I can move this to another place. Is there a good place to put "util" like functions?

[Mugen87]

TBH, I think the signature of intersectInstancedMesh() is problematic since apps do usually not know the instance IDs which are required for an intersection test. Hence, it's better to implement intersectInstancedMesh() at application level.

The average raycasting performance of InstancedMesh can be improved in another way. The first step for this is #21507. That would allow proper view frustum culling and the new bounding volumes could be honored in InstancedMesh.raycast(). The bounding volumes of the single instances are already used by the way.

[mikesol]

Thanks for the feedback!

If I were to move this logic out of Raycaster.js to something with application logic, where would the right place be? Would I be right in thinking that it'd go in something like https://github.com/mrdoob/three.js/tree/dev/examples/js/raycasting/InstancedRaycast.js?

[mikesol]

I can move this to another place. Is there a good place to put "util" like functions?

[Mugen87]

Actually I was suggesting to not add this code to the library but use it as a part of your custom code.

[mikesol]

Lemme see if that's possible - I'm currently working with a forked version of three with the code in this PR, but if I can extract it out 100% to the application level then I'll close the PR. Otherwise I'll let you know the bits that would need to be exposed.

[mikesol]

I tested it out in my custom code & getting raycastInstances to work required extending InstanceMesh and adding the method. As the method is a copy/paste of raycast with some minor modifications, this feels like it'd be better as library than application code.

Would it make sense to slim down the PR to only have raycastInstances and the example/doc without the change to Raycaster.js? The only downside there is that raycast in InstancedMesh is currently undocumented, which leads me to believe that it's part of an internal API, so perhaps this wouldn't be a good fix. nvm it is documented here.

[Mugen87]

TBH, I'm not in favor of having a raycasting method that selectively performs tests based on a bunch of instance IDs.

The problem is how are users find out the appropriate range of instance IDs for an intersection test. This might be clear for your specific use case but not for others. I think it's better to not provide an API that requires inputs which are hard to determine.

[mikesol]

The problem is how are users find out the appropriate range instance IDs for an intersection test. This might be clear for your specific use case but not for others. I think it's better to not provide an API that requires inputs which are hard to determine.

In my case, I'm developing a rhythm game where each tile is an instanced mesh, and different tiles become touchable based on different states in the game, but only a small subset are ever touchable at one time. So the instance IDs used for raycasting are used in the same places as setMatrix4 and setColor.

I'm very new to three I don't have a good sense of the culture of how the API evolves, but as this change is not breaking & it accommodates a use case that's common from my experience in rhythm game development, I hope it or a variation of it can make it in the code base.

That said, I have a working example now that uses my own subclass, so all is good atm. But the subclass is 90% copy/paste from InstancedMesh.js, so it's pretty fragile.

[gkjohnson]

I'm very new to three I don't have a good sense of the culture of how the API evolves, but as this change is not breaking & it accommodates a use case that's common from my experience in rhythm game development, I hope it or a variation of it can make it in the code base.

It's very common for people to request that changes be made to core three.js code that could be handled at the app level. In this case if you know which instances you care to check it's very possible to do so yourself with no changes to three by extracting the matrixWorld of the given instance id, applying it to a regular THREE.Mesh instance, and raycasting against it.

[arpu]

@gkjohnson is there a example we could look at it, best with three-mesh-bvh

[mikesol]

I'm very new to three I don't have a good sense of the culture of how the API evolves, but as this change is not breaking & it accommodates a use case that's common from my experience in rhythm game development, I hope it or a variation of it can make it in the code base.

It's very common for people to request that changes be made to core three.js code that could be handled at the app level. In this case if you know which instances you care to check it's very possible to do so yourself with no changes to three by extracting the matrixWorld of the given instance id, applying it to a regular THREE.Mesh instance, and raycasting against it.

👍 this is how I'm currently handing it, and it's also what the current implementation of raycast does in InstancedMesh.

In this PR, it's a <10 loc change to implement InstancedMesh-s raycast as a special case of raycastInstance. The alternative is copying raycast into a subclass and making the changes there. I don't know the threejs internals well enough to know how often they change, but I'm worried that the copy/paste approach will break or degrade my application if I update three and the internal implementation of raycast changes.

I definitely understand the reluctance to add stuff to the lib that can be accomplished on the application level. Here, my gut feeling is that the tradeoff is acceptable, but as I said above, I don't have a good sense in this project of what makes the cut for changing core vs not.

[Mugen87]

Sorry, I think we should not add this code to the lib. The solution explained by @gkjohnson in #24171 (comment) sounds like the better approach for this use case. At least for now.

Nevertheless, thanks for making the PR.