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

Make sure clipping plane origin matches the bounding sphere center #8392

Closed
wants to merge 1 commit into from

Conversation

OmarShehata
Copy link
Contributor

This addresses the offset reported in:

This is easy to reproduce in this Sandcastle. Notice that the X/Y directions work fine, where 0 cuts through the middle of the tileset, and it correctly goes from -1 to 1 across in that axis. For Z, the 0 is at the bottom of the model.

This offset is because the difference between where the tileset.root.computedTransform places the origin, and where the tileset.root.boundingSphere.center is. In the image below, the red sphere marks the bounding sphere center. The white marks the root tile's computed transform's origin:

image

You can draw these two points on the power plant model to show that they are very close, but not the same, and that's where the small discrepancy there comes from. This PR fixes this issue by making sure to use the bounding sphere center as the origin for the clipping plane in the case where the bounding sphere center is not above the surface (where the tileset transform is what places the tile in its location on the globe).

I marked as a draft because I still need to go through the discussion in #7182 to see why this wasn't done initially/make sure it's not breaking anything else.

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ 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.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

@liyangis
Copy link

liyangis commented Nov 15, 2019

Thank you for you reply,But,

  1. I hope that I want to move the clipping plane origin anywhere on the fly when I need to cut,not only tileset.root.boundingSphere.center or tileset.root.transform ,
  2. In my example,I have move the the clipping plane to the center of boundingSphere,but why the clipping plane has not arrived the center successfully?

var offset = boundingSphere.center var translation = Cesium.Cartesian3.subtract(offset, transformCenter, new Cesium.Cartesian3()); clippingPlanes.modelMatrix = Cesium.Matrix4.fromTranslation(translation);

@OmarShehata
Copy link
Contributor Author

@liyangis the clipping plane is positioned relative to the bounding sphere center. You can use the clippingPlanes.modelMatrix to transform it further relative to that origin.

Setting the modelMatrix to be a transform relative to the bounding center should work. Were you testing your code with this branch? If so, do you have a Sandcastle example showing the issue?

@liyangis
Copy link

liyangis commented Nov 15, 2019

@liyangis the clipping plane is positioned relative to the bounding sphere center. You can use the clippingPlanes.modelMatrix to transform it further relative to that origin.

Setting the modelMatrix to be a transform relative to the bounding center should work. Were you testing your code with this branch? If so, do you have a Sandcastle example showing the issue?

look at this example
this one

@OmarShehata
Copy link
Contributor Author

@liyangis yes this example is fixed in my branch. See this modified version of it.

I commented out this section of code, since in this branch it is no longer true that the clipping plane origin is offset from the bounding sphere center:

// The clipping plane is initially positioned at the tileset's root transform.
            // Apply an additional matrix to center the clipping plane on the bounding sphere center.
            transformCenter = Cesium.Matrix4.getTranslation(tileset.root.transform, new Cesium.Cartesian3());
            var height = Cesium.Cartesian3.distance(transformCenter, tileset.boundingSphere.center);
            //clippingPlanes.modelMatrix = Cesium.Matrix4.fromTranslation(new Cesium.Cartesian3(0.0, 0.0, height));
              var offset = boundingSphere.center
                // 有方向的,第一个参数为移动目的地,第二个参数为原来的位置
                var translation = Cesium.Cartesian3.subtract(offset, transformCenter, new Cesium.Cartesian3());
                clippingPlanes.modelMatrix = Cesium.Matrix4.fromTranslation(translation);
            var hpr = Cesium.Transforms.fixedFrameToHeadingPitchRoll(tileset.root.transform);

@liyangis
Copy link

@liyangis yes this example is fixed in my branch. See this modified version of it.

I commented out this section of code, since in this branch it is no longer true that the clipping plane origin is offset from the bounding sphere center:

