Fix clipping planes for tiles with RTC and/or no transforms #7034

OmarShehata · 2018-09-11T21:27:12Z

Don't merge yet. Opening for feedback. I still have to:

Write tests.
Update tileset traversal code to use the new transforms.
Remove the debug return and eslint disable from Cesium3DTile.js.
Compute correct clipping matrix for PointCloud3DTileContent and make sure Composite works.
Make a pass for optimization/re-using scratch matrices (feedback welcome here).
Make docs explicit about what the coordinates are relative to and how to define them for some common cases.
Make sure ClippingPlane API is consistent with Plane (in that the signed distance means the same thing).

This resolves #6600. I started with the solution in #6616 and noticed that it has some peculiar behavior:

Notice how this is almost correct, except there's a narrow window in which the the whole tileset becomes visible. This is fixed when this scratch matrix is moved inside the function. The reason this inverse of the tile's modelMatrix was multiplied there is because the clipping plane's coordinates should be relative to just the root tile, not to each child, so when Model.js multiplies in each tile's modelMatrix, its inverse cancels its out.

I think a simpler way is to simply save the root tile's matrix and just use that when this model is part of a tileset. This works just like before and fixes the issue in the GIF above.

Tilesets with No Root Transforms

The problem with the other tileset in the original issue is that the root tile had no defined transform, which required you to manually move the clipping plane so that it's relative to the tileset:

clippingPlanes.modelMatrix = 
 Cesium.Transforms.eastNorthUpToFixedFrame(tileset.boundingSphere.center)

Since the user expects the clipping plane to be relative to the tileset, this can be annoying. The proposed solution here uses the tileset's bounding sphere if no root transform is defined.

Tilesets with RTC Coordinates

Another problem was tilesets that are defined with RTC coordinates. The clipping plane thinks the model's coordinates are relative to the center of the earth, so it also required manually moving the clipping planes to the bounding sphere as above. The solution here applies the RTC offset when found.

@lilleyse there are a few things I'm unsure about here:

Would the CESIUM_RTC extension need to be handled separately? What's a good test case for this?
I've applied the RTC offset to i3dm and b3dm. Do we need to do this for all other tile formats?
- Similarly for the bounding sphere fallback?

There's one dataset this doesn't work as I expect it to. On Cesium3DTiles/Instanced/InstancedRTC the clipping plane seems tilted:

Even though the plane is defined as new Cesium.ClippingPlane(new Cesium.Cartesian3(1.0, 0.0, 0.0), 0) with no other transformation. Here's a localhost:8080 Sandcastle link. I'm not sure why it's rotated that way or if this is expected.

Overall, this fixes the original issue as well as a lot of the tilesets under the Cesium3DTiles/Instanced/ directory, many of which either had no transforms on the root tile or were using RTC coordinates.

cesium-concierge · 2018-09-11T21:27:13Z

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.

Reviewers, don't forget to make sure that:

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

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

lilleyse · 2018-09-11T21:48:37Z

Nice summary of the various cases here. 👍

The approach seems good to me, bump when the code is ready to review.

Would the CESIUM_RTC extension need to be handled separately? What's a good test case for this?

This case should get caught by the bounding sphere fallback. I don't think there's a clean way for Batched3DModel3DTileContent to know that this extension exists.

For test data grab any of the test tilesets in Cesium 1.47 or earlier. Specs/Data/Cesium3DTiles/Tilesets/Tileset should be using that extension.

I've applied the RTC offset to i3dm and b3dm. Do we need to do this for all other tile formats?
Similarly for the bounding sphere fallback?

Handle the RTC offset for pnts too. Check that cmpt works as well just in case. vctr and geom don't use clipping planes and can be skipped.

There's one dataset this doesn't work as I expect it to. On Cesium3DTiles/Instanced/InstancedRTC the clipping plane seems tilted:

Maybe the matrix has to be east-north-up-ified if RTC exists but the transform does not?

lilleyse · 2018-09-12T13:47:12Z

I would also suggest adding to the task list to make the clipping plane documentation super clear with good examples.

OmarShehata · 2018-09-14T14:24:56Z

Small update: just confirmed that #6879 is fixed with this PR! And you were right about:

There's one dataset this doesn't work as I expect it to. On Cesium3DTiles/Instanced/InstancedRTC the clipping plane seems tilted:

