Clipping Planes #6026

ggetz · 2017-12-04T15:19:57Z

Part of #5765

Code changes from #5996 (comment), and merged in master and updated CHANGES.md.

@lilleyse This is my fork, we'll still need to delete the other clip-planes-master branch.

Clipping planes for models, tilesets, point clouds

…-plane-primitive

hpinkos · 2017-12-07T18:03:33Z

Source/Core/PlaneGeometry.js

+
+        if (vertexFormat.position &&
+            (vertexFormat.st || vertexFormat.normal || vertexFormat.tangent || vertexFormat.bitangent)) {
+            if (vertexFormat.position) {


Won't this always be true? It's in the block for if (vertexFormat.position && ...)

hpinkos · 2017-12-07T18:13:51Z

Source/Core/PlaneGeometry.js

+                positions[11] = 0.0;
+
+                // -z face
+                positions[12] = min.x;


I don't get why we need to create a front and back face. We don't do this for WallGeometry. You just need to set the correct attributes for the appearance when you're creating the primitive so it doesn't do backface culling

hpinkos · 2017-12-07T18:14:59Z

Source/Core/PlaneGeometry.js

+            });
+
+            // 12 triangles:  2 faces, 2 triangles each.
+            indices = new Uint16Array(2 * 2 * 3);


This is wrong. We only have 1 face here with 2 triangles

hpinkos · 2017-12-07T18:17:30Z

Source/Core/PlaneOutlineGeometry.js

+            attributes : attributes,
+            indices : indices,
+            primitiveType : PrimitiveType.LINES,
+            boundingSphere : new BoundingSphere(Cartesian3.ZERO, Math.sqrt(2.0))


The value for this bounding sphere is different than that for PlaneGeometry. Which one is correct?

hpinkos · 2017-12-07T18:21:28Z

Source/DataSources/ModelVisualizer.js

