-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
Line2 world units raycaster #23358
Line2 world units raycaster #23358
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks for the addition.
The required raycasting logic between world-unit lines and screen-space thickness lines should be sufficiently different that I think the logic would be easier to follow if there were two separate loops for each case. There's also a lot of unnecessary computation happening with world unit lines, as well:
if ( worldUnits ) {
// for loop finding world unit intersections
} else {
// for loop finding screen space intersections
}
And a couple other thoughts:
- I think we should make the intersection spheres in the example a lot smaller so it's more clear where the intersection points are being reported.
- It might be best to treat the world-unit line intersections as capsules similar to the way they're being rendered? As it is it looks like the "hit" point is the point of closest approach, instead, meaning the hit point is not necessarily on the surface. This is happening with screenspace lines, too, though it might be more noticeable with world unit lines. Not sure if that has to happen in this PR, though.
Live link to the updated example:
cc @WestLangley not sure if you have other thoughts
I would create a separate example for raycasting with fat lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example looks great, thanks!
I've added a couple comments on the complexity of the loops and how to address. Can you explain the "isLoopingInside" variable? It doesn't seem like it should be necessary.
examples/jsm/lines/LineSegments2.js
Outdated
|
||
intersects.push( { | ||
if ( ! isInside && isLoopingInside ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes to the non world units case have made this harder to follow. The original goal in writing the loop logic was to make it as close as possible to the shader projection logic and simple so it was easy to verify correctness. I think it would be best to avoid modifying the screenspace case as much as possible here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted to dev's version - #23358 (comment)
examples/jsm/lines/LineSegments2.js
Outdated
// skip the segment if it's entirely behind the camera | ||
const isBehindCameraNear = _start4.z > near && _end4.z > near; | ||
if ( isBehindCameraNear ) { | ||
|
||
// skip the segment if it's entirely behind the camera | ||
const isBehindCameraNear = _start4.z > near && _end4.z > near; | ||
if ( isBehindCameraNear ) { | ||
continue; | ||
|
||
continue; | ||
} | ||
|
||
} | ||
// trim the segment if it extends behind camera near | ||
trimSegment( near ); | ||
|
||
// trim the segment if it extends behind camera near | ||
if ( _start4.z > near ) { | ||
if ( ! isInClipSpace( projectionMatrix, resolution ) ) { | ||
|
||
const deltaDist = _start4.z - _end4.z; | ||
const t = ( _start4.z - near ) / deltaDist; | ||
_start4.lerp( _end4, t ); | ||
continue; | ||
|
||
} else if ( _end4.z > near ) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the world units lines there's no reason to ensure the lines are in clip space anymore. The only reason the camera is required otherwise is because it's necessary they are in screen space. I think the simplest approach would be to iterate over the lines without any screenspace or camera logic and checking ray / line segment distances for intersections. Then there's no requirement that the lines be visible in order to be interacted with. Let me know if that sounds good to you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand - let's say we have a very long line that's going from behind the camera near to inside the camera frustum
We don't want the parts of the lines before the camera near to be considered, but if we only keep the intersections and try to calculate it at the end we will be missing some information - no?
I agree about the clipSpace, remove the if
block for that
Let me know if you meant to remove the trimSegment and isBehindCameraNear as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, out of curiosity - why don't we need to consider far?
The raytracer removes points that are behind the far plane - but sometimes a closer part of the line might fall within the threshold so we would miss intersections there, am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason that world lines have to be handled in screen space. The only reason the screen space lines used the camera is because it's required based on how they're displayed. Picking effectively happens in 2D and given the way they're rendering proper picking is effectively undefined outside of the camera clip space. It's definitely a special case for the screen space line variant.
This isn't the case with the world unit lines, though. They're basically rendered as capsules. Meshes, for example, don't have special handling when they're behind the camera and don't consider the camera at all. This enables rays that are not derived from a camera and mouse coordinate to be used for raycasting (ie vr controller rays outside of view or rays for game mechanics outside of view)
The built in GL line raycasting doesn't use a camera and includes support for a threshold which effectively means they're treated as capsules. I think that's more along the lines of what I imagine for world unit raycasting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would mean the "computeStartEnd", "trimSegment", and "isInClipSpace" functions do not need to be shared and don't need to be pulled out into separate functions. I just want to keep the changes to the original logic minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I forgot to update - I changed the main camera's near to 10
to test behavior and luckily committed it by accident so it can still be tested:
Screen.Recording.2022-02-19.at.19.33.23.mov
The lines are rendered truncated by the near plane and the intersection still happens (denoted by the cursor and the little canvas on the bottom left which still has near set to 1
) - which I'm pretty sure is a bug in my last commits
I just want to confirm - this is the desired behavior? if so I'll remove the calls to trimSegment/isBehindCamera instead of debugging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a function of the math involved. The lines are rendered as capsules and have thickness -- you're not seeing back faces of the capsule but technically the near plane (and therefore the ray start) is within the capsule and registers as a "hit" within the given threshold.
Made a lot of changes, hopefully without new bugs:
With inLoopingInside: Open questions: |
I think world units and screen space is good.
I think if we want to improve screen space line behavior it should be done in a separate PR and this should focus on just adding world space line raycasting. The number of changes has made it harder to follow exactly what's been changed so far. The screen space line logic was a fairly complicated piece of logic to add so I'm less keen on making any changers to it here. That aside there are ambiguities / redundancies at points where segments overlap which causes the popping but I think that's just the nature of this kind of raycasting. I believe it happens with the GL line variant, as well.
Can we confirm that this happens with the original code on master? I just want to make sure it's not a newly introduced issue. If it's on master I think you need to worry about it here and it can be addressed in a different PR. |
I took the current example and used it with the dev version of LineSegments2.js and saw the same behavior - I'll open an issue after this PR |
Hopefully this time everything's sorted out I couldn't make the diff work for the current way the code is organized, but I reverted the raycasting function to mrdoob/dev version. I kept the split in the functions because I the if/else was getting too long (thus the alleged big diff you see in the PR). There are minor changes in variable/global variable names, and the functionality regarding sphere/box is common so I kept it outside the type-specific functions Screen.Recording.2022-02-19.at.20.13.48.mov |
The inset in the original example was added to study the impact of canvas size and the user-set resolution parameter. I would not include the inset viewport in the raycasting example -- unless there is a compelling reason to do so. |
This example also showcases how the resolution affects the rendered lines relative size |
@bergden-resonai Thank you for your contribution. I'll defer to whatever @gkjohnson recommends. :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks again.
I'll defer to whatever @gkjohnson recommends. :-)
I don't have any strong feelings on this. It can always be adjusted later.
I don't
Thanks! |
* Try brute force... * Add example and clean-up code * Remove comment * Applying PR comments, bug fix, creating new example for raycasting * adding new example to the tags * Add example icon and undo build/ changes * Revert build files * Checkout build/ from updated upstream * WIP refactor to split logic * Bug fixes and cleanup * Fix screenshot * PR notes * Cleaning up * More cleanup * Bug fixes Co-authored-by: ariel-resonai <[email protected]>
* Try brute force... * Add example and clean-up code * Remove comment * Applying PR comments, bug fix, creating new example for raycasting * adding new example to the tags * Add example icon and undo build/ changes * Revert build files * Checkout build/ from updated upstream * WIP refactor to split logic * Bug fixes and cleanup * Fix screenshot * PR notes * Cleaning up * More cleanup * Bug fixes Co-authored-by: ariel-resonai <[email protected]>
Fixed #22611.
Description
This contribution is funded by Resonai.