Maybe the matrix has to be east-north-up-ified if RTC exists but the transform does not?

Applying east-north-up fixes it!

pjcozzi · 2018-09-15T18:58:06Z

Perhaps @ggetz can take a first pass on review and test before @lilleyse takes another look?

ggetz · 2018-09-17T17:57:44Z

Source/Scene/Cesium3DTile.js


-        var tileset = this._tileset;
+        var tileset = this._tileset; // eslint-disable-line no-unreachable


It sounds like you are going to take a look at tileset traversal soon, but I wouldn't disable this check so it's not overlooked.

ggetz · 2018-09-17T18:00:00Z

Source/Scene/Batched3DModel3DTileContent.js

@@ -358,10 +358,16 @@ define([
        content._rtcCenterTransform = Matrix4.clone(Matrix4.IDENTITY);
        var rtcCenter = featureTable.getGlobalProperty('RTC_CENTER', ComponentDatatype.FLOAT, 3);
        if (defined(rtcCenter)) {
-            content._rtcCenterTransform = Matrix4.fromTranslation(Cartesian3.fromArray(rtcCenter), content._rtcCenterTransform);
+            content._rtcCenterTransform = Transforms.eastNorthUpToFixedFrame(Cartesian3.fromArray(rtcCenter), content._rtcCenterTransform);
+            tile._hasRTC = true;


Here and throughout, object properties, even if private, should be defined in the constructor.

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Documentation/Contributors/CodingGuide/README.md#constructor-functions

ggetz · 2018-09-17T18:03:38Z

Source/Scene/Model.js

@@ -4426,7 +4429,12 @@ define([
            var clippingPlanes = this._clippingPlanes;
            var currentClippingPlanesState = 0;
            if (defined(clippingPlanes) && clippingPlanes.enabled) {
-                Matrix4.multiply(context.uniformState.view3D, modelMatrix, this._modelViewMatrix);
+                if (Matrix4.equals(this._clippingPlaneOffsetMatrix, Matrix4.IDENTITY)) {


This seems a little confusing. When not just initialize it to undefined?

ggetz · 2018-09-17T18:11:07Z

Source/Scene/Batched3DModel3DTileContent.js

@@ -465,6 +471,19 @@ define([
        // Update clipping planes
        var tilesetClippingPlanes = this._tileset.clippingPlanes;
        if (this._tile.clippingPlanesDirty && defined(tilesetClippingPlanes)) {
+            // Use the root tile's transform.
+            if (!this._tile._useBoundingSphereForClipping) {


Does pre-computing this property in initialize provide much benefit? I don't see it used elsewhere.

If not, I think we should perform the check here to keep similar logic in one place. Same for Instanced3DModel3DTileCotent.

Source/Scene/Batched3DModel3DTileContent.js

OmarShehata · 2018-09-18T20:44:02Z

I think this is finally ready @lilleyse. Thanks @ggetz for the review!

The final fix is a lot more simpler and elegant than the original commit. On creation of the root tile, it will check if the transform is missing and save a clippingPlanes matrix that uses the bounding sphere instead. All tile formats will use that if it exists, or otherwise just use the root tile's transform.

After finished the traversal update, I realized that #6879 isn't actually a bug. I'm fairly certain that tile that pops up just has an incorrect bounding box, so the traversal code marks it as completely inside so it doesn't attempt to clip it with the shader. This is easier to see when you turn on the debug bounding boxes.

lilleyse · 2018-09-19T01:16:49Z

Update CHANGES.md and add a detailed note about the breaking change:

Make sure ClippingPlane API is consistent with Plane (in that the signed distance means the same thing).

The note should include a short example of how someone using previous versions can upgrade their clipping plane code. Probably something as simple as flipping some signs, just make sure it is super clear.

lilleyse

This looks really good @OmarShehata. The approach looks solid and the doc updates are nice and clear. I haven't actually tested anything too carefully yet, though did run through the Sandcastle examples. The only problem I noticed was the i3dm example in Many Clipping Planes doesn't seem to work.

lilleyse · 2018-09-19T01:39:32Z

Source/Scene/Batched3DModel3DTileContent.js

+                this._model._clippingPlaneOffsetMatrix = this._tileset.root.computedTransform;
+            } else {
+                this._model._clippingPlaneOffsetMatrix = this._tileset.root._clippingPlaneOffsetMatrix;
+            }


It might be slightly cleaner to move this all to a getter property in Cesium3DTileset - tileset.clippingPlaneOffsetMatrix or something like that. A lot of areas can use the getter rather than checking the logic themselves.

E.g. this._model._clippingPlaneOffsetMatrix = this._tileset.clippingPlaneOffsetMatrix

lilleyse · 2018-09-19T01:45:21Z

Source/Scene/Cesium3DTile.js

+        if (!defined(this.parent) && !defined(this._header.transform)) {
+            this._clippingPlaneOffsetMatrix = Transforms.eastNorthUpToFixedFrame(this.boundingSphere.center);
+            this._useBoundingSphereForClipping = true;
+        }


It seems a little wasteful that _useBoundingSphereForClipping and _clippingPlaneOffsetMatrix are defined for all Cesium3DTiles even though it's only used by the root. Maybe they can be moved to properties of Cesium3DTileset, perhaps initialized right after the json is loaded.

In general we try to keep Cesium3DTile and other commonly created objects as trimmed down as possible, though the wall of properties in Cesium3DTile might seem deceiving (and could be improved).

lilleyse · 2018-09-19T01:46:26Z

Source/Scene/Cesium3DTile.js

@@ -822,6 +828,9 @@ define([
        var clippingPlanes = tileset.clippingPlanes;
        if (defined(clippingPlanes) && clippingPlanes.enabled) {
            var tileTransform = tileset.root.computedTransform;
+            if(defined(tileset.root._clippingPlaneOffsetMatrix)) {


Style: add space after the if: if (

lilleyse · 2018-09-19T01:48:25Z

Source/Scene/Cesium3DTile.js

@@ -857,6 +866,9 @@ define([
        var clippingPlanes = tileset.clippingPlanes;
        if (defined(clippingPlanes) && clippingPlanes.enabled) {
            var tileTransform = tileset.root.computedTransform;
+            if(defined(tileset.root._clippingPlaneOffsetMatrix)) {


Same style comment.

lilleyse · 2018-09-19T01:55:24Z

Source/Scene/ClippingPlaneCollection.js

+     * var entity = viewer.entities.add({
+     *     position : Cesium.Cartesian3.fromDegrees(-123.0744619, 44.0503706, 10000),
+     *     model : {
+     *         uri : '../../../../Apps/SampleData/models/CesiumAir/Cesium_Air.glb',


This type of path only works in the built version hosted on the website. Check if other examples do this. It might also be fine to say "model.glb" or something easier to parse.

lilleyse · 2018-09-19T02:02:59Z

Source/Scene/Model.js

+        // If defined, it's used to position the clipping planes instead of the modelMatrix.
+        // This is so that when models are part of a tileset they all get clipped relative
+        // to the root tile.
+        this._clippingPlaneOffsetMatrix = undefined;


Slight wording tweak // If defined, use this matrix to position the clipping planes instead of the modelMatrix.

lilleyse · 2018-09-19T02:03:30Z

Source/Scene/PointCloud.js

@@ -185,6 +185,10 @@ define([
        this.clippingPlanes = undefined;
        this.isClipped = false;
        this.clippingPlanesDirty = false;
+        // If defined, it's used to position the clipping planes instead of the modelMatrix.
+        // This is so that when point clouds are part of a tileset they all get clipped relative
+        // to the root tile.


Same comment here.

lilleyse · 2018-09-19T02:14:52Z

Source/Scene/PointCloud.js

+                } else {
+                    Matrix4.multiply(context.uniformState.view3D, pointCloud._clippingPlaneOffsetMatrix, scratchClippingPlaneMatrix);
+                }
+                return scratchClippingPlaneMatrix;


Is clippingPlanes.modelMatrix getting accounted for?

This may explain the failing test: Scene/TimeDynamicPointCloud sets clipping planes

Yes, that's my bad! That shouldn't have been removed. Thanks for catching this.

lilleyse · 2018-09-19T02:19:57Z

Source/Scene/ClippingPlaneCollection.js

+     * </p>
+     * <p>
+     * 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 {@link Cesium3DTile#boundingSphere} is used instead.
+     * </p>


Also add a similar note explaining what the clipping plane's coordinate system is when using it on a model and on the globe.

Do you think this is necessary? I was thinking the comment above that "clipping planes' coordinates are relative to the object they're attached to" covers the globe and individual models, since it would just be relative to the center of the globe and the model respectively. Since 3D Tiles are a bit more ambiguous I think it needs the extra explanation.

Alright makes sense.

lilleyse · 2018-09-19T02:23:38Z

Apps/Sandcastle/gallery/3D Tiles Clipping Planes.html

-        loadTileset(pointCloudUrl).then(function(tileset) {
-            tileset.clippingPlanes.modelMatrix = Cesium.Transforms.eastNorthUpToFixedFrame(tileset.boundingSphere.center);
-        });
+        loadTileset(pointCloudUrl);


The point cloud example might be a good use case showing when to use the clipping plane's model matrix since the clipping plane is visually off-center from the church due to the offset bounding sphere.

I tried doing this but one issue is that, you only really need to offset the entity, so it wouldn't really need to use the clipping planes' model matrix.

The other issue is that it requires converting back and forth from Cartographic to Cartesian and it kind of makes creating a clipping plane look complicated.

Makes sense too. And I remember now that the terrain example uses a model matrix, so one example is enough to illustrate the point.

OmarShehata · 2018-09-19T20:06:07Z

Should be good now @lilleyse . The i3dm in "Many Clipping Planes" wasn't working because it was still manually setting the clipping plane's model matrix. I updated it to fix it.

The only thing I'm unsure about now is that after moving the clippingPlaneOffsetMatrix to the tileset, the root tile still updates the tileset's matrix in Cesium3DTile.updateTransform. I think it makes more sense if this happens in the tileset's update function, but this way it only updates when necessary.

lilleyse · 2018-09-20T01:14:31Z

Source/Scene/PointCloud.js

+                    Matrix4.multiply(context.uniformState.view3D, pointCloud._modelMatrix, scratchClippingPlaneMatrix);
+                } else {
+                    Matrix4.multiply(context.uniformState.view3D, pointCloud._clippingPlaneOffsetMatrix, scratchClippingPlaneMatrix);
+                }


Use defaultValue here as well.

lilleyse · 2018-09-20T01:16:52Z

Source/Scene/PointCloud3DTileContent.js

@@ -297,6 +297,8 @@ define([
        var styleDirty = this._styleDirty;
        this._styleDirty = false;

+        pointCloud._clippingPlaneOffsetMatrix = tileset.clippingPlaneOffsetMatrix;


Probably best to rename _clippingPlaneOffsetMatrix to clippingPlaneOffsetMatrix like the other variables that are set in here. Consider doing this for Model too.

In general any variable that can be set from another file doesn't use underscores.

lilleyse · 2018-09-20T01:22:23Z

The only thing I'm unsure about now is that after moving the clippingPlaneOffsetMatrix to the tileset, the root tile still updates the tileset's matrix in Cesium3DTile.updateTransform. I think it makes more sense if this happens in the tileset's update function, but this way it only updates when necessary.

Moving it to the tileset update function sounds fine to me, one extra eastNorthUpToFixedFrame call per frame isn't too bad especially if it is only done if clipping planes are enabled.

OmarShehata · 2018-09-20T15:31:55Z

Should be good now @lilleyse !

OmarShehata · 2018-09-20T17:32:49Z

@lilleyse I updated CHANGES.md, but it looks like the only breaking change is that users will need to remove/re-adjust their model matrix if they were using it to move the clipping planes to the model.

I think the correction to the signed distance happened in this PR #6073. The reason the Plane and ClippingPlane looked reversed in the earlier demos was because I was using the modelMatrix to move them instead of distance.

This Sandcastle localhost:8080/... allows you to move the distance, and you can see that the clipping, and the plane entity do match up. If you move the model matrix slider instead, it moves in the opposite direction, which makes sense when the normal is (1,0,0).

Here's this same Sandcastle on the current release cesiumjs.org/..., aside from the fact that one of the tile's doesn't use the correct transform, the behavior is the same. It's clipping the same side.

lilleyse · 2018-09-21T00:20:10Z

That's really good to hear that ClippingPlane follows Plane behavior and we don't actually need what I thought would be a breaking change.

All the doc edits looks good.

On my final sweep of the Sandcastle demos I actually have two, hopefully small, suggestions.

In the terrain demo have an example where union clipping regions is used to isolate a part of the terrain, like a park area or mountain. It gives a cool view of the scene and is a way to show unionClippingRegions in action. I created a quick demo of what I had in mind - feel free to tweak it: demo
Edge color doesn't seem to work with unionClippingRegions here or in master. Hopefully it is a quick fix.

OmarShehata · 2018-09-21T16:03:03Z

@lilleyse the reason the color wasn't working was because it was incorrectly computing the distance to the planes in the union function. For the intersection we compute the maximum distance from each pixel to any plane. But for union we want the minimum distance.

I also added a "isolating the grand canyon" example to the terrain demo. This should be good to go now!

lilleyse · 2018-09-21T17:42:06Z

Both updates look great. Thanks @OmarShehata!

Omar Shehata added 2 commits September 11, 2018 14:37

Always use root tileset's transform

ad7abcd

Apply RTC & bounding sphere fallback for clipping planes

9f55539

Fixed crash if root tile wasn't loaded

9ed3695

Apply eastNorthUp to RTC center

bdb8799

pjcozzi mentioned this pull request Sep 15, 2018

Fix clipping plane issue with child tiles with transforms #6616

Closed

ggetz requested changes Sep 17, 2018

View reviewed changes

Shehata added 10 commits September 17, 2018 18:18

Fixed RTC for b3dm

6cb0426

Traversal uses correct tileset transforms for clipping

aa71e96

Sinplified BoundingSphere fallback check

0ec49a5

No need to keep track of RTC anymore

0a07e56

Simplify clipping sandcastle example

5aee22a

Use correct matrix for point clouds

1f1cd16

Fixed old tests

8996da6

Fix point cloud tests

7d5bb57

Added bounding sphere tests

b605a61

Added clipping plane doc example

0d2dd5f

lilleyse reviewed Sep 19, 2018

View reviewed changes

Omar Shehata added 6 commits September 19, 2018 10:54

Fix 'Many Clipping Planes' example

814c5ae

Put back point cloud clipping plane matrix

dd3a7c6

Style and wording edits

9db4500

Moved clippingPlaneOffsetMatrix to Tileset

2dac9f4

Updated tests

2fadcf9

Fixed old clipping planes test

b251c6b

Fix lint

e879cca

lilleyse reviewed Sep 20, 2018

View reviewed changes

Shehata added 2 commits September 20, 2018 09:09

Remove underscore from _clippingPlaneOffsetMatrix

5245643

Moved clippingPlaneOffset update to tileset

65a8496

Shehata added 2 commits September 20, 2018 13:14

No need to use setter for clippingPlaneOffsetMatrix

3cc7aa0

Added breaking change to CHANGES.md

888e2b2

Shehata added 2 commits September 20, 2018 17:16

Fix sign in doc

f56c71c

Reworded unionClippingRegions

911a520

Shehata added 2 commits September 21, 2018 11:19

Fix edge color for union clipping planes

a43ab57

Added terrain union example

79a49e0

lilleyse merged commit b73152b into CesiumGS:master Sep 21, 2018


		var tileset = this._tileset;
		var tileset = this._tileset; // eslint-disable-line no-unreachable

Fix clipping planes for tiles with RTC and/or no transforms #7034

Fix clipping planes for tiles with RTC and/or no transforms #7034

Conversation

OmarShehata commented Sep 11, 2018 • edited Loading

Tilesets with No Root Transforms

Tilesets with RTC Coordinates

cesium-concierge commented Sep 11, 2018

lilleyse commented Sep 11, 2018 • edited Loading

lilleyse commented Sep 12, 2018

OmarShehata commented Sep 14, 2018

pjcozzi commented Sep 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarShehata commented Sep 18, 2018

lilleyse commented Sep 19, 2018

lilleyse left a comment

Choose a reason for hiding this comment

lilleyse Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

lilleyse Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarShehata commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Sep 20, 2018

OmarShehata commented Sep 20, 2018

OmarShehata commented Sep 20, 2018

lilleyse commented Sep 21, 2018

OmarShehata commented Sep 21, 2018

lilleyse commented Sep 21, 2018

OmarShehata commented Sep 11, 2018 •

edited

Loading

lilleyse commented Sep 11, 2018 •

edited

Loading

lilleyse Sep 19, 2018 •

edited

Loading

lilleyse Sep 19, 2018 •

edited

Loading