Implemented raycast method for fat lines. #15356
Conversation
|
@Mugen87 Thanks for review! Should I create new branch with fixes and then create new pull request? Or there is easier way? |
|
You can just add additional commits to this branch. There is no need for a new PR. |
|
Okay, let's see what @WestLangley thinks about the PR. |
examples/js/lines/LineSegments2.js
Outdated
|
|
||
| return function raycast( raycaster, intersects ) { | ||
|
|
||
| var precision = raycaster.linePrecision; // todo: calculate proper precision |
There was a problem hiding this comment.
Line width is in pixels. What you are doing here is not correct. You can't mix world units and pixels. Maybe, if you see how sprite raycasting is handled, there is a workaround.
There was a problem hiding this comment.
I think @weeklyTea just adapted the approach of Line.raycast().
There was a problem hiding this comment.
Well, that won't work because lines can be fat and are specified in pixels -- unless you are willing to require the user to set the precision based on the camera settings and screen resolution, and accept that approach as "good-enough".
There was a problem hiding this comment.
@WestLangley , @Mugen87 , yes you rigth. In our proj we have another implementation: we project 3d points into 2d pixel coords using Vector3.project(camera) so then it is ok to compare cursor coordinates and linewidth, but also we use our custom raycaster contains camera.. I checked raycast method for sprites, am i right it is possible to project point3 onto a view plane using this.modelViewMatrix (seems function transformVertex in src/objects/Spite.js does what i described)? If yes our approach can be easily reimplemented without using camera.
There was a problem hiding this comment.
@weeklyTea I am glad fat lines are working for you! Perhaps you will be able to rework this PR. Also see #14936. Some of the issues may be similar...
There was a problem hiding this comment.
@WestLangley I tried to find solution using modelViewMatrix and found only one approach which works only for orthographic camera and with some hacks. Sprite approach can't be used here because sprites change own size (width, len, height) in pixels on zoomIn/Out, and fatlines don't change width in pixels on zoomIn/Out. Afraid it is impossible to count linewidth in pixel properly without projectionMatrix in raycast method.
So I see next solutions:
- Save projection matrix (or whole camera) in
raycaterobject insetFromCameramethod. - As @WestLangley suggested: require the user to set the precision and pass it to raycaster..
For me 1st looks very good. In 2nd it is impossible to set proper precision for lines which have startPoint near camera and endPoint far from camera.
There was a problem hiding this comment.
Yes, I mentioned option (2), but I don't like it, either. If you want to file a PR for (1), we can have a look. I do not know if (1) will be acceptable to others or not.
…ray as optionalTarget
Mugen87
dismissed
their stale review
Raycasting is not proper implemented yet
|
@Mugen87, @WestLangley, I implemented new approach. Now raycast works with points with ndc coordinates (projections of points on ray and linesegment), so now it is possible to compare distance between points and linewidth. But now raycast works only if camera was passed into raycaster. |
|
@weeklyTea The raycasting appears to me to have huge positional errors... ??? |
As I understand it happens because resolution in shader is not equals to resolution in raycast method. Seems guilty line is 2b056a3#diff-3807f6cd3995839aef359ea88e0eb2f5R220 if you comment it everything should work fine. |
src/core/Raycaster.js
Outdated
|
|
||
| this.ray.set( origin, direction ); | ||
|
|
||
| this.camera = camera; |
There was a problem hiding this comment.
I think it should be possible to set origin and direction without reseting this.camera so maybe better to use something like this.camera = camera !== undefined ? camera : this.camera ?
There was a problem hiding this comment.
I personally think this tight coupling between Raycaster and Camera is a bad design. I vote for option 2 (working with a user-defined precision value).
There was a problem hiding this comment.
But 2 will work only if start of line will be near to end by Z coordinate (I mean line will has small depth)..
Why saving camera in raycaster is bad? Will it be better if the only camera matrices will be saved?
There was a problem hiding this comment.
I think the best solution may be GPU picking. Option 2 will not work reliably. Option 1 requires camera parameters.
There was a problem hiding this comment.
@WestLangley I've taken a look how to make GPU picking and it seems in this case we still need additional data like renderer and mouse position (the last one is needed only for perspective camera) :(
|
@Mugen87 @WestLangley could you give a feedback about this solution? Is it ok or should I change something? |
src/core/Raycaster.js
Outdated
|
|
||
| this.ray.set( origin, direction ); | ||
|
|
||
| this.camera = camera; |
There was a problem hiding this comment.
I personally think this tight coupling between Raycaster and Camera is a bad design. I vote for option 2 (working with a user-defined precision value).
The raycasting is still not accurate when I test it. I tested with 100 px line width. |
|
Closing since the implementation is not yet satisfying. Moving on with this issue in #17160 |
4 participants
Fixes #15006