@@ -153,6 +153,7 @@ define([
            model.color = Property.getValueOrDefault(modelGraphics._color, time, defaultColor, model._color);
            model.colorBlendMode = Property.getValueOrDefault(modelGraphics._colorBlendMode, time, defaultColorBlendMode);
            model.colorBlendAmount = Property.getValueOrDefault(modelGraphics._colorBlendAmount, time, defaultColorBlendAmount);
+            model.clippingPlanes = Property.getValueOrUndefined(modelGraphics._clippingPlanes, time);


Should this take a result parameter?

I don't think so, it should just return undefined if there's no value.

hpinkos · 2017-12-07T18:22:20Z

Source/Core/PlaneGeometry.js

+        var vertexFormat = planeGeometry._vertexFormat;
+
+        var min = new Cartesian3(-0.5, -0.5, 0.0);
+        var max = new Cartesian3( 0.5,  0.5, 0.0);


Since these are always the same we can move then out of this function scope

hpinkos · 2017-12-07T18:23:04Z

Source/Core/PlaneOutlineGeometry.js

+     * @returns {Geometry|undefined} The computed vertices and indices.
+     */
+    PlaneOutlineGeometry.createGeometry = function() {
+        var min = new Cartesian3(-0.5, -0.5, 0.0);


hpinkos · 2017-12-07T18:29:51Z

Source/DataSources/PlaneGeometryUpdater.js

+        }
+
+        if (!defined(dimensions)) {
+            dimensions = new Cartesian2(1.0, 1.0);


Can we use a scratch variable for this?

hpinkos · 2017-12-07T18:32:30Z

Source/Scene/Cesium3DTile.js

+        if (defined(clippingPlanes) && clippingPlanes.enabled) {
+            var tileTransform = tileset._root.computedTransform;
+            var intersection = clippingPlanes.computeIntersectionWithBoundingVolume(boundingVolume, tileTransform);
+            this._isClipped = (intersection !== Intersect.INSIDE);


You don't need the ( )

hpinkos · 2017-12-07T18:32:40Z

Source/Scene/Cesium3DTile.js

+        if (defined(clippingPlanes) && clippingPlanes.enabled) {
+            var tileTransform = tileset._root.computedTransform;
+            var intersection = clippingPlanes.computeIntersectionWithBoundingVolume(boundingVolume, tileTransform);
+            this._isClipped = (intersection !== Intersect.INSIDE);


mramato · 2017-12-07T18:35:31Z

@mramato do you have opinions on this?

I've only barely looked over the code here, but yes; it should be consistent with the rest of our API and the way it's currently set up is definitely not the way to go.

hpinkos · 2017-12-07T18:36:41Z

Source/Scene/Cesium3DTileset.js

+         *
+         * @type {ClippingPlanesCollection}
+         */
+        this.clippingPlanes = options.clippingPlanes;


BUG: I set this value to a ClippingPlaneCollection and it worked correctly. Then I set this value to undefined but the tileset was still clipped

hpinkos · 2017-12-07T18:37:30Z

Source/Scene/ClippingPlanesCollection.js

+        options = defaultValue(options, defaultValue.EMPTY_OBJECT);
+
+        /**
+         * An array of up to 6 {@link Plane} objects used to selectively disable rendering on the outside of each plane.


Why 6? What happens if you add more? Should we throw a developer error when someone tries to do that?

This is an arbitrary limit we have based on the uniform we pass to the shader. We'd like to include more, but need to restructure how we pass in the planes, see the roadmap. If set to more, they are simply ignored.

We have it at 6 for now because that's all the faces of a cube.

Okay, that makes sense. I think we want to have this class follow the same API as the other XXXCollection classes, so make this.planes -> this._planes, then in the constructor and the ClippingPlanesCollection.prototype.add function throw a developer error if the length is greater than 6. That way people using clipping planes will be aware of what's going on instead of wondering why their planes were ignored

hpinkos · 2017-12-07T18:39:58Z

Source/Scene/ClippingPlanesCollection.js

+         * @type {Matrix4}
+         * @default Matrix4.IDENTITY
+         */
+        this.modelMatrix = defaultValue(options.modelMatrix, Matrix4.clone(Matrix4.IDENTITY));


This should be Matrix4.clone(defaultValue(options.modelMatrix, Matrix4.IDENTITY)) instead. Or do it in an if statement. Using defaultValue makes the Matrxi4.clone resolve first so we're creating a new Matrix4 every time regardless of whether or not we're using it.

Same comment for edgeColor below

hpinkos · 2017-12-07T18:41:11Z

Source/Scene/ClippingPlanesCollection.js

+
+    function getTestIntersectionFunction(combineClippingRegions) {
+        if (combineClippingRegions) {
+            return function(value) {


This creates a new function each time this is called. Move these outside of the scope instead.

hpinkos · 2017-12-07T18:42:40Z

Source/Scene/ClippingPlanesCollection.js

+     * Applies the transformations to each plane and packs it into an array.
+     * @private
+     *
+     * @param viewMatrix The 4x4 matrix to transform the plane into eyespace.


param types?

hpinkos · 2017-12-07T18:45:47Z

Source/Scene/ClippingPlanesCollection.js

+            array = new Array(length);
+
+            for (i = 0; i < length; ++i) {
+                array[i] = new Cartesian4();


We can be smarter about this. You can assume that the result array is already filled with Cartesian4 right? So we only need to create new ones for the range i = oldArrayLength.length; i < length; i++
If the array passed in is too large, we can simply do array.length = length

Why are we using a Cartesian4 anyway? Wouldn't it be better just to use a flat array where every 4 indices is equal to 1 plane?

You can assume that the result array is already filled with Cartesian4 right?

We changed to this because it's a public method, and the result parameter is optional like our other methods that take result paramters. We can require it though if that's ok.

It's alright if the result parameter is optional. But if a result parameter is supplied, we should be able to assume [0..length] is already filled with Cartesian4 so we don't need to make a new one for those indices

hpinkos · 2017-12-07T18:49:03Z

Source/Scene/ClippingPlanesCollection.js

+            var plane = this.planes[i];
+            var resultPlane = result.planes[i];
+            resultPlane.normal = plane.normal;
+            resultPlane.distance = plane.distance;


Let's add a clone function to Plane to handle this

You'll want to clone plane.normal as well

hpinkos · 2017-12-07T18:53:29Z

Source/Scene/GlobeSurfaceTile.js

@@ -80,6 +80,7 @@ define([
        this.pickTerrain = undefined;

        this.surfaceShader = undefined;
+        this.isClipped = true;


It seems like this should default to false or undefined instead of true.

hpinkos · 2017-12-07T18:55:15Z

Source/Scene/GlobeSurfaceTileProvider.js

+            var planeIntersection = clippingPlanes.computeIntersectionWithBoundingVolume(boundingVolume);
+            tile.isClipped = (planeIntersection !== Intersect.INSIDE);
+            if (planeIntersection === Intersect.OUTSIDE) {
+                return Intersect.OUTSIDE;


The doc for this function says it returns a Visibility so this return value doesn't seem correct

hpinkos · 2017-12-07T18:58:36Z

Source/Scene/ClippingPlanesCollection.js

+        var length = this.planes.length;
+        var i;
+        if (result.planes.length !== length) {
+            result.planes = new Array(length);


Same comment I made for ClippingPlanesCollection.transformAndPackPlanes

hpinkos · 2017-12-07T18:59:30Z

Source/Scene/GlobeSurfaceTileProvider.js

+            }
+
+            var clippingPlanesProperty = uniformMapProperties.clippingPlanes;
+            if (length !== clippingPlanesProperty.length) {


Same comment I made for ClippingPlanesCollection.transformAndPackPlanes

hpinkos · 2017-12-07T19:02:16Z

Source/Scene/Model.js

+        }
+
+        if (model._packedClippingPlanes.length !== length) {
+            model._packedClippingPlanes = new Array(length);


Same comment I made for ClippingPlanesCollection.transformAndPackPlanes

hpinkos · 2017-12-07T19:03:07Z

Source/Scene/PointCloud3DTileContent.js

+        }
+
+        if (this._packedClippingPlanes.length !== length) {
+            this._packedClippingPlanes = new Array(length);


Same comment I made for ClippingPlanesCollection.transformAndPackPlanes

hpinkos · 2017-12-07T19:10:35Z

@ggetz please open a new PR with these changes

@lilleyse for a PR with a new feature like this, please give other people on the team more time to review it before merging it in. This was only open for a day and I found a number of mistakes.

lilleyse · 2017-12-07T19:16:11Z

Thanks for the review, clearly I missed a lot of stuff. I'll keep it open longer next time.

hpinkos · 2017-12-07T19:17:06Z

BUG: I zoomed into a point cloud until some of the higher resolution tiles started popping in. Then I added a clipping plane. Then I zoomed out and the lower resolution tiles were not clipped

hpinkos · 2017-12-07T19:32:18Z

Source/Scene/ClippingPlanesCollection.js

+         * @type {Boolean}
+         * @default true
+         */
+        combineClippingRegions : {


I found the name of this property to be a little confusing and thought it did the opposite of what it actually does at first. It's essentially either doing a union or an intersection of the regions, right? The regions are more or less combined either way. I would rename it to unionClippingRegions or something along those lines instead.

ggetz · 2017-12-07T20:41:36Z

Thanks @hpinkos ! I'll open a new PR for these changes.

ggetz and others added 30 commits October 17, 2017 10:44

Put clipping planes shader code in model

05a4387

Tileset clippingPlanes

5d0d22e

Added sandcastle example

cb685e4

Point clouds

5512b11

Fix error, cleanup

ff35a7d

Plane relative to model origin

c3945d1

Sandcastle updates, point cloud update

9252ad5

Refactoring, update CHANGES.md

2b342c0

Fix shader

7a56927

Point cloud uniforms

1513f0c

Cleanup sandcastle

d25a825

Sandcastle cleanup, fix shaders

8642cf7

Fix scaling

f843aa3

Updating clipping plane model scaling

d9a2c56

Updating clipping plane model scaling

bb4c63f

Clipping planes use computed model matrix

5be9ef6

Apply scaling to clipping planes

973ae3a

Packed planes class member

7d6cba2

Formatting

1c101a8

Check for undefined

b01ffa9

Merge pull request #5913 from ggetz/clipping-planes-models

7567c0c

Clipping planes for models, tilesets, point clouds

Tileset traversal with clipping planes

271dade

Add plane geometry

d188014

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

5bd4ad6

Cleanup CullingVolume changes

ad4f9e5

Flip 'inside' and 'outside' for clipping

02bdf52

Merge branch 'clipping-planes-tileset-traversal' into clipping-planes…

8ef6754

…-plane-primitive

Disable clipping in shader when tile will not be clipped

072400c

Doc fixes and sandcastle updates

8330177

Doc udates

45a48d0

hpinkos reviewed Dec 7, 2017

View reviewed changes

ggetz mentioned this pull request Dec 11, 2017

Clipping planes updates #6045

Merged

Clipping Planes #6026

Clipping Planes #6026

Conversation

ggetz commented Dec 4, 2017

hpinkos Dec 7, 2017 • 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

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

mramato commented Dec 7, 2017

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

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 commented Dec 7, 2017

lilleyse commented Dec 7, 2017

hpinkos commented Dec 7, 2017

Choose a reason for hiding this comment

ggetz commented Dec 7, 2017

hpinkos Dec 7, 2017 •

edited

Loading