Use tileset bounding volume to position clipping plane #7182

OmarShehata · 2018-10-24T13:13:45Z

Clipping planes currently do not take into account the tileset's model matrix.

In master, moving this tileset up and down makes it go in and out of the clipping plane. The correct behavior is that the clipping plane should move with the tileset since its coordinates should always be relative to what it's attached to.

The original solution was to set the clipping plane's position to the root tile's. But sometimes the root tile's transform isn't defined. In those cases, we used the bounding sphere center as a fallback. This meant that if you had a root transform defined as the identity, the behavior would change. This is bad because the spec says that an undefined transform should be treated as identity.

This solution simply just uses the root tile's bounding volume, which is always defined (and this is what is used to compute the bounding sphere). The model matrix is then applied as an additional transform. This simplifies the positioning so there are no special cases.

@lilleyse the only thing I'm unsure about is that I'm applying the model matrix in the getter of clippingPlaneOffsetMatrix. Applying it in the tileset's update function makes it lag one frame behind as the model matrix changes.

cesium-concierge · 2018-10-24T13:13:46Z

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.

🌍 🌎 🌏

hpinkos · 2018-10-24T14:22:41Z

@OmarShehata merge in master

OmarShehata · 2018-10-25T15:01:36Z

Thanks for the reminder @hpinkos !

@lilleyse I'm fairly certain this new approach is a lot more solid. The only guesswork involved now is whether or not the center of the untransformed bounding volume is on the center of the Earth (which I compute by comparing it against the minimum known terrain height). If it is, I apply ENU, otherwise the assumption is that the combination of the user provided root tile transform and model matrix will orient the tileset the way the user wants.

The root tile computed transform and the model matrix are applied at run time, since those may change at any time.

I would consider this a fix and not a breaking change. I believe in master if you set the root tile transform to identity, it will have a different effect from leaving it undefined, which should no longer be the case.

If this looks good I just need to:

Update failing tests that check for _useBoundingSphereForClipping which we don't use anymore.
Write test(s) for the clipping planes correctly applying the tileset model matrix.
Update changes.md.

lilleyse · 2018-10-25T19:55:48Z

The approach looks good. I'll do a full review once everything's ready.

OmarShehata · 2018-10-29T15:42:50Z

@lilleyse I added some robust tests. One new thing that came up while writing tests was that it would apply an ENU orientation if the bounding volume center was on the surface, regardless of whether a root tile transform is defined.

I think it makes sense to only do ENU if there is no defined transform and it's determined to be on the surface, so that's what I updated it to. The tests have a lot of comments because I think it requires understanding the context behind this expected behavior.

This should be ready to review.

lilleyse

The approach looks good. Just one comment about a potential edge case.

lilleyse · 2018-10-30T13:30:45Z

Specs/Scene/Cesium3DTilesetSpec.js

+            // Changing the model matrix should change the clipping planes matrix
+            tileset.modelMatrix = Matrix4.fromTranslation(new Cartesian3(100, 0, 0));
+            scene.renderForSpecs();
+            tileset.update(scene.frameState);


tileset.update(scene.frameState); is redundant. It gets called in scene.renderForSpecs().

lilleyse · 2018-10-30T13:43:27Z

Source/Scene/Cesium3DTileset.js

+                // we want to apply an ENU orientation as our best guess of orientation.
+                // Otherwise, we assume it gets its position/orientation completely from the
+                // root tile transform and the tileset's model matrix
+                var heightAboveEllipsoid = Cartographic.fromCartesian(clippingPlanesOrigin).height;


It's usually a good idea to set the ellipsoid parameter in engine code in case non-standard ellipsoids are being used.

I can see that other parts of the engine do this, but doesn't it make sense to use the actual ellipsoid that's being used instead of the default one? For example, if you were using the moon, or something much smaller, using that ellipsoid here would work perfectly whereas it would no longer be a correct measure of above/below the surface with the default ellipsoid.

Also, it looks like it already uses WGS84 in this case if ellipsoid isn't passed as a parameter in fromCartesian, so that should be the correct behavior right?

Always pass in the ellipsoid. We have a bunch of bugs for non-earth ellipsoids because (especially at the entity layer) we aren't passing the right ellipsoid around. I recommend not using these Cartographic.fromCartesian, Cartographic.toCartesian, Cartesian3.fromDegrees helper functions in code we develop because while they're convenient for our end users, it's easy to forget to pass in the ellipsoid. I always use ellipsoid.cartesianToCartographic and ellipsoid.cartographicToCartesian.

Oh, I think I misunderstood. So we do want to pass in the ellipsoid the user is using. Thanks for clarifying that!

lilleyse · 2018-10-30T14:11:23Z

Source/Scene/Cesium3DTileset.js

+                // root tile transform and the tileset's model matrix
+                var heightAboveEllipsoid = Cartographic.fromCartesian(clippingPlanesOrigin).height;
+                if (Matrix4.equals(that.root.transform, Matrix4.IDENTITY) && heightAboveEllipsoid > ApproximateTerrainHeights._defaultMinTerrainHeight) {
+                    that._clippingPlaneOffsetMatrix = Transforms.eastNorthUpToFixedFrame(clippingPlanesOrigin);
                }


