Skip to content

Conversation

@WestLangley
Copy link
Collaborator

As suggested in #14685 (comment).

...plus it removes another annoying cyclic reference in the build. :-)

In some examples, Vector3.unproject() is recomputing the matrix inverse every frame.

I think we are safe making this change now, but we need to be careful when using projectionMatrix.copy().

@Mugen87 What do you think?

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 16, 2019

I think it's okay to do this change since we also assume in Vector3.project() that camera.matrixWorldInverse is in sync with the world matrix of the camera.

@WestLangley
Copy link
Collaborator Author

@Mugen87 Search for projectionMatrix.copy and notice where it is used without copying the inverse, too. This is what I was referring to...

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 16, 2019

I see what you mean. In the examples, the copy happens in Reflector, Refractor, Water and CubeTexturePass. And in StereoCamera and CameraHelper which are located in the core.

It seems CameraHelper might need a small fix since it uses Vector3.unproject() in setPoint(). I guess we should add camera.projectionMatrixInverse.copy( this.camera.projectionMatrixInverse ); after the following line:

camera.projectionMatrix.copy( this.camera.projectionMatrix );

@WestLangley
Copy link
Collaborator Author

@Mugen87 I think we need this subtle change in the comment and code:

// we need just camera projection matrix inverse
// world matrix must be identity

camera.projectionMatrixInverse.copy( this.camera.projectionMatrixInverse );

@WestLangley WestLangley added this to the r103 milestone Mar 19, 2019
@mrdoob mrdoob merged commit a7d80a0 into mrdoob:dev Mar 22, 2019
@mrdoob
Copy link
Owner

mrdoob commented Mar 22, 2019

Thanks!

@WestLangley WestLangley deleted the dev-unproject branch March 22, 2019 16:12
@ghost
Copy link

ghost commented May 30, 2019

I've isolated this PR as the one that breaks raycasting in my app.

I am using projectionMatrix.copy because I'm using a hybrid camera that transitions between the two. Any advice for how to fix this? Here's probably the most relevant part of my code

const matrixToCopy = this.isUsingOrthographicCamera ?
    this._orthographicCamera.projectionMatrix :
    this._perspectiveCamera.projectionMatrix;

this._hybridCamera.projectionMatrix.copy(matrixToCopy);

And the full file is here: https://gist.github.com/bleighb/83ff6814a7c8968425d48db0f580f442

@ghost
Copy link

ghost commented May 30, 2019

Pinging @Mugen87 @WestLangley Any help would be greatly appreciated 😊

@WestLangley
Copy link
Collaborator Author

@bleighb This is not a help site. Please use the three.js forum instead.

@ghost
Copy link

ghost commented May 31, 2019

Sorry about that. I'm not asking for handouts, it's just I took a good amount of time to isolate the exact commit that broke my app and I figured other people might be affected by this bug. Since you made the change, I thought the answer might either indicate that either there was a mistake on your part or I'm doing something terribly wrong 😄

@ghost
Copy link

ghost commented May 31, 2019

I just fixed it by copying over the projectionMatrixInverse when I switch between camera types.

You mentioned before:

Thing is, projectionMatrixInverse is computed in updateProjectionMatrix() only; it is not set elsewhere. For example, it is not set whenever projectionMatrix.copy() is used. This will cause the matrix and its inverse to get out-of-sync.

We need to come up with an approach that will ensure they are synchronized if we want to make use of projectionMatrixInverse.

I believe this is what happened to me. The matrices came out of sync and it yielded weird side effects. I'm not asking for help anymore - I'm just trying to see if this is an issue or could be improved.

@WestLangley
Copy link
Collaborator Author

Good. You were able to solve the bug in your app yourself. I think the library is fine as is.

@funwithtriangles
Copy link
Contributor

Thanks @bleighb for highlighting this, helped me with my own debugging!

For any 8th Wall users who have ended up here due to issues since r103, here's the fix:
https://gist.github.com/funwithtriangles/256de89332e438d8740301253ad9efd4

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.

4 participants