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

LineSegments2: Fix raytracing when the geometry has instanceCount set. #25032

Merged

Conversation

MixMasterMitch
Copy link
Contributor

Description

The LineSegments2 mesh has support for raytracing. However, all segments are evaluated when checking for intersections, even if the geometry's instanceCount has been set to a lower value than the total segment count. Therefore, intersections are still detected with the non-visible line segments. This PR fixes this by considering the instanceCount when setting the iteration count during ray tracing.

The bounding box/sphere methods still do not consider the instanceCount, but this is a more challenging problem to solve correctly and does not affect the correctness of the ray tracing results.

The behavior before and after can be reproduced by applying a transformation in the Line2 ray tracing example (/examples/#webgl_lines_fat_raycasting). Here is an example patch that can be used:

diff --git a/examples/webgl_lines_fat_raycasting.html b/examples/webgl_lines_fat_raycasting.html
index 232623c744..a830a533a8 100644
--- a/examples/webgl_lines_fat_raycasting.html
+++ b/examples/webgl_lines_fat_raycasting.html
@@ -162,6 +162,7 @@
 				segments.scale.set( 1, 1, 1 );
 				scene.add( segments );
 				segments.visible = false;
+				segmentsGeometry.instanceCount = 50;

 				thresholdSegments = new LineSegments2( segmentsGeometry, matThresholdLine );
 				thresholdSegments.computeLineDistances();

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 29, 2022

It seems there is now a conflict after merging #24405. Do you mind updating the PR?

@MixMasterMitch MixMasterMitch force-pushed the line-segments-2-instance-count-raycast-fix branch from b95c355 to 728b0f3 Compare November 29, 2022 18:27
@MixMasterMitch
Copy link
Contributor Author

It seems there is now a conflict after merging #24405. Do you mind updating the PR?

Done @Mugen87

@MixMasterMitch
Copy link
Contributor Author

@Mugen87 Can this change be merged now?

@Mugen87 Mugen87 requested a review from gkjohnson December 9, 2022 09:20
Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me!

@Mugen87 Mugen87 added this to the r148 milestone Dec 9, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 9, 2022

The bounding box/sphere methods still do not consider the instanceCount, but this is a more challenging problem to solve correctly and does not affect the correctness of the ray tracing results.

It has been discussed in #8690 whether bounding volumes should honor certain geometry properties or not. It seems BufferGeometry.groups should not be supported but there is no decision so far about drawRange (or related properties which determine what part of the geometry is going to be rendered).

If we add no support for drawRange, I doubt instanceCount is going to be supported, too.

@Mugen87 Mugen87 changed the title Fix raytracing of LineSegments2 when the geometry has instanceCount set. LineSegments2: Fix raytracing when the geometry has instanceCount set. Dec 9, 2022
@Mugen87 Mugen87 merged commit 5ff3fe6 into mrdoob:dev Dec 9, 2022
RodrigoHamuy added a commit to RodrigoHamuy/three-stdlib that referenced this pull request Aug 5, 2024
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.

3 participants