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: Check bounding box and bounding sphere when raycasting #21496

Merged
merged 4 commits into from
Mar 29, 2021

Conversation

gkjohnson
Copy link
Collaborator

Related issue: --

Description

Improve LineSegments2 raycasting performance by checking against the bounding sphere and bounding box including a margin computed from the closest distance to the bounds and the screen space line width. Tested by locally adding raycasting to the "webgl_lines_fat" example and zooming inside the bounds as well out far while checking that intersections are returned for both thin (1px) and thick (50px) lines.

50px width 1px width
box margin ~17.6 units ~0.37 units
box screenshot image image
sphere margin ~23.6 units ~0.47 units
sphere screenshot image image

@WestLangley your eyes on the logic would be appreciated here 😁

@mrdoob mrdoob requested a review from WestLangley March 23, 2021 16:28
@mrdoob mrdoob added this to the r127 milestone Mar 23, 2021
@WestLangley WestLangley removed their request for review March 28, 2021 16:14
@WestLangley
Copy link
Collaborator

@mrdoob Garrett is maintaining the fat-lines raycast code, so I would trust him on this. TBH, I have not yet studied it.

//

Also, I think a separate test case would be nice. Thing is, the fat lines example was selected to be pretty, not typical. In the existing example, the line segments are very short, and consist of mostly end-caps. Relatively-longer line segments in a raycast test would make the most sense to me. For example:

const divisions = points.length - 1;

Screen Shot 2021-03-28 at 12 34 07 PM

@gkjohnson
Copy link
Collaborator Author

Well from my testing it all works as expected. I tried it in a more practical case in a project with a lot of LineSegments2 objects and I saw performance improvement from around 40-50ms down to 10ms when raycasting so it's definitely an improvement, as well. It all seems reasonable and ready to merge to me but a bit of a review if possible is always nice.

Also, I think a separate test case would be nice. Thing is, the fat lines example was selected to be pretty, not typical. In the existing example, the line segments are very short, and consist of mostly end-caps. Relatively-longer line segments in a raycast test would make the most sense to me. For example:

I'm happy to add another example to test raycasting in a subsequent PR if @mrdoob okay with that. In my opinion short line segments aren't necessarily an uncommon case -- I use these frequently to render arcs or circles as well as longer lines. I think a raycasting example with both types of segments would be good.

@mrdoob mrdoob merged commit efef2b3 into mrdoob:dev Mar 29, 2021
@mrdoob
Copy link
Owner

mrdoob commented Mar 29, 2021

Thanks!

@gkjohnson gkjohnson deleted the improved-line2-raycast branch March 29, 2021 18:26
@gkjohnson
Copy link
Collaborator Author

@mrdoob are you interested in another example that demonstrates raycasting for LineSegments2 for testing or should I hold off on that.

@mrdoob
Copy link
Owner

mrdoob commented Mar 29, 2021

Hmm, I think so yes! 👍

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