-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 clipping planes for tiles with RTC and/or no transforms #7034
Changes from 14 commits
ad7abcd
9f55539
9ed3695
bdb8799
6cb0426
aa71e96
0ec49a5
0a07e56
5aee22a
1f1cd16
8996da6
7d5bb57
b605a61
0d2dd5f
814c5ae
dd3a7c6
9db4500
2dac9f4
2fadcf9
b251c6b
e879cca
5245643
65a8496
3cc7aa0
888e2b2
f56c71c
911a520
a43ab57
79a49e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,11 +9,11 @@ define([ | |
'../Core/destroyObject', | ||
'../Core/DeveloperError', | ||
'../Core/FeatureDetection', | ||
'../Core/getBaseUri', | ||
'../Core/getStringFromTypedArray', | ||
'../Core/Matrix4', | ||
'../Core/RequestType', | ||
'../Core/RuntimeError', | ||
'../Core/Transforms', | ||
'../Renderer/Pass', | ||
'./Axis', | ||
'./Cesium3DTileBatchTable', | ||
|
@@ -33,11 +33,11 @@ define([ | |
destroyObject, | ||
DeveloperError, | ||
FeatureDetection, | ||
getBaseUri, | ||
getStringFromTypedArray, | ||
Matrix4, | ||
RequestType, | ||
RuntimeError, | ||
Transforms, | ||
Pass, | ||
Axis, | ||
Cesium3DTileBatchTable, | ||
|
@@ -355,10 +355,10 @@ define([ | |
primitive : tileset | ||
}; | ||
|
||
content._rtcCenterTransform = Matrix4.clone(Matrix4.IDENTITY); | ||
content._rtcCenterTransform = 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 = Matrix4.fromTranslation(Cartesian3.fromArray(rtcCenter)); | ||
} | ||
|
||
content._contentModelMatrix = Matrix4.multiply(tile.computedTransform, content._rtcCenterTransform, new Matrix4()); | ||
|
@@ -465,6 +465,11 @@ define([ | |
// Update clipping planes | ||
var tilesetClippingPlanes = this._tileset.clippingPlanes; | ||
if (this._tile.clippingPlanesDirty && defined(tilesetClippingPlanes)) { | ||
if (!this._tileset.root._useBoundingSphereForClipping) { | ||
this._model._clippingPlaneOffsetMatrix = this._tileset.root.computedTransform; | ||
} else { | ||
this._model._clippingPlaneOffsetMatrix = this._tileset.root._clippingPlaneOffsetMatrix; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be slightly cleaner to move this all to a getter property in E.g. |
||
// 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ define([ | |
'../Core/RequestType', | ||
'../Core/Resource', | ||
'../Core/RuntimeError', | ||
'../Core/Transforms', | ||
'../ThirdParty/when', | ||
'./Cesium3DTileContentFactory', | ||
'./Cesium3DTileContentState', | ||
|
@@ -60,6 +61,7 @@ define([ | |
RequestType, | ||
Resource, | ||
RuntimeError, | ||
Transforms, | ||
when, | ||
Cesium3DTileContentFactory, | ||
Cesium3DTileContentState, | ||
|
@@ -199,7 +201,6 @@ define([ | |
Cesium3DTile._deprecationWarning('contentUrl', 'This tileset JSON uses the "content.url" property which has been deprecated. Use "content.uri" instead.'); | ||
contentHeaderUri = contentHeader.url; | ||
} | ||
|
||
hasEmptyContent = false; | ||
contentState = Cesium3DTileContentState.UNLOADED; | ||
contentResource = baseResource.getDerivedResource({ | ||
|
@@ -333,6 +334,12 @@ define([ | |
this._priority = 0.0; | ||
this._isClipped = true; | ||
this._clippingPlanesState = 0; // encapsulates (_isClipped, clippingPlanes.enabled) and number/function | ||
this._useBoundingSphereForClipping = false; | ||
// If this is the root tile, and it doesn't have a defined transform, fall back to bounding sphere. | ||
if (!defined(this.parent) && !defined(this._header.transform)) { | ||
this._clippingPlaneOffsetMatrix = Transforms.eastNorthUpToFixedFrame(this.boundingSphere.center); | ||
this._useBoundingSphereForClipping = true; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems a little wasteful that In general we try to keep |
||
|
||
this._debugBoundingVolume = undefined; | ||
this._debugContentBoundingVolume = undefined; | ||
|
@@ -702,7 +709,6 @@ define([ | |
} | ||
|
||
var contentFailedFunction = getContentFailedFunction(this); | ||
|
||
promise.then(function(arrayBuffer) { | ||
if (that.isDestroyed()) { | ||
// Tile is unloaded before the content finishes loading | ||
|
@@ -822,6 +828,9 @@ define([ | |
var clippingPlanes = tileset.clippingPlanes; | ||
if (defined(clippingPlanes) && clippingPlanes.enabled) { | ||
var tileTransform = tileset.root.computedTransform; | ||
if(defined(tileset.root._clippingPlaneOffsetMatrix)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: add space after the |
||
tileTransform = tileset.root._clippingPlaneOffsetMatrix; | ||
} | ||
var intersection = clippingPlanes.computeIntersectionWithBoundingVolume(boundingVolume, tileTransform); | ||
this._isClipped = intersection !== Intersect.INSIDE; | ||
if (intersection === Intersect.OUTSIDE) { | ||
|
@@ -857,6 +866,9 @@ define([ | |
var clippingPlanes = tileset.clippingPlanes; | ||
if (defined(clippingPlanes) && clippingPlanes.enabled) { | ||
var tileTransform = tileset.root.computedTransform; | ||
if(defined(tileset.root._clippingPlaneOffsetMatrix)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same style comment. |
||
tileTransform = tileset.root._clippingPlaneOffsetMatrix; | ||
} | ||
var intersection = clippingPlanes.computeIntersectionWithBoundingVolume(boundingVolume, tileTransform); | ||
this._isClipped = intersection !== Intersect.INSIDE; | ||
if (intersection === Intersect.OUTSIDE) { | ||
|
@@ -1051,6 +1063,9 @@ define([ | |
this._viewerRequestVolume = this.createBoundingVolume(header.viewerRequestVolume, this.computedTransform, this._viewerRequestVolume); | ||
} | ||
|
||
if (this._useBoundingSphereForClipping) { | ||
this._clippingPlaneOffsetMatrix = Transforms.eastNorthUpToFixedFrame(this.boundingSphere.center); | ||
} | ||
// Destroy the debug bounding volumes. They will be generated fresh. | ||
this._debugBoundingVolume = this._debugBoundingVolume && this._debugBoundingVolume.destroy(); | ||
this._debugContentBoundingVolume = this._debugContentBoundingVolume && this._debugContentBoundingVolume.destroy(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,13 @@ define([ | |
/** | ||
* Specifies a set of clipping planes. Clipping planes selectively disable rendering in a region on the | ||
* outside of the specified list of {@link ClippingPlane} objects for a single gltf model, 3D Tileset, or the globe. | ||
* <p> | ||
* In general the clipping planes' coordinates are relative to the object they're attached to, so a plane with distance set to 0 will clip | ||
* through the center of the object. | ||
* </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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also add a similar note explaining what the clipping plane's coordinate system is when using it on a model and on the globe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright makes sense. |
||
* | ||
* @alias ClippingPlaneCollection | ||
* @constructor | ||
|
@@ -66,6 +73,30 @@ define([ | |
* @param {Boolean} [options.unionClippingRegions=false] If true, a region will be clipped if included in any plane in the collection. Otherwise, the region to be clipped must intersect the regions defined by all planes in this collection. | ||
* @param {Color} [options.edgeColor=Color.WHITE] The color applied to highlight the edge along which an object is clipped. | ||
* @param {Number} [options.edgeWidth=0.0] The width, in pixels, of the highlight applied to the edge along which an object is clipped. | ||
* | ||
* @demo {@link https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=3D%20Tiles%20Clipping%20Planes.html|Clipping 3D Tiles and glTF models.} | ||
* @demo {@link https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=Terrain%20Clipping%20Planes.html|Clipping the Globe.} | ||
* | ||
* @example | ||
* // This clipping plane's distance is positive, which means its normal | ||
* // is facing the origin. This will clip everything that is behind | ||
* // the plane, which is all pixels with y > 5. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I initially said something like "all points with y > 5" but that to me implies like clipping vertices, and saying "objects" implies whole objects are hidden/shown. "Pixels" is really what gets clipped, but I changed it to "anything with y coordinate > 5" since all we want to communicate here is in what direction the clipping happens. |
||
* var clippingPlanes = new Cesium.ClippingPlaneCollection({ | ||
* planes : [ | ||
* new Cesium.ClippingPlane(new Cesium.Cartesian3(0.0, 1.0, 0.0), 5.0) | ||
* ], | ||
* }); | ||
* // Create an entity and attach the ClippingPlaneCollection to the model. | ||
* var entity = viewer.entities.add({ | ||
* position : Cesium.Cartesian3.fromDegrees(-123.0744619, 44.0503706, 10000), | ||
* model : { | ||
* uri : '../../../../Apps/SampleData/models/CesiumAir/Cesium_Air.glb', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
* minimumPixelSize : 128, | ||
* maximumScale : 20000, | ||
* clippingPlanes : clippingPlanes | ||
* } | ||
* }); | ||
* viewer.zoomTo(entity); | ||
*/ | ||
function ClippingPlaneCollection(options) { | ||
options = defaultValue(options, defaultValue.EMPTY_OBJECT); | ||
|
@@ -587,7 +618,7 @@ define([ | |
|
||
var modelMatrix = this.modelMatrix; | ||
if (defined(transform)) { | ||
modelMatrix = Matrix4.multiply(modelMatrix, transform, scratchMatrix); | ||
modelMatrix = Matrix4.multiply(transform, modelMatrix, scratchMatrix); | ||
} | ||
|
||
// If the collection is not set to union the clipping regions, the volume must be outside of all planes to be | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -539,6 +539,10 @@ define([ | |
this.clippingPlanes = options.clippingPlanes; | ||
// Used for checking if shaders need to be regenerated due to clipping plane changes. | ||
this._clippingPlanesState = 0; | ||
// 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Slight wording tweak |
||
|
||
/** | ||
* This property is for debugging only; it is not for production use nor is it optimized. | ||
|
@@ -4426,7 +4430,11 @@ define([ | |
var clippingPlanes = this._clippingPlanes; | ||
var currentClippingPlanesState = 0; | ||
if (defined(clippingPlanes) && clippingPlanes.enabled) { | ||
Matrix4.multiply(context.uniformState.view3D, modelMatrix, this._modelViewMatrix); | ||
if (!defined(this._clippingPlaneOffsetMatrix)) { | ||
Matrix4.multiply(context.uniformState.view3D, modelMatrix, this._modelViewMatrix); | ||
} else { | ||
Matrix4.multiply(context.uniformState.view3D, this._clippingPlaneOffsetMatrix, this._modelViewMatrix); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
currentClippingPlanesState = clippingPlanes.clippingPlanesState; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here. |
||
this._clippingPlaneOffsetMatrix = undefined; | ||
|
||
this.attenuation = false; | ||
this._attenuation = false; | ||
|
@@ -819,8 +823,13 @@ define([ | |
if (!defined(clippingPlanes)) { | ||
return Matrix4.IDENTITY; | ||
} | ||
var modelViewMatrix = Matrix4.multiply(context.uniformState.view3D, pointCloud._modelMatrix, scratchClippingPlaneMatrix); | ||
return Matrix4.multiply(modelViewMatrix, clippingPlanes.modelMatrix, scratchClippingPlaneMatrix); | ||
|
||
if (!defined(pointCloud._clippingPlaneOffsetMatrix)) { | ||
Matrix4.multiply(context.uniformState.view3D, pointCloud._modelMatrix, scratchClippingPlaneMatrix); | ||
} else { | ||
Matrix4.multiply(context.uniformState.view3D, pointCloud._clippingPlaneOffsetMatrix, scratchClippingPlaneMatrix); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
return scratchClippingPlaneMatrix; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may explain the failing test: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's my bad! That shouldn't have been removed. Thanks for catching this. |
||
} | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense too. And I remember now that the terrain example uses a model matrix, so one example is enough to illustrate the point.