Tileset traversal with clipping planes #5966

ggetz · 2017-11-03T18:02:45Z

Added clipping planes tileset traversal optimization in visibility and contentVisibility
Added show debug boundingVolumes options to sandcastle example
Moved some duplicate logic to the Plane class
Updated shader to reflect math in Plane

Part of #5765

cesium-concierge · 2017-11-03T18:02:46Z

@ggetz, thanks for the pull request! Maintainers, we have a signed CLA from @ggetz, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

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

🌍 🌎 🌏

lilleyse

This seems to work well when running in Sandcastle. I have some general architecture comments.

lilleyse · 2017-11-06T15:49:58Z

Source/Scene/Cesium3DTile.js

@@ -940,7 +966,7 @@ define([
 var showVolume = tileset.debugShowBoundingVolume || (tileset.debugShowContentBoundingVolume && !hasContentBoundingVolume);
 if (showVolume) {
 if (!defined(tile._debugBoundingVolume)) {
- var color = tile._finalResolution ? (hasContentBoundingVolume ? Color.WHITE : Color.RED) : Color.YELLOW;
+ var color = tile._finalResolution ? (hasContentBoundingVolume ? (tile._debugClipped ? Color.CYAN : Color.WHITE) : Color.RED) : Color.YELLOW;


If the tile is clipped won't its debug bounding volume not be visible?

That's true, I was using this for my own debugging purposes, I'll remove this and the _debugClipped property.

lilleyse · 2017-11-06T15:51:38Z

Source/Scene/PointCloud3DTileContent.js

- Cartesian3.multiplyByScalar(plane.normal, plane.distance, scratchCartesian);
- Matrix4.multiplyByPoint(context.uniformState.modelView3D, scratchCartesian, scratchCartesian);
- packedPlane.w = Cartesian3.dot(packedPlane, scratchCartesian);
+ Matrix4.multiply(context.uniformState.view3D, content._modelMatrix, scratchMatrix);


Following Model, computing the scratch matrix could go outside the for loop as well.

lilleyse · 2017-11-06T15:56:59Z

Source/Scene/Cesium3DTile.js

+ var length = planes.length;
+ for (var i = 0; i < length; ++i) {
+ var plane = planes[i];
+ Plane.transformPlane(plane, tile._tileset._root.computedTransform, scratchPlane);


I think this should be tile._computedTransform instead.

No, this should definitely be the root transform. The plane is defined relative to the tileset, not each individual tile.

I tested both on the NY tileset as well.

But let me know if there is a cleaner way to get this value

lilleyse · 2017-11-06T16:03:26Z

Source/Core/CullingVolume.js

 * @returns {Number} A plane mask as described above (which can be applied to this boundingVolume's children).
 *
 * @private
 */
- CullingVolume.prototype.computeVisibilityWithPlaneMask = function(boundingVolume, parentPlaneMask) {
+ CullingVolume.prototype.computeVisibilityWithPlaneMask = function(boundingVolume, parentPlaneMask, checkClipped) {


This callback doesn't really belong in CullingVolume. The logic of whether to proceed with checking the clipping planes should exist in Cesium3DTile.

lilleyse · 2017-11-06T16:04:19Z

Source/Core/Plane.js

+ * @param {Plane} [result] The object into which to store the result.
+ * @returns {Plane} The plane transformed by the given transformation matrix.
+ */
+ Plane.transformPlane = function(plane, transform, result) {


Make sure to write tests for this once you start writing tests.

Also since this is a new thing it should be added to CHANGES.md, unless this is better off private.

Just name this transform since Plane is the class name?

lilleyse · 2017-11-06T16:05:32Z

Source/Core/Plane.js

+ if (!defined(transform)) {
+ throw new DeveloperError('transform is required.');
+ }
+ //>>includeEnd('debug');


Everything in this file should move over to the Check syntax.

lilleyse · 2017-11-06T16:07:03Z

Source/Scene/Cesium3DTile.js

+ var plane = planes[i];
+ Plane.transformPlane(plane, tile._tileset._root.computedTransform, scratchPlane);
+ if (boundingVolume.intersectPlane(scratchPlane) === Intersect.INSIDE) {
+ tile._debugClipped = true;


I'm not sure why this works... Is Intersect.INSIDE the result if the contents are completely on the opposite side of the clipping plane? Does this mean the normal is reverse what it should be?

I wonder if the tileset should contain a culling volume built from its clipping planes. The check here should be functionally the same as CullingVolume.computeVisibility, so it would be better to reuse that code.

I'm not sure why this works... Is Intersect.INSIDE the result if the contents are completely on the opposite side of the clipping plane? Does this mean the normal is reverse what it should be?

I think I assumed we wanted to clip what was on the "inside" of the plane. If we want the "outside" to be clipped, it should be fairly easy to update the shader to clip the opposite of what we are now, and check for outside intersections instead.

I wonder if the tileset should contain a culling volume built from its clipping planes. The check here should be functionally the same as CullingVolume.computeVisibility, so it would be better to reuse that code.

I think the only difference is that we only care if the volume is inside (or outside after the above changes)

Actually, another difference is that CullingVolume uses the Cartesian4 representation for the coefficients of the plane equation to calculate intersections, rather than a Plane object with normal/distance. So I'm going to go with no clipping volume unless we want to convert the planes to their equations everytime we update.

ggetz · 2017-11-06T20:00:33Z

@lilleyse Thanks for taking a look! Updated.

pjcozzi · 2017-11-06T22:48:36Z

Source/Scene/Cesium3DTile.js

+ var length = planes.length;
+ for (var i = 0; i < length; ++i) {
+ var plane = planes[i];
+ Plane.transformPlane(plane, tile._tileset._root.computedTransform, scratchPlane);


Outside of the loop:

var rootTransform = tile._tileset._root.computedTransform;

pjcozzi · 2017-11-06T22:49:45Z

Source/Scene/Cesium3DTile.js

@@ -747,6 +768,11 @@ define([
 Cesium3DTile.prototype.visibility = function(frameState, parentVisibilityPlaneMask) {
 var cullingVolume = frameState.cullingVolume;
 var boundingVolume = getBoundingVolume(this, frameState);
+
+ if (checkTileClipped(this, boundingVolume)) {


Part of this optimization is to use the original shader when the tile is not clipped. Does this PR do that?

ggetz · 2017-11-08T15:51:47Z

Thanks @pjcozzi, @lilleyse

Updated to include the additional optimization.

Also included fixes from discussion in my previous PR: #5913 (comment)

pjcozzi · 2017-11-08T21:34:32Z

Thanks @ggetz!

@lilleyse please merge when ready.

ggetz · 2017-11-17T20:16:47Z

@lilleyse Can you take a look at this? I have a successive branch with all the additional changes.

lilleyse

I'll review the second branch more closely. It's also possible some of the issues I'm seeing have been fixed there.

lilleyse · 2017-11-08T22:35:07Z

Source/Scene/Model.js


+ var scratchPlane = new Plane(Cartesian3.UNIT_X, 0.0);
 function createClippingPlanesFunction(model, context) {


context isn't used any more.

lilleyse · 2017-11-08T22:36:49Z

Apps/Sandcastle/gallery/Clipping Planes.html

@@ -44,25 +44,27 @@
 <tr>


I'm seeing a crash when zooming into the BIM tileset.

This is fixed in the next branch.

lilleyse · 2017-11-08T22:37:08Z

Apps/Sandcastle/gallery/Clipping Planes.html

@@ -44,25 +44,27 @@
 <tr>
 <td>x</td>


The point cloud tileset doesn't seem to show up for me, even after playing around with the sliders.

Noted, but I think it will be easier to fix this in the next branch as I have added easier ways to handle this.

lilleyse · 2017-11-21T01:13:08Z

Source/Scene/Cesium3DTileset.js

+ * Optimization option. If set to false, the tileset will not perform clipping operations.
+ *
+ * @type {Boolean}
+ * @default {undefined}


Default is true

lilleyse · 2017-11-21T01:13:36Z

Source/Scene/Model.js

 *
- * @default []
+ * @default false


Default is true

ggetz · 2017-11-21T14:49:24Z

Thanks @lilleyse, updated!

Tileset traversal with clipping planes

271dade

lilleyse reviewed Nov 6, 2017

View reviewed changes

ggetz added 3 commits November 6, 2017 12:30

Cleanup debug, use 'Check' in Plane, update CHANGES.md

5bd4ad6

Cleanup CullingVolume changes

ad4f9e5

Flip 'inside' and 'outside' for clipping

02bdf52

pjcozzi reviewed Nov 6, 2017

View reviewed changes

ggetz added 4 commits November 7, 2017 16:25

Disable clipping in shader when tile will not be clipped

072400c

Doc fixes and sandcastle updates

8330177

Doc udates

45a48d0

Allow undefined clipping planes, updated shader

a7fe4db

ggetz added 2 commits November 8, 2017 11:01

Refactor name

f1197cd

Performance fixes

e161b31

lilleyse reviewed Nov 21, 2017

View reviewed changes

Cleanup

32b01c1

lilleyse merged commit 261d0fc into CesiumGS:clip-planes-master Nov 21, 2017

ggetz deleted the clipping-planes-tileset-traversal branch November 21, 2017 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tileset traversal with clipping planes #5966

Tileset traversal with clipping planes #5966

ggetz commented Nov 3, 2017 •

edited

Loading

cesium-concierge commented Nov 3, 2017

lilleyse left a comment

lilleyse Nov 6, 2017

ggetz Nov 6, 2017

lilleyse Nov 6, 2017

lilleyse Nov 6, 2017

ggetz Nov 6, 2017

ggetz Nov 6, 2017

lilleyse Nov 6, 2017

lilleyse Nov 6, 2017

lilleyse Nov 6, 2017

pjcozzi Nov 6, 2017

lilleyse Nov 6, 2017

lilleyse Nov 6, 2017

lilleyse Nov 6, 2017

ggetz Nov 6, 2017

ggetz Nov 6, 2017

ggetz Nov 6, 2017

ggetz commented Nov 6, 2017

pjcozzi Nov 6, 2017

pjcozzi Nov 6, 2017

ggetz commented Nov 8, 2017

pjcozzi commented Nov 8, 2017

ggetz commented Nov 17, 2017

lilleyse left a comment

lilleyse Nov 8, 2017

lilleyse Nov 8, 2017

ggetz Nov 21, 2017

lilleyse Nov 8, 2017

ggetz Nov 21, 2017

lilleyse Nov 21, 2017

lilleyse Nov 21, 2017

ggetz commented Nov 21, 2017


		var scratchPlane = new Plane(Cartesian3.UNIT_X, 0.0);
		function createClippingPlanesFunction(model, context) {

Tileset traversal with clipping planes #5966

Tileset traversal with clipping planes #5966

Conversation

ggetz commented Nov 3, 2017 • edited Loading

cesium-concierge commented Nov 3, 2017

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

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

ggetz commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Nov 8, 2017

pjcozzi commented Nov 8, 2017

ggetz commented Nov 17, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Nov 21, 2017

ggetz commented Nov 3, 2017 •

edited

Loading