// The clipping plane is initially positioned at the tileset's root transform.
            // Apply an additional matrix to center the clipping plane on the bounding sphere center.
            transformCenter = Cesium.Matrix4.getTranslation(tileset.root.transform, new Cesium.Cartesian3());
            var height = Cesium.Cartesian3.distance(transformCenter, tileset.boundingSphere.center);
            //clippingPlanes.modelMatrix = Cesium.Matrix4.fromTranslation(new Cesium.Cartesian3(0.0, 0.0, height));
              var offset = boundingSphere.center
                // 有方向的,第一个参数为移动目的地,第二个参数为原来的位置
                var translation = Cesium.Cartesian3.subtract(offset, transformCenter, new Cesium.Cartesian3());
                clippingPlanes.modelMatrix = Cesium.Matrix4.fromTranslation(translation);
            var hpr = Cesium.Transforms.fixedFrameToHeadingPitchRoll(tileset.root.transform);

oh,if I want to change the clipping plane's origin to another position on the fly ,how to do?

@cesium-concierge
Copy link

Thanks again for your contribution @OmarShehata!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

1 similar comment
@cesium-concierge
Copy link

Thanks again for your contribution @OmarShehata!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@lilleyse
Copy link
Contributor

#8555 might be relevant here. There was a mistake in the sandcastle code. If you want to position the clipping planes at the bounding sphere center try this instead:

        if (!Cesium.Matrix4.equals(tileset.root.transform, Cesium.Matrix4.IDENTITY)) {
            // The clipping plane is initially positioned at the tileset's root transform.
            // Apply an additional matrix to center the clipping plane on the bounding sphere center.
            var transformCenter = Cesium.Matrix4.getTranslation(tileset.root.transform, new Cesium.Cartesian3());
            var transformCartographic = Cesium.Cartographic.fromCartesian(transformCenter);
            var boundingSphereCartographic = Cesium.Cartographic.fromCartesian(tileset.boundingSphere.center);
            var height = boundingSphereCartographic.height - transformCartographic.height;
            clippingPlanes.modelMatrix = Cesium.Matrix4.fromTranslation(new Cesium.Cartesian3(0.0, 0.0, height));
        }

@liyangis If you want to position the clipping planes arbitrarily, create a matrix in global coordinates and multiply it by the inverse of the tileset's root transform (or tileset.clippingPlanesOriginMatrix if you are ok with using private vars). The key is to define the matrix in the tileset's local space.

Sandcastle example of this approach here.

I also opened up #8554 since a think we should move to clipping planes always being defined in global space.

@cesium-concierge
Copy link

Thanks again for your contribution @OmarShehata!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

1 similar comment
@cesium-concierge
Copy link

Thanks again for your contribution @OmarShehata!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@OmarShehata
Copy link
Contributor Author

@cesium-concierge stop

@mramato
Copy link
Contributor

mramato commented Mar 30, 2020

@OmarShehata what's the status of this PR? It's a really small change and it's unclear what is holding it up.

@OmarShehata
Copy link
Contributor Author

@mramato looking at this again, this isn't resolving a bug as much as a breaking change to make the behavior more intuitive. The ClippingPlaneCollection docs states:

For 3D Tiles, the root tile's transform is used to position the clipping planes. If a transform is not defined, the root tile's Cesium3DTile#boundingSphere is used instead.

Which means the clipping origin being at the bottom of the tileset is expected (in these cases where the root transform is defined).

It isn't clear to me that this is indeed a desired change that won't cause any other issues. I think it makes sense to close this and consider the approach Sean outlined here #8554 where the clipping plane origin will be defined explicitly so that there is no ambiguity.

@mramato
Copy link
Contributor

mramato commented Mar 30, 2020

Thanks!

@mramato mramato closed this Mar 30, 2020
@flyingsnowers
Copy link

Thank you for you reply,But,

  1. I hope that I want to move the clipping plane origin anywhere on the fly when I need to cut,not only tileset.root.boundingSphere.center or tileset.root.transform ,
  2. In my example,I have move the the clipping plane to the center of boundingSphere,but why the clipping plane has not arrived the center successfully?

var offset = boundingSphere.center var translation = Cesium.Cartesian3.subtract(offset, transformCenter, new Cesium.Cartesian3()); clippingPlanes.modelMatrix = Cesium.Matrix4.fromTranslation(translation);

你好,请问你的问题解决了吗,我也遇到相同的问题,请问能否请教下呢?我的版本是1.75

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.

6 participants