I don't think it's necessary to check that the root transform is identity since the untransformed bounding sphere's height should be independent of that.

The main case I'm worried about is someone has a tileset like NYC, realizes they need to adjust the height slightly, then saves that transform into the tileset.json.

If this is to get around the case where a region uses it's initial transform in the calculation, and therefore passing in identity produces an incorrect result, it can be fixed be either adding a new function like Cesium3DTile.prototype.createUntransformedBoundingVolume (or better name) that passes in the right transform to createBoundingVolume based on the bounding volume type (identity for box/sphere and initial transform for region), or treat the identity matrix as a special case in createRegion.

It was mostly as a matter of principle - the reason we apply ENU is because we don't know what the orientation should be, so it's a guess. If an orientation is defined in the form of a non-identity matrix, then we should respect that.

I could instead check if the orientation extracted from that matrix is non-default though. I wasn't planning on accounting for that region case in this PR.

I think it would be fine to simply remove this identity check then. If you do have a tileset that's on the surface, oriented in a non-standard way, the clipping plane will be oriented ENU, but the user can then override that with the clipping planes model matrix.

How does that sound?

Sounds good.

lilleyse · 2018-10-30T14:14:33Z

Source/Scene/Cesium3DTileset.js

+                    this._recomputeClippingPlaneMatrix = false;
+                }
+
+                return this._clippingPlaneTransformMatrix;


The naming is a little confusing. What is the difference between clippingPlaneOffsetMatrix and clippingPlaneTransformMatrix, and why is clippingPlaneTransformMatrix returned for a getter called clippingPlaneOffsetMatrix. See if there's a more descriptive name for what is _clippingPlaneOffsetMatrix.

lilleyse · 2018-10-30T14:16:27Z

Specs/Scene/Cesium3DTilesetSpec.js

+
+            boundingSphereEastNorthUp = Transforms.eastNorthUpToFixedFrame(tileset.root.boundingSphere.center);
+            offsetMatrix = tileset.clippingPlaneOffsetMatrix;
+            expect(Matrix4.equalsEpsilon(offsetMatrix, boundingSphereEastNorthUp, CesiumMath.EPSILON3)).toBe(true);


This can be simplified to expect(offsetMatrix).toEqualEpsilon(boundingSphereEastNorthUp, CesiumMath.EPSILON3);

OmarShehata · 2018-10-30T16:31:24Z

Aside from #7182 (comment) (for which I removed the identity check), this should be all good. I renamed the clippingPlaneOffsetMatrix here 26f5cab. I changed the getter name as well, it was initially "offset" because the very first solution was to apply an offset correct to the clipping planes, but what this matrix really is a transformation of the origin & reference frame of the clipping planes. The "_initial" is the untransformed one, which matches the same naming that is used elsewhere.

Since this is private this rename need not be mentioned in CHANGES.md.

I've confirmed that the only failing test when running locally is the one documented here failing in master #7205.

hpinkos · 2018-10-30T16:35:15Z

I feel like we're trying to be too clever here. The end user is responsible for creating the ClippingPlaneCollection and passing it into the Cesium3DTileset. I feel like it's the end user's responsibility to make sure the ClippingPlaneCollection has the correct model matrix instead of trying to guess if it's East/North/Up depending on the height of the tileset. We might be making that guess correctly for the kinds of sample tilesets we use frequently, but it may not work that way for the various types of tilesets our end users are generating.

Are we sure this shouldn't just be a documentation change or a better sandcastle example?

OmarShehata · 2018-10-30T17:53:13Z

@hpinkos I think it's just a matter of making the most common case easier. Not doing the ENU means all cases of the tileset on the surface without a root transform will "look wrong" and the user will have to figure out how to orient it. But it will work correctly if the tileset has a root transform that orients it.

Doing the ENU will at least have an orientation that looks like it's aligned with the ground/environment in the first case above, and in the second case, it actually won't apply the ENU if the root transform is what positions and orients it on the surface (since the untransformed bounding volume will be below the height of the surface then).

I think more people use clipping planes without explicitly setting a model matrix for them, and I think that makes them easier to use.

hpinkos · 2018-10-30T18:08:45Z

Yeah sorry, I don't agree with you @OmarShehata. If the user is able to set the tileset.modelMatrix they should just as easily be able to set clippingPlanes.modelMatrix to the same thing. As far as I know tileset.modelMatrix isn't set anywhere automatically behind the scenes.

I know the motivation for this was for our one widget, and this seems like something that should be handled at the widget level.

lilleyse · 2018-10-30T18:59:09Z

@hpinkos As it stands now a clipping plane collection is defined as relative to the coordinate system of the object it's attached to, so I think we'd need to consider making a breaking change to make the collection completely independent of the tileset. I'm not sure if your suggestion is to go that far, or just to not guess the tileset's coordinate system if its geometry is baked in WGS84.

