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 #9135

Merged
merged 13 commits into from
Sep 7, 2020
Merged

Conversation

baothientran
Copy link
Contributor

Fixes #9109

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

I also see gltf_clippingPlanesMatrix is used for IBL as it assumes the matrix stores the reference frame components. This may no longer be true, so I change it to a different uniform

@IanLilleyT Can you please take a look at it? Please let me know if I should fix anything.

@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 changed the title Revised plane transform Fix the transformation of plane's normal Sep 4, 2020
@@ -1155,7 +1155,7 @@ describe(
tileset.clippingPlanes = new ClippingPlaneCollection({
planes: [
new ClippingPlane(Cartesian3.UNIT_Z, -10.0),
new ClippingPlane(Cartesian3.UNIT_X, 0.0),
new ClippingPlane(Cartesian3.UNIT_X, 1.0),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test have the clipping plane close to the center. It can generate a different color. I move it off a little bit. This is the sandcastle example for the test

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, not sure I understand what to look out for here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test fails on my machine if the distance is 0.0. I tried on Sandcastle and it seems to work the same.

@IanLilleyT
Copy link
Contributor

IanLilleyT commented Sep 5, 2020

@baothientran I made a few changes. Some are based on your changes, and some are completely new.

  1. Changed the names from transposeInverse to inverseTranspose. Only doing this because it's the more standard naming convention. Behavior is identical.

  2. Changed clippingPlanesOriginMatrix to referenceMatrix in Model.js, and deriving _clippingPlaneModelViewMatrix and _iblReferenceFrameMatrix from it.

  3. I noticed that the ibl reference matrix had scale built in, because it was using clippingPlanesOriginMatrix without removing the scale. This was a bug waiting to happen, but luckily the shader was normalizing the result after applying the ibl matrix. Ultimately the behavior is the same as before. I used this sandcastle for my testing.

Let me know if these changes seem good to you.

@baothientran
Copy link
Contributor Author

@IanLilleyT The change looks good to me. This will make it less confusing about why using clipping plane matrix for IBL as before

@lilleyse
Copy link
Contributor

lilleyse commented Sep 6, 2020

@IanLilleyT and I did a final pass on the code and added more documentation and some code cleanup. We tested this using @IanLilleyT's sandcastle above and all the other clipping plane sandcastles. During testing we noticed a bug with Model.js shader caching and opened an issue here: #9141. The sandcastle in that issue actually crashes in master so this PR improved the situation a little bit. No need to look into right now.

@IanLilleyT IanLilleyT merged commit e30c325 into master Sep 7, 2020
@IanLilleyT IanLilleyT deleted the revised-plane-transform branch September 7, 2020 23:09
@IanLilleyT
Copy link
Contributor

Merged. Thanks @baothientran !

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