mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.83k stars 35.31k forks source link

Raycasting lines returns distance in object space but intersection in world space #5827

Closed m-schuetz closed 9 years ago

m-schuetz commented 9 years ago

I believe that raycasting to lines returns the calculated distance in object space and the intersection point in world space.

In https://github.com/mrdoob/three.js/blob/dev/src/objects/Line.js#L51 the ray is transformed to object space. In https://github.com/mrdoob/three.js/blob/dev/src/objects/Line.js#L102, https://github.com/mrdoob/three.js/blob/dev/src/objects/Line.js#L135 and https://github.com/mrdoob/three.js/blob/dev/src/objects/Line.js#L166 the intersection point is transformed to world space but the distance is left untouched.

This affects objects with non-rigid-transformations such as scale.

m-schuetz commented 9 years ago

Due to this, raycasting may sort objects in a wrong order if they're differently scaled.

WestLangley commented 9 years ago

Yep. Furthermore, it appears the precision test is being performed in local space, too.

jee7 commented 9 years ago

I just ran into this bug. Had to bake the scale transformation inside the geometry, otherwise the first intersection was wrong at times, when there were differently scaled lines in the scene.

WestLangley commented 9 years ago

@jee7 There have been a lot of changes since this issue was posted.

If you can demonstrate a three.js bug, can you provide a test case so we can reproduce your results? Please see "How to report a bug" in the guidelines.

@m-schuetz Is this issue still an issue for you?

jee7 commented 9 years ago

Yes, I can provide a test case. I initially did not, because it seemed you have already discussed that issue. Anyway, here it is:

I have 2 Line object in my scene. I scale one of them using line.scale.set(), if I send a ray towards them, then it incorrectly orders the intersections (and I think that I don't even get all of them, there are only 3, while it should be 4). If I bake the scale transformation into the geometry using applyMatrix(...) then the result is correct.

Current result: Current result

Desired result: Current result

Here is my test case: http://tume-maailm.pri.ee/ylikool/ComputerGraphics-2015/other/threejs/bug2/

I am using Three.js r71 (just downloaded it just to be sure). Chrome version: Version 44.0.2403.157 m Windows 7 and 8.

Also, I think the direction vector of the ray should be normalized. It does not say this in the documentation, but if I read the library correctly, then you are doing the scalar projection onto the direction when testing if the ray intersects the bounding sphere of the line object. This requirement to normalize the direction vector does not seem to be documented, if I'm not mistaken.

WestLangley commented 9 years ago

@jee7 All 4 of the results are returned -- you just call shift() in the application, and the console is fooling you.

The problem is that the distances are being computed in local space, not world space -- which is the point of the bug report.

Try this change:

if ( distSq > precisionSq ) continue;

interRay.applyMatrix4( this.matrixWorld ); // --------- added

var distance = raycaster.ray.origin.distanceTo( interRay ); // -------- changed

if ( distance < raycaster.near || distance > raycaster.far ) continue;

/ping @m-schuetz

And, yes, all direction vectors in three.js are assumed to be normalized.

jee7 commented 9 years ago

You are right, I fooled myself with the shift().

You are also correct about the nature of the problem and the fix seems to work. Should I create a PR with that fix?

Maybe requirement for the normalized direction vector should be documented under the Raycaster? Should I create a PR for that as well?

m-schuetz commented 9 years ago

@WestLangley Not an important issue for me since I worked around it by recalculating the correct distances afterwards ( https://github.com/potree/potree/blob/8c171aaacfa9134dbbc0fe4ae7fb195f44a78014/src/utils/TransformationTool.js#L632 ) but if that fix is applied I could clean up the workaround.

WestLangley commented 9 years ago

@jee7

Should I create a PR with that fix?

Yes, please.

documented under the Raycaster?

Yes. Note the inline documentation.

@m-schuetz You are a perfect test case, then. : - )

mrdoob commented 9 years ago

@m-schuetz you can now remove that workaround 😉