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

Fix the transformation of plane's normal #9110

Closed
wants to merge 9 commits into from
Closed

Conversation

baothientran
Copy link
Contributor

@baothientran baothientran commented Aug 25, 2020

Fixes #9109

This PR will convert the transformation matrix to a normal matrix before multiplying with the plane's normal. Most of the fix happens in glsl czm_transformPlane() function and Cesium.Plane.transform()

@lilleyse Can you please take a look at it?

@cesium-concierge
Copy link

Thanks for the pull request @baothientran!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@baothientran baothientran requested a review from lilleyse August 25, 2020 17:45
@baothientran
Copy link
Contributor Author

It is weird that prettier-check on CI is failing some of the files that I don't modify

@@ -4799,13 +4818,15 @@ function modifyShaderForClippingPlanes(
shader +=
"uniform highp sampler2D gltf_clippingPlanes; \n" +
"uniform mat4 gltf_clippingPlanesMatrix; \n" +
"uniform mat4 gltf_normalClippingPlanesMatrix; \n" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The normal matrix should probably be a Matrix3 everywhere. This keeps it consistent with czm_normal which is also a Matrix3.

@lilleyse
Copy link
Contributor

@baothientran only one suggestion here (which is small but affects a lot of files). I tested non-uniform models and point clouds and it works well.

Copy link
Contributor

@IanLilleyT IanLilleyT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are doing inverse transpose in a lot of different areas now, it could be useful to move that into the Matrix3 and Matrix4 files and add a couple unit tests, similar to what you did in PlaneSpec.js.

Also, I know @lilleyse suggested using Matrix3 but I feel like Matrix4 makes more sense. For clipping planes, we would no longer need to pass an additional u_normalClippingPlanesMatrix. Instead, we just pass u_clippingPlanesMatrix which has the inverse transpose built in. Then czm_transformPlane becomes more simple and we can do return transform * clippingPlane; (if my understanding of https://stackoverflow.com/questions/7685495/transforming-a-3d-plane-using-a-4x4-matrix is correct). This would also remove the need for calcNormalClippingPlanesMatrix functions, since the inverse transpose will happen as part of calcClippingPlanesMatrix. Unless I'm overlooking something I think this will make the code change in this PR a lot smaller.

Thoughts @baothientran @lilleyse ?

@baothientran
Copy link
Contributor Author

@IanLilleyT it is indeed a lot simpler to do transform * clippingPlane with the transform = inverse(transpose(originalMatrix)). I will update it right away

@baothientran
Copy link
Contributor Author

@IanLilleyT there is a little complication. It seems like gltf_clippingPlanesMatrix is also used to calculate IBL in this code. I'm not really sure about how IBL works. The PBR material behavior may be changed if I transpose and inverse the clipping plane matrix.

@baothientran
Copy link
Contributor Author

@IanLilleyT I close this and open another PR #9135 instead. The approaches between two PR are quite difference, so it may be better to start a new one to prevent accidental changes

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.

Transforming a plane with a matrix doesn't produce correct normal
4 participants