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.

Sign up for GitHub

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

Use tileset bounding volume to position clipping plane #7182

Merged
merged 16 commits into from
Oct 31, 2018
Merged

Use tileset bounding volume to position clipping plane #7182

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
469afea
Take tileset model matrix into account for clipping planes
Oct 23, 2018
4d0d6b5
Fixed lint and updated changes
Oct 24, 2018
b82795a
Merge master
Oct 24, 2018
7228132
Only apply modelMatrix once, apply ENU
Oct 24, 2018
0ed3894
Set recompute matrix to true initially
Oct 25, 2018
618c936
Prevent crash if accessing getter before ready
Oct 25, 2018
44ab164
Only apply ENU when transform is identity
Oct 29, 2018
4b182ab
Add clipping planes tests
Oct 29, 2018
e99c7e3
Merge branch 'master' into clipping-model-matrix
OmarShehata Oct 29, 2018
d92a18c
Merge branch 'master' into clipping-model-matrix
OmarShehata Oct 30, 2018
7bca206
Clean up tests
Oct 30, 2018
26f5cab
Rename clippingPlaneOffsetMatrix to be more descriptive
Oct 30, 2018
aac739a
Use ellipsoid for getting height
Oct 30, 2018
07d8291
FInish clippingPlaneOffsetMatrix rename
Oct 30, 2018
2c26782
Fix lint
Oct 30, 2018
0c463a5
Rename _recomputeClippingPlaneMatrix
Oct 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ Change Log
* Added per-feature selection to the 3D Tiles BIM Sandcastle example. [#7181](https://github.com/AnalyticalGraphicsInc/cesium/pull/7181)

##### Fixes :wrench:
* Fixed issue removing geometry entities with different materials [#7163](https://github.com/AnalyticalGraphicsInc/cesium/pull/7163)
* Fixed issue removing geometry entities with different materials. [#7163](https://github.com/AnalyticalGraphicsInc/cesium/pull/7163)
* Fixed an issue where `pickPosition` would return incorrect results when called after `sampleHeight` or `clampToHeight`. [#7113](https://github.com/AnalyticalGraphicsInc/cesium/pull/7113)
* Fixed an issue where `sampleHeight` and `clampToHeight` would crash if picking a primitive that doesn't write depth. [#7120](https://github.com/AnalyticalGraphicsInc/cesium/issues/7120)
* Fixed crash when updating polyline attributes twice in one frame [#7155](https://github.com/AnalyticalGraphicsInc/cesium/pull/7155)
Expand All @@ -24,6 +24,7 @@ Change Log
* Fixed accuracy of rotation matrix generated by `VelocityOrientationProperty` [#6641](https://github.com/AnalyticalGraphicsInc/cesium/pull/6641)
* Fixed clipping plane crash when adding a plane to an empty collection. [#7168](https://github.com/AnalyticalGraphicsInc/cesium/pull/7168)
* Fixed texture coordinate calculation for polygon entities with `perPositionHeight` [#7188](https://github.com/AnalyticalGraphicsInc/cesium/pull/7188)
* Fixed clipping planes on tilesets not taking into account the tileset model matrix. [#7182](https://github.com/AnalyticalGraphicsInc/cesium/pull/7182)

### 1.50 - 2018-10-01

Expand Down
12 changes: 7 additions & 5 deletions Source/Scene/Batched3DModel3DTileContent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,12 +468,14 @@ define([

// Update clipping planes
var tilesetClippingPlanes = this._tileset.clippingPlanes;
if (this._tile.clippingPlanesDirty && defined(tilesetClippingPlanes)) {
if (defined(tilesetClippingPlanes)) {
this._model.clippingPlaneOffsetMatrix = this._tileset.clippingPlaneOffsetMatrix;
// Dereference the clipping planes from the model if they are irrelevant.
// Link/Dereference directly to avoid ownership checks.
// This will also trigger synchronous shader regeneration to remove or add the clipping plane and color blending code.
this._model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined;
if (this._tile.clippingPlanesDirty) {
// Dereference the clipping planes from the model if they are irrelevant.
// Link/Dereference directly to avoid ownership checks.
// This will also trigger synchronous shader regeneration to remove or add the clipping plane and color blending code.
this._model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined;
}
}

// If the model references a different ClippingPlaneCollection due to the tileset's collection being replaced with a
Expand Down
38 changes: 27 additions & 11 deletions Source/Scene/Cesium3DTileset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
define([
'../Core/ApproximateTerrainHeights',
'../Core/Cartesian2',
'../Core/Cartesian3',
'../Core/Cartographic',
Expand Down Expand Up @@ -42,6 +43,7 @@ define([
'./TileBoundingSphere',
'./TileOrientedBoundingBox'
], function(
ApproximateTerrainHeights,
Cartesian2,
Cartesian3,
Cartographic,
Expand Down Expand Up @@ -214,8 +216,9 @@ define([

this._ellipsoid = defaultValue(options.ellipsoid, Ellipsoid.WGS84);

this._useBoundingSphereForClipping = false;
this._clippingPlaneOffsetMatrix = undefined;
this._clippingPlaneOffsetMatrix = Matrix4.IDENTITY;
this._clippingPlaneTransformMatrix = undefined;
this._recomputeClippingPlaneMatrix = true;

/**
* Optimization option. Whether the tileset should refine based on a dynamic screen space error. Tiles that are further
Expand Down Expand Up @@ -739,10 +742,19 @@ define([
that._extensionsUsed = tilesetJson.extensionsUsed;
that._gltfUpAxis = gltfUpAxis;
that._extras = tilesetJson.extras;
if (!defined(tilesetJson.root.transform)) {
that._useBoundingSphereForClipping = true;
that._clippingPlaneOffsetMatrix = Transforms.eastNorthUpToFixedFrame(that.boundingSphere.center);
// Save the original, untransformed bounding volume position so we can apply
// the tile transform and model matrix at run time
var boundingVolume = that._root.createBoundingVolume(tilesetJson.root.boundingVolume, Matrix4.IDENTITY);
var clippingPlanesOrigin = boundingVolume.boundingSphere.center;
// If this origin is above the surface of the earth and root transform isn't identity
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@hpinkos hpinkos Oct 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (Matrix4.equals(that.root.transform, Matrix4.IDENTITY) && heightAboveEllipsoid > ApproximateTerrainHeights._defaultMinTerrainHeight) {
that._clippingPlaneOffsetMatrix = Transforms.eastNorthUpToFixedFrame(clippingPlanesOrigin);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

that._clippingPlaneTransformMatrix = Matrix4.clone(that._clippingPlaneOffsetMatrix);
that._readyPromise.resolve(that);
}).otherwise(function(error) {
that._readyPromise.reject(error);
Expand Down Expand Up @@ -1158,10 +1170,16 @@ define([
*/
clippingPlaneOffsetMatrix : {
get : function() {
if (this._useBoundingSphereForClipping) {
return this._clippingPlaneOffsetMatrix;
if (!defined(this._clippingPlaneTransformMatrix)) {
return Matrix4.IDENTITY;
}
return this.root.computedTransform;

if (this._recomputeClippingPlaneMatrix) {
Matrix4.multiply(this.root.computedTransform, this._clippingPlaneOffsetMatrix, this._clippingPlaneTransformMatrix);
this._recomputeClippingPlaneMatrix = false;
}

return this._clippingPlaneTransformMatrix;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
},

Expand Down Expand Up @@ -1899,11 +1917,9 @@ define([

// Update clipping planes
var clippingPlanes = this._clippingPlanes;
this._recomputeClippingPlaneMatrix = true;
if (defined(clippingPlanes) && clippingPlanes.enabled) {
clippingPlanes.update(frameState);
if (this._useBoundingSphereForClipping) {
this._clippingPlaneOffsetMatrix = Transforms.eastNorthUpToFixedFrame(this.boundingSphere.center);
}
}

this._timeSinceLoad = Math.max(JulianDate.secondsDifference(frameState.time, this._loadTimestamp) * 1000, 0.0);
Expand Down
12 changes: 7 additions & 5 deletions Source/Scene/Instanced3DModel3DTileContent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,11 +475,13 @@ define([
if (defined(model)) {
// Update for clipping planes
var tilesetClippingPlanes = this._tileset.clippingPlanes;
if (this._tile.clippingPlanesDirty && defined(tilesetClippingPlanes)) {
model.clippingPlaneOffsetMatrix = this._tileset.clippingPlaneOffsetMatrix;
// Dereference the clipping planes from the model if they are irrelevant - saves on shading
// Link/Dereference directly to avoid ownership checks.
model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined;
if (defined(tilesetClippingPlanes)) {
model.clippingPlaneOffsetMatrix = this._tileset.clippingPlaneOffsetMatrix;
if (this._tile.clippingPlanesDirty) {
// Dereference the clipping planes from the model if they are irrelevant - saves on shading
// Link/Dereference directly to avoid ownership checks.
model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined;
}
}

// If the model references a different ClippingPlaneCollection due to the tileset's collection being replaced with a
Expand Down
46 changes: 32 additions & 14 deletions Specs/Scene/Cesium3DTilesetSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3257,27 +3257,45 @@ defineSuite([
});
});

it('uses bounding sphere for clipping only if root has no transforms', function() {
return Cesium3DTilesTester.loadTileset(scene, tilesetOfTilesetsUrl).then(function(tileset) {
expect(tileset._useBoundingSphereForClipping).toBe(true);
it('clippingPlaneOffsetMatrix has correct orientation', function() {
return Cesium3DTilesTester.loadTileset(scene, withTransformBoxUrl).then(function(tileset) {
// The bounding volume of this tileset puts it under the surface, so no
// east-north-up should be applied. Check that it matches the orientation
// of the original transform.
var offsetMatrix = tileset.clippingPlaneOffsetMatrix;

expect(Matrix4.equals(offsetMatrix, tileset.root.computedTransform)).toBe(true);

return Cesium3DTilesTester.loadTileset(scene, withTransformBoxUrl).then(function(tileset) {
expect(tileset._useBoundingSphereForClipping).toBe(false);
return Cesium3DTilesTester.loadTileset(scene, tilesetUrl).then(function(tileset) {
// The bounding volume of this tileset puts it on the surface, and its
// root transform is identity, so we want to apply east-north-up as our best guess.
offsetMatrix = tileset.clippingPlaneOffsetMatrix;
expect(Matrix4.equals(tileset.root.transform, Matrix4.IDENTITY)).toBe(true);
// The clipping plane matrix is not the same as the original because we applied east-north-up.
expect(Matrix4.equals(offsetMatrix, tileset.root.computedTransform)).toBe(false);

// But they have the same translation.
var clippingPlanesOrigin = Matrix4.getTranslation(offsetMatrix, new Cartesian3());
expect(Cartesian3.equals(tileset.root.boundingSphere.center, clippingPlanesOrigin)).toBe(true);
});
});
});

it('correctly computes clippingPlaneOffsetMatrix', function() {
it('clippingPlaneOffsetMatrix matches root tile bounding sphere', function() {
return Cesium3DTilesTester.loadTileset(scene, tilesetOfTilesetsUrl).then(function(tileset) {
var offsetMatrix = tileset.clippingPlaneOffsetMatrix;
var boundingSphereMatrix = Transforms.eastNorthUpToFixedFrame(tileset.root.boundingSphere.center);
expect(Matrix4.equals(offsetMatrix,boundingSphereMatrix)).toBe(true);
var offsetMatrix = Matrix4.clone(tileset.clippingPlaneOffsetMatrix, new Matrix4());
var boundingSphereEastNorthUp = Transforms.eastNorthUpToFixedFrame(tileset.root.boundingSphere.center);
expect(Matrix4.equals(offsetMatrix, boundingSphereEastNorthUp)).toBe(true);

return Cesium3DTilesTester.loadTileset(scene, withTransformBoxUrl).then(function(tileset) {
offsetMatrix = tileset.clippingPlaneOffsetMatrix;
expect(Matrix4.equals(offsetMatrix,tileset.root.computedTransform)).toBe(true);
});
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

expect(Matrix4.equals(offsetMatrix, tileset.clippingPlaneOffsetMatrix)).toBe(false);

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

Choose a reason for hiding this comment

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

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

});
});

}, 'WebGL');