Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raycaster: add ability to stop raycast traversal #27709

Merged
merged 6 commits into from
Apr 27, 2024

Conversation

AlaricBaraou
Copy link
Contributor

@AlaricBaraou AlaricBaraou commented Feb 8, 2024

Fixed #27702

Description

When using set of data amounting to thousands of objects and hundreds or thousands of meshes. Without any custom implementation even a single raycast can take an extremely long time.
The use case for this is well described in the issue.

This solution add a method to the Raycaster that allows for the ability to stop raycast traversal.
By calling Raycaster.stopTraversal() from inside one of the raycast, it'll terminate the Raycast traversal.

According to this perf test ( that isn't consistent, might need a better test ) there is no downside for the regular usage and performance of the current Raycast.

This contribution is funded by Alaric Baraou ✌

Copy link

github-actions bot commented Feb 8, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
674.4 kB (167.1 kB) 674.4 kB (167.1 kB) +21 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
453.8 kB (109.6 kB) 453.8 kB (109.6 kB) +0 B

src/core/Raycaster.js Fixed Show fixed Hide fixed
src/core/Raycaster.js Fixed Show fixed Hide fixed
@gkjohnson
Copy link
Collaborator

Thanks for taking a look at this!

The implementation in this PR doesn't look exactly right, though. Currently at the first encounter of any raycast that terminates traversal it will stop all raycast traversal instead of just stopping at that branch. When an object terminates raycast traversal the siblings should still be checked. I believe adding the check in the function intersectObject( object, raycaster, intersects, recursive ) would be correct.

And then error throwing approach is interesting - I was thinking something more like event.preventDefault paradigm so the rest of the function can still run after calling the function (ie set a field on raycaster and check it after the raycast function). I might wait for @mrdoob and / or @Mugen87 to give feedback on the approach before putting too much time in. But to that end this is what using this function would look like in user-land:

class TilesGroup extends Group {

  // ... set up

  raycast( raycaster, intersects ) {

    raycaster.stopTraversal();

    // ... perform raycasting traversal of children

  }
}

@AlaricBaraou
Copy link
Contributor Author

AlaricBaraou commented Feb 8, 2024

When an object terminates raycast traversal the siblings should still be checked.

I think there is two use cases then.
The one I made so far indeed stops checking all raycasts.
If the scene tree has been "ordered" in a specific way, doesn't it make sense to have the option to stop everything on first hit ?
Like a scene where you don't have a hierarchy and no overlapping mesh for instance.

If so I guess you would have two methods like.
raycaster.stopRaycast(); to stop all raycasts
and
raycaster.stopTraversal(); to stop for the current branch while continuing to check on the siblings.

@AlaricBaraou
Copy link
Contributor Author

I limited the PR to the current suggestion of @gkjohnson

I can make ajustements depending on feedbacks etc.

@gkjohnson
Copy link
Collaborator

I think there is two use cases then.
The one I made so far indeed stops checking all raycasts.
If the scene tree has been "ordered" in a specific way, doesn't it make sense to have the option to stop everything on first hit?
Like a scene where you don't have a hierarchy and no overlapping mesh for instance.

This seems like a very narrow use case - I think it's best to wait until we see requests for something like this before adding it.

I limited the PR to the current suggestion of @gkjohnson

Great! I think it would still be nice to design this in a way that lets the remaining code run after calling raycaster.stopTraversal() but we can wait for feedback from others.

@AlaricBaraou
Copy link
Contributor Author

I agree on both point.
Then for a design that lets the remaining code run after calling raycaster.stopTraversal() with minimal impact on the perf could be something like.

// new method of Raycaster that updates the _recursive
stopTraversal() {
	this._recursive = false;
}

And function intersectObject could become

function intersectObject( object, raycaster, intersects, recursive ) {

        //reset _recursive on each iteration
	raycaster._recursive = recursive;
	
	if ( object.layers.test( raycaster.layers ) ) {
		object.raycast( raycaster, intersects );
	}
	
	// usual check for raycaster._recursive 
	//+ check if it didn't change within object.raycast() from a user call to raycaster.stopTraversal()
	if ( raycaster._recursive === true ) {
		const children = object.children;
		for ( let i = 0, l = children.length; i < l; i ++ ) {
			intersectObject( children[ i ], raycaster, intersects, true );
		}
	}
}

this change wouldn't affect the performance for non empty raycast as the change is neglectable
Link to test of 10000 "raycast"

it would affect the perf when doing a ton of raycast with no-op but one of the goal of this PR is to avoid traversing tons of no-op raycast so I guess it's not an issue
Link to test of 10000 "no-op raycast"

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 9, 2024

If the scene tree has been "ordered" in a specific way, doesn't it make sense to have the option to stop everything on first hit ?

This could a separate feature. Unreal supports this in their Traces API as well (see section Returning Single or Multiple Hits):

https://docs.unrealengine.com/5.3/en-US/traces-in-unreal-engine---overview/

AFAICS, three.js only supports the "multiple hits" option right now .

The discussion in #27702 made me have a closer look on what Unreal offers in this area. It would be interesting to know how you would achieve the intended behavior of this PR (stop traversal of ray casting) in Unreal (or Unity) so we can easier compare solutions.

@IRobot1 Can you provide us additional insight in this question?

@AlaricBaraou Can we implement stopTraversal() without relying on a custom exception? TBH, this feels a bit like a misusage.

@gkjohnson I'm not fully understand how the intended usage of Raycaster.stopTraversal(); would look like. Would it always be required by the app to overwrite the raycast() method of 3D objects like demonstrated in #27709 (comment)?

Would a pure configurable solution be thinkable? Meaning a definition that tells the raycaster when to stop doing intersection tests.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Feb 9, 2024

https://docs.unrealengine.com/5.3/en-US/traces-in-unreal-engine---overview/
The discussion in #27702 made me have a closer look on what Unreal offers in this area.

The "Block" flag is for basically just returning the first hit from a raycast and is a bit different than what I'm asking for here. That feature is closer to the "firstHitOnly" flag used in both 3dTilesRenderer and three-mesh-bvh as an optimization for raycasting the subhierarchy and stoping when we know we've found the first intersection. The feature in this PR is about allowing a parent node to handle the raycasting of its children in a custom way - ie take advantage of a spatial hierarchy that's baked into a loaded data set.

It would be interesting to know how you would achieve the intended behavior of this PR (stop traversal of ray casting) in Unreal (or Unity) so we can easier compare solutions.

Unity (and likely Unreal) both automatically store and manage everything in a spatial hierarchy in the background so it's likely there'd be no need for this kind of feature since raycasting is fundamentally handled differently via the physics system.

@gkjohnson I'm not fully understand how the intended usage of Raycaster.stopTraversal(); would look like. Would it always be required by the app to overwrite the raycast() method of 3D objects like demonstrated in #27709 (comment)?

It would require overwriting raycast. The "stopTraversal" option was one approach but another option I listed in the issue was a flag on Object3D to denote that the node is a raycast leaf. Something like so:

const group = new Group();
// ... add children
group.raycast = () => { ... custom intersect children... };
group.raycastLeaf = true;
scene.add( group );
// (in practice this would best be an extended class)

raycaster.intersectObjects( scene );

After thinking about it a bit more I think I may prefer this method since it allows for the code to know procedurally whether an object is a raycast leaf before raycasting or not. This can be useful when trying to gather all nodes to cast against and sort them as an optimization, for example.

Would a pure configurable solution be thinkable? Meaning a definition that tells the raycaster when to stop doing intersection tests.

Yup! See above.

@AlaricBaraou
Copy link
Contributor Author

AlaricBaraou commented Feb 10, 2024

Can we implement stopTraversal() without relying on a custom exception? TBH, this feels a bit like a misusage.

yes I didn't commit it but that's what I made in this comment #27709 (comment)

@gkjohnson

another option I listed in the issue was a flag on Object3D to denote that the node is a raycast leaf.

A concern with this approach is the complexity that arises in scenarios involving multiple raycasts within a single scene. Each raycast could have its distinct hierarchy, where an Object3D might act as a raycast leaf for one raycast but not necessarily for another.
Just mentionning in case it matters for your current use case, don't want to overcomplicate it.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 10, 2024

Regarding #27702 (comment).

When I understand the issue correctly, you can already achieve the same result with layers but it won't stop the raycaster from traversing the children. And that is a performance issue for certain use cases.

Instead of adding a property to Object3D, couldn't we add a new property to Raycaster that controls if a failed layers test excludes descendant nodes? Meaning when the layer test fails, recursive is set to false.

I have the feeling something like Object3D.raycastLeaf is a bit use case specific and a solution in Raycaster would better encapsulate things. However, one disadvantage of this approach is that you can only apply a per-raycaster setting and can't configure on object level like with layers. Would that be an issue for your use cases?

@IRobot1
Copy link

IRobot1 commented Feb 10, 2024

I tried the idea of modifying raycaster to support firstHitOnly. I changed the intersectObject function to return false as soon as the intersection array length is >0

function intersectObject(object, raycaster, intersects, recursive, firstHitOnly) {

 if (object.layers.test(raycaster.layers)) {

  object.raycast(raycaster, intersects);

  if (firstHitOnly && intersects.length > 0)
   return false;

 }

 if (recursive === true) {

  const children = object.children;

  for (let i = 0, l = children.length; i < l; i++) {

   if (!intersectObject(children[i], raycaster, intersects, true, firstHitOnly))
    break;

  }

 }

 return true;
}

In the traversal case, this will help. But for raycast instanced mesh example, it doesn't help, since the hit check is done in object.raycast. I think this is what @gkjohnson is looking to change.

I think any raycaster hit behavior options should be added as a parameter to constructor

constructor(origin, direction, near = 0, far = Infinity, behavior = {}) {

firstHitOnly is an obvious behavior, but maxHits might be more flexible. When set to 1, it has the same effect as firstHitOnly. There might be situations where two hits makes sense to have.

Recursive is really a type of behavior, but hard to change for compatibility reasons.

If the desired behavior options are in raycaster, then the only change is in all the object.raycast methods to support the possible behaviors.

BTW, is there a reason function intersectObject is not part of the raycaster class? I think a better name is checkObjectIntersect and adding to the class. This way, it can be overridden/replaced with a custom intersection method if needed. For example, one that ignores child objects when the parent is hidden

@IRobot1
Copy link

IRobot1 commented Feb 10, 2024

Well that doesn't help. The problem is that you don't know the closest ray until after all the possible intersections are sorted. Stopping on first hit during traversal likely won't hit the closest object. Sorting the objects before raycasting is really the only way to resolve this.

Each raycast method does both the bounding sphere check and the hit detection. If these could be broken into separate phases

  1. compute if bounding sphere is hit and the distance
  2. sort distance to bounding spheres
  3. computer the hits for closest or all spheres

Raycasting could potentially be much faster.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Feb 12, 2024

@IRobot1 performing first hit raycast is a separate topic. This is about flagging objects to disable further recursing during raycasting so they can handle child intersections in a custom way without doubling the hits.

@Mugen87

Instead of adding a property to Object3D, couldn't we add a new property to Raycaster that controls if a failed layers test excludes descendant nodes? Meaning when the layer test fails, recursive is set to false.
...
However, one disadvantage of this approach is that you can only apply a per-raycaster setting and can't configure on object level like with layers. Would that be an issue for your use cases?

Can you describe what this would look like?

One issue with layers is that there are no reserved layers or settings for anything of these things. In the Unreal case there are reserved flags for the different raycast settings. In three.js even using layers to disallow raycasting on a subset of objects while still allowing them to render is convoluted. To do this the user must:

  • pick a layer to set as the "do raycast" layer (eg 2)
  • adjust the raycaster layer mask to 1 << 2
  • explicitly make sure that all objects in the scene that you do want to raycast have that layer set.

So even if you just want one object to skip raycasting you have to modify the layers for everything else in the scene. It's actually significantly easier and less invasive to just set the raycast function of the object you want to skip to a no-op instead of messing with layers.

If there were reserved layers for certain functionality (ie layer 2 is "ignore raycast" as Unity supports) then this would work better, though still a bit strange. Layers feels like an odd place for this, in my opinion.

I have the feeling something like Object3D.raycastLeaf is a bit use case specific and a solution in Raycaster would better encapsulate things.

One goal for this is to have some kind of feature that lets this "just work" without the user having to adjust settings, otherwise they'll see extremely low performance and wonder why. Three.js puts a lot of effort into making things easy to use and understandable from the API perspective but it would be nice to see some hooks that allow libraries in the ecosystem to build with the same philosophy.

In terms of other use cases any sub group that might use a spatial hierarchy, disable only certain child from raycast (common need when creating a group for multi draw effects, such as #20861), or a set of objects that should be treated a single group.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 12, 2024

@gkjohnson TBH, I'm still not 100% confident about which solution is best at the moment so I want to defer to @mrdoob here. A new property in Object3D is definitely something he should approve.

If there were reserved layers for certain functionality (ie layer 2 is "ignore raycast" as Unity supports) then this would work better, though still a bit strange. Layers feels like an odd place for this, in my opinion.

TBH, I still favor a solution based on layers but I don't want to force others into it 😅. But maybe we can adapt a similar system like Unity. That said, this kind of design question are ideal for @mrdoob^^.

@gkjohnson
Copy link
Collaborator

cc @mrdoob any thoughts here? This impacts the globe controls performance for the google tiles data set quite a bit, which I'd like to get fixed.

@mrdoob
Copy link
Owner

mrdoob commented Feb 20, 2024

Could we use a boolean like this instead?

class TilesGroup extends Group {

  // ... set up

  raycast( raycaster, intersects ) {

    raycaster.enabled = false;

    // ... perform raycasting traversal of children

  }
}

@gkjohnson
Copy link
Collaborator

gkjohnson commented Feb 20, 2024

Could we use a boolean like this instead?

Can you explain what setting "raycaster.enabled" would do? This core issue is that when we encounter an Object3D with a flag or fires the "stopTraversal" callback, for example, that we stop traversing and do not check any children. However we should continue checking any siblings of that Object3D in the rest of the scene. Setting "enabled" to false makes it sound like the raycaster will stop checking everything.

@mrdoob
Copy link
Owner

mrdoob commented Feb 20, 2024

Oh. I guess I misunderstood...

So the idea is to avoid checking the children of an object but continue checking the rest of the graph, right?

@gkjohnson
Copy link
Collaborator

So the idea is to avoid checking the children of an object but continue checking the rest of the graph, right?

That's right. The stats from #27702 show that in the Google Earth tiles case the traversal of the children can well more than double the time it takes to perform a raycast just due to traversal even when no raycasting is being performed by those children (the raycast function for the children has been overridden to be a no op).

Something like a Object3D.doNotRaycastChildren flag would be suit the need, if that makes it more clear.

@mrdoob
Copy link
Owner

mrdoob commented Feb 28, 2024

Sorry, I renamed the internal function for readability. 71c1f51

@mrdoob
Copy link
Owner

mrdoob commented Feb 28, 2024

How about this?

function intersect( object, raycaster, intersects, recursive ) {

	let propagate = true;

	if ( object.layers.test( raycaster.layers ) ) {

		const result = object.raycast( raycaster, intersects );

		if ( result === false ) propagate = false;

	}

	if ( propagate === true && recursive === true ) {

		const children = object.children;

		for ( let i = 0, l = children.length; i < l; i ++ ) {

			intersect( children[ i ], raycaster, intersects, true );

		}

	}

}

Then to cancel the propagation we'd do this:

class TilesGroup extends Group {

  // ... set up

  raycast( raycaster, intersects ) {

     if ( objectHitAndChildrenCanBeIgnored ) {

       return false;

     }

  }

}

@gkjohnson
Copy link
Collaborator

How about this?

So basically adjust the behavior of "Object3D.raycast" so that it returns a boolean indicating whether the children should be raycast against, correct? I think I still prefer an explicit flag on Object3D since it allows for procedurally inspecting the object ahead of time and knowing whether children will be raycast against which is more flexible. But if returning a boolean is the preferred approach that's fine for me and we can revisit the flag later if it's needed.

  let propagate = true;

  if ( object.layers.test( raycaster.layers ) ) {

  	const result = object.raycast( raycaster, intersects );

  	if ( result === false ) propagate = false;

  }

The default value of undefined would need to be handled, as well, which would have the same behavior as returning "true", right?

@gkjohnson
Copy link
Collaborator

cc @mrdoob - any further thoughts on this?

@gkjohnson
Copy link
Collaborator

cc @mrdoob is one of these solutions acceptable so we can add this feature?

@mrdoob
Copy link
Owner

mrdoob commented Apr 23, 2024

Sorry for the delay.

So basically adjust the behavior of "Object3D.raycast" so that it returns a boolean indicating whether the children should be raycast against, correct?

Correct.

The default value of undefined would need to be handled, as well, which would have the same behavior as returning "true", right?

That's correct.

cc @mrdoob is one of these solutions acceptable so we can add this feature?

I think I still prefer returning a boolean so we don't have to add more API.

@gkjohnson
Copy link
Collaborator

I think I still prefer returning a boolean so we don't have to add more API.

Sounds good. I still think there may be use cases in the future that a returned value doesn't support but we can evaluate that later.

@AlaricBaraou would you like ot adjust the PR such that the raycast function returns a boolean? Otherwise I can give it a look this weekend.

@AlaricBaraou
Copy link
Contributor Author

Sure I'll do the adjustments probably today or tomorrow at the latest.

@AlaricBaraou
Copy link
Contributor Author

@gkjohnson @mrdoob

I removed my initial code and added a flag to stop traversing children only when object.raycast returns true. That way the default undefined is handled.

I was thinking of updating the documentation and maybe adding an example but I thought I should check with you first.

Mugen87 added 2 commits April 24, 2024 16:45
Fix code style.
@Mugen87 Mugen87 added this to the r165 milestone Apr 26, 2024
@gkjohnson gkjohnson merged commit 258c28f into mrdoob:dev Apr 27, 2024
12 checks passed
@gkjohnson gkjohnson changed the title Raycaster: add Raycaster.stopTraversal() Raycaster: add ability to stop raycast traversal May 9, 2024
@mrdoob
Copy link
Owner

mrdoob commented May 29, 2024

  let propagate = true;

  if ( object.layers.test( raycaster.layers ) ) {

  	const result = object.raycast( raycaster, intersects );

  	if ( result === false ) propagate = false;

  }

The default value of undefined would need to be handled, as well, which would have the same behavior as returning "true", right?

That's correct.

Actually, I misspoke... undefined is already "handled" because propagate is true by default.

I still think it makes more sense to return false when we want to stop propagation instead of true and I guess it's not too late yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Object3D / Raycaster: Provide way to stop raycast intersection traversal
5 participants