OmarShehata · 2018-10-30T19:49:15Z

@hpinkos the user would not be setting tileset.modelMatrix. In the first case I described, the tileset is on the surface either because the geometry itself has the surface position or it's using something like the RTC extension. In these cases, the user just adds a tileset, and then adds a clipping plane collection, perhaps with just one clipping plane with a normal of (1, 0, 0). I think it would be strange to see it clipping at an angle not aligned with the globe, and I can see people thinking it's a bug.

hpinkos · 2018-10-31T14:50:14Z

Eh, I don't know. I still feel weird about this change but I guess the code there's no good way for the end user to figure out the transform themselves. @lilleyse if you think this is the right thing to do then it's okay by me.

OmarShehata · 2018-10-31T15:01:51Z

Yeah, I wanted to add, I don't believe "guessing" is a good strategy in general, but I don't know a better way to make the common case "just work" the users expect.

lilleyse · 2018-10-31T17:05:33Z

Yeah it's not ideal but it's the right thing to do for ease of use.

The recent code edits look good. Thanks @OmarShehata.

Take tileset model matrix into account for clipping planes

469afea

OmarShehata mentioned this pull request Oct 24, 2018

Create function to update tileset clipping plane matrix #7177

Closed

Fixed lint and updated changes

4d0d6b5

Omar Shehata added 4 commits October 24, 2018 15:50

Merge master

b82795a

Only apply modelMatrix once, apply ENU

7228132

Set recompute matrix to true initially

0ed3894

Prevent crash if accessing getter before ready

618c936

Shehata and others added 3 commits October 29, 2018 09:54

Only apply ENU when transform is identity

44ab164

Add clipping planes tests

4b182ab

Merge branch 'master' into clipping-model-matrix

e99c7e3

lilleyse reviewed Oct 30, 2018

View reviewed changes

OmarShehata and others added 5 commits October 30, 2018 11:47

Merge branch 'master' into clipping-model-matrix

d92a18c

Clean up tests

7bca206

Rename clippingPlaneOffsetMatrix to be more descriptive

26f5cab

Use ellipsoid for getting height

aac739a

FInish clippingPlaneOffsetMatrix rename

07d8291

Fix lint

2c26782

Rename _recomputeClippingPlaneMatrix

0c463a5

lilleyse merged commit eb5a98c into CesiumGS:master Oct 31, 2018

lilleyse mentioned this pull request Nov 1, 2018

Check if cartographic is defined in clipping plane setup #7216

Merged

OmarShehata mentioned this pull request Aug 12, 2019

Why is the clipping plane different from the actual cut #8053

Closed

OmarShehata mentioned this pull request Nov 14, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use tileset bounding volume to position clipping plane #7182

Use tileset bounding volume to position clipping plane #7182

OmarShehata commented Oct 24, 2018

cesium-concierge commented Oct 24, 2018

hpinkos commented Oct 24, 2018

OmarShehata commented Oct 25, 2018 •

edited

Loading

lilleyse commented Oct 25, 2018

OmarShehata commented Oct 29, 2018

lilleyse left a comment

lilleyse Oct 30, 2018

lilleyse Oct 30, 2018

OmarShehata Oct 30, 2018

OmarShehata Oct 30, 2018

hpinkos Oct 30, 2018 •

edited

Loading

OmarShehata Oct 30, 2018

lilleyse Oct 30, 2018

OmarShehata Oct 30, 2018

lilleyse Oct 30, 2018

lilleyse Oct 30, 2018

lilleyse Oct 30, 2018

OmarShehata commented Oct 30, 2018

hpinkos commented Oct 30, 2018 •

edited

Loading

OmarShehata commented Oct 30, 2018

hpinkos commented Oct 30, 2018

lilleyse commented Oct 30, 2018

OmarShehata commented Oct 30, 2018

hpinkos commented Oct 31, 2018

OmarShehata commented Oct 31, 2018

lilleyse commented Oct 31, 2018

Use tileset bounding volume to position clipping plane #7182

Use tileset bounding volume to position clipping plane #7182

Conversation

OmarShehata commented Oct 24, 2018

cesium-concierge commented Oct 24, 2018

hpinkos commented Oct 24, 2018

OmarShehata commented Oct 25, 2018 • edited Loading

lilleyse commented Oct 25, 2018

OmarShehata commented Oct 29, 2018

lilleyse left a 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

hpinkos Oct 30, 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

OmarShehata commented Oct 30, 2018

hpinkos commented Oct 30, 2018 • edited Loading

OmarShehata commented Oct 30, 2018

hpinkos commented Oct 30, 2018

lilleyse commented Oct 30, 2018

OmarShehata commented Oct 30, 2018

hpinkos commented Oct 31, 2018

OmarShehata commented Oct 31, 2018

lilleyse commented Oct 31, 2018

OmarShehata commented Oct 25, 2018 •

edited

Loading

hpinkos Oct 30, 2018 •

edited

Loading

hpinkos commented Oct 30, 2018 •

edited

Loading