-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Use tileset bounding volume to position clipping plane #7182
Changes from 12 commits
469afea
4d0d6b5
b82795a
7228132
0ed3894
618c936
44ab164
4b182ab
e99c7e3
d92a18c
7bca206
26f5cab
aac739a
07d8291
2c26782
0c463a5
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 |
---|---|---|
@@ -1,4 +1,5 @@ | ||
define([ | ||
'../Core/ApproximateTerrainHeights', | ||
'../Core/Cartesian2', | ||
'../Core/Cartesian3', | ||
'../Core/Cartographic', | ||
|
@@ -42,6 +43,7 @@ define([ | |
'./TileBoundingSphere', | ||
'./TileOrientedBoundingBox' | ||
], function( | ||
ApproximateTerrainHeights, | ||
Cartesian2, | ||
Cartesian3, | ||
Cartographic, | ||
|
@@ -214,8 +216,9 @@ define([ | |
|
||
this._ellipsoid = defaultValue(options.ellipsoid, Ellipsoid.WGS84); | ||
|
||
this._useBoundingSphereForClipping = false; | ||
this._clippingPlaneOffsetMatrix = undefined; | ||
this._initialClippingPlanesOriginMatrix = Matrix4.IDENTITY; // Computed from the tileset JSON. | ||
this._clippingPlanesOriginMatrix = undefined; // Combines the above with any run-time transforms. | ||
this._recomputeClippingPlaneMatrix = true; | ||
|
||
/** | ||
* Optimization option. Whether the tileset should refine based on a dynamic screen space error. Tiles that are further | ||
|
@@ -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 | ||
// 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; | ||
if (heightAboveEllipsoid > ApproximateTerrainHeights._defaultMinTerrainHeight) { | ||
that._initialClippingPlanesOriginMatrix = Transforms.eastNorthUpToFixedFrame(clippingPlanesOrigin); | ||
} | ||
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 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 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 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? 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. Sounds good. |
||
that._clippingPlanesOriginMatrix = Matrix4.clone(that._initialClippingPlanesOriginMatrix); | ||
that._readyPromise.resolve(that); | ||
}).otherwise(function(error) { | ||
that._readyPromise.reject(error); | ||
|
@@ -1156,12 +1168,18 @@ define([ | |
/** | ||
* @private | ||
*/ | ||
clippingPlaneOffsetMatrix : { | ||
clippingPlanesOriginMatrix : { | ||
get : function() { | ||
if (this._useBoundingSphereForClipping) { | ||
return this._clippingPlaneOffsetMatrix; | ||
if (!defined(this._clippingPlanesOriginMatrix)) { | ||
return Matrix4.IDENTITY; | ||
} | ||
return this.root.computedTransform; | ||
|
||
if (this._recomputeClippingPlaneMatrix) { | ||
Matrix4.multiply(this.root.computedTransform, this._initialClippingPlanesOriginMatrix, this._clippingPlanesOriginMatrix); | ||
this._recomputeClippingPlaneMatrix = false; | ||
} | ||
|
||
return this._clippingPlanesOriginMatrix; | ||
} | ||
}, | ||
|
||
|
@@ -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); | ||
|
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.
It's usually a good idea to set the
ellipsoid
parameter in engine code in case non-standard ellipsoids are being used.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 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.
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.
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?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.
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 useellipsoid.cartesianToCartographic
andellipsoid.cartographicToCartesian
.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.
Oh, I think I misunderstood. So we do want to pass in the ellipsoid the user is using. Thanks for clarifying that!