Clipping Planes- Terrain & optimizations, and/or option, plane primitives, edge highlighting #5996

ggetz · 2017-11-21T18:18:53Z

Added ClippingPlanesCollection to encompass clipping plane options
Added inclusive flag to clipping planes, which allows you to and/or the clipping across the planes specified
Added edgeWidth and edgeColor options to style the edges
Added terrainClippingPlanes option to Globe
Optimized the tile visibility function when clipping planes are applied to the globe
Added planePrimitive for visualizations.

…-plane-primitive

…ain-and-highlight-edges

…rigin

…ing-planes-terrain

cesium-concierge · 2017-11-21T18:18:53Z

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

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

🌍 🌎 🌏

ggetz · 2017-11-21T18:23:33Z

@lilleyse This is all the rest of the features we had in scope for clipping planes. I have one item left to do that is only relevant to the sandcastle examples.

TODO:

Fix point cloud clipping example
Update both clipping sandcastle example thumbnails

pjcozzi · 2017-11-22T02:17:20Z

Please bump when the TODOs are addressed.

pjcozzi · 2017-11-22T02:17:49Z

Also, please merge in master.

…g-planes-terrain

ggetz · 2017-11-22T20:32:45Z

@pjcozzi @lilleyse TODO's are updated.

pjcozzi · 2017-11-24T16:52:44Z

CI is failing:

/home/travis/build/AnalyticalGraphicsInc/cesium/Source/DataSources/PlaneGeometryUpdater.js
  371:13  error  'options' is assigned a value but never used  no-unused-vars
/home/travis/build/AnalyticalGraphicsInc/cesium/Source/Scene/Cesium3DTile.js
  745:14  error  'checkTileClipped' is defined but never used  no-unused-vars
/home/travis/build/AnalyticalGraphicsInc/cesium/Source/Scene/ClippingPlanesCollection.js
  40:13  error  'options' is already defined  no-redeclare
/home/travis/build/AnalyticalGraphicsInc/cesium/Source/Scene/GlobeSurfaceTileProvider.js
  1046:9   error  'scratchPlane' is assigned a value but never used   no-unused-vars
  1047:9   error  'scratchMatrix' is assigned a value but never used  no-unused-vars
  1318:25  error  'clippingPlanes' used outside of binding context    block-scoped-var
  1318:44  error  'clippingPlanes' used outside of binding context    block-scoped-var
  1319:17  error  'clippingPlanes' used outside of binding context    block-scoped-var
  1320:64  error  'clippingPlanes' used outside of binding context    block-scoped-var
  1321:85  error  'clippingPlanes' used outside of binding context    block-scoped-var
  1322:64  error  'clippingPlanes' used outside of binding context    block-scoped-var
  1325:49  error  'clippingPlanes' used outside of binding context    block-scoped-var
  1325:68  error  'clippingPlanes' used outside of binding context    block-scoped-var

https://travis-ci.org/AnalyticalGraphicsInc/cesium/builds/305997777?utm_source=github_status&utm_medium=notification

pjcozzi · 2017-11-24T16:55:30Z

Part of #5765.

@ggetz can you please update the tasklist in #5765 (comment)

lilleyse · 2017-12-01T00:52:38Z

My recent comment was buried - just a reminder to address #5996 (comment)

pjcozzi · 2017-12-01T00:56:34Z

Specs/Scene/Cesium3DTilesetSpec.js

@@ -2807,4 +2813,55 @@ defineSuite([
        });
    });

+    it('clipping planes cull hidden tiles', function() {


Instead of / or in addition to these tests, can you add tests like:

Calls update and checks that n draw commands are returned, sets a clipping plane, and checks that m draw calls are returned under different cases, e.g., when all draw commands are culled, no draw commands are culled, when a subset are culled in an HLOD fashion

Include i3dm in a tileset

Renders, checks that a pixel was colored, sets a clip plane to clip everything, renders, then checks that nothing was rendered

Multiple planes with and without combine

Similiar comment for terrain.

Also, looks like explicit tests are missing for Model?

The tests in PointCloud3DTileContentSpec.js look pretty solid - why not apply the pattern throughout? Don't worry about the duplication or encapsulate it.

Finally, please check code coverage for the new files and make it as high as reasonable.

pjcozzi · 2017-12-01T00:58:06Z

Given the work that this needs, let's target 1.40. No need to rush for 1.39 even if it gets merged soon after the release.

CC @tfili

lilleyse

The globe and point clouds tests look good! @pjcozzi already addressed everything else.

lilleyse · 2017-12-01T00:59:18Z

Source/Scene/ClippingPlanesCollection.js

+    Intersect,
+    Matrix4,
+    Plane) {
+    'use strict';


Fix indentation.

lilleyse · 2017-12-01T01:03:05Z

Specs/Core/PlaneSpec.js

+    });
+
+    it('transform throws without a plane', function() {
+        var point = Cartesian3.ZERO;


This should be a Matrix4.

lilleyse · 2017-12-01T01:03:14Z

Specs/Core/PlaneSpec.js

+        }).toThrowDeveloperError();
+    });
+
+    it('transform throws without a point', function() {


point -> transform.

lilleyse · 2017-12-01T01:04:03Z

Specs/DataSources/PlaneGeometryUpdaterSpec.js

+    createDynamicGeometryBoundingSphereSpecs,
+    createDynamicProperty,
+    createScene) {
+    'use strict';


Fix indentation.

lilleyse · 2017-12-01T01:10:48Z

Specs/Scene/Cesium3DTilesetSpec.js

+            visibility = tileset._root.visibility(scene.frameState, CullingVolume.MASK_INSIDE);
+
+            expect(visibility).not.toBe(CullingVolume.MASK_OUTSIDE);
+        });


This test should be tweaked to call tileset.update and check the number of draw commands, similar to some of the other tests. It should expect 5 commands before the clipping plane is enabled and 1 command after. Checking the visibility directly is a bit too low level.

Realized later this is basically a duplicate of @pjcozzi's comment.

lilleyse · 2017-12-01T01:11:15Z

Specs/Scene/ClippingPlanesCollectionSpec.js

+    Intersect,
+    Matrix4,
+    Plane) {
+    'use strict';


Fix indentation.

lilleyse · 2017-12-01T01:13:49Z

Specs/Scene/ClippingPlanesCollectionSpec.js

+
+        var result = clippingPlanes.transformAndPackPlanes(transform);
+        expect(result.length).toEqual(2);
+        expect(result[0] instanceof Cartesian4).toBe(true);


Use toBeInstanceOf

lilleyse · 2017-12-01T01:19:47Z

Specs/Scene/GlobeSurfaceTileProviderSpec.js

+        });
+    });
+
+    it('computes tile visibility culls tiles when clipped', function() {


This wording is awkward.

ggetz · 2017-12-01T20:48:28Z

@lilleyse Spec updates are in, and addressed the plane normals and point cloud issue.

ggetz · 2017-12-01T20:49:55Z

I assume it would be best to merge in master and update CHANGES.md finally when merging clip-planes-master into master?

lilleyse · 2017-12-01T21:04:40Z

Source/Scene/ClippingPlanesCollection.js

+        }
+        for (var i = 0; i < length; ++i) {
+            var plane = this.planes[i];
+            result.planes[i] = new Plane(plane.normal, plane.distance);


Also try to avoid the new here. Above when the new array is created it could initialize with new planes.

Is it ok if I point this at the same plane object? The planes aren't manipulated down at the model level after the collection is cloned, just the enabling and modelMatrix. Or better yet just point it at the same array?

It's probably better not to use the same plane object or array since callers would expect a deep clone (even though the code right now doesn't care).

lilleyse · 2017-12-01T21:21:22Z

I tried running the Sandcastle demo with an i3dm tileset but it seems to always clip. This may just be a matter of hooking up clipping planes in Instanced3DModel3DTileContent and ModelInstanceCollection.

ggetz · 2017-12-01T21:24:28Z

Sorry, I didn't know I also had to hook up that class. It looks like the content is a collection of models, so that's what I should clip, correct?

lilleyse · 2017-12-01T21:27:14Z

Sorry, I didn't know I also had to hook up that class. It looks like the content is a collection of models, so that's what I should clip, correct?

Yeah it should be easy I think, the clipping planes just need to get to the underlying model that the model instance collection creates.

lilleyse · 2017-12-01T21:30:14Z

Also - don't worry about clipping individual instances (i.e. changing the draw command's instance count).

ggetz · 2017-12-01T22:17:58Z

@lilleyse Added i3dm and tests, and it works with the composite tilset.

lilleyse · 2017-12-01T22:25:54Z

Source/Scene/ClippingPlanesCollection.js

            var plane = this.planes[i];
-            result.planes[i] = new Plane(plane.normal, plane.distance);
+            var resultPlane = result.planes[i];
+            resultPlane.normal = plane.normal;


Use Cartesian3.clone here, with resultPlane.normal as the result.

Never mind I see that this is already in.

hpinkos · 2017-12-02T16:02:56Z

Apps/Sandcastle/gallery/3D Tiles Clipping Planes.html

+var scratchPlane = new Cesium.Plane(Cesium.Cartesian3.UNIT_X, 0.0);
+function createPlaneUpdateFunction(plane, transform) {
+    return function () {
+        plane.distance = Cesium.Math.lerp(plane.distance, targetY, 0.1);


I don't think lerping here makes sense. Doing it this way never converges, so the value of plane.distance changes slightly every frame

I'll remove it. It is there to make the plane movement smooth since reflecting the mouse movement directly can be shaky.

lilleyse · 2017-12-04T14:21:52Z

Thanks @ggetz all looks good from my end.

ggetz · 2017-12-04T14:27:26Z

Thanks @lilleyse, @hpinkos ! Updated.

lilleyse · 2017-12-04T14:30:13Z

I assume it would be best to merge in master and update CHANGES.md finally when merging clip-planes-master into master?

Sounds good. Feel free to open that PR now. I might do one quick glance over everything but it should be quick to merge.

ggetz added 18 commits November 6, 2017 11:34

Add plane geometry

d188014

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

8ef6754

…-plane-primitive

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

51f28d0

…-plane-primitive

Added plane graphics and geometries, update clipping planes example

d142b0f

Cleanup

4dfea3a

Update sandcastle demo, colors and scenario

ee8b97a

Clipping planes on terrain

33eb86a

Modified shaders to highlight clipping edge

951a33b

Add demo

8981779

Merge branch 'clipping-planes-edge-styling' into clipping-planes-terr…

a0932d5

…ain-and-highlight-edges

Add highlighting to terrain edges

c0ec2c4

Multiple clipping planes && together, define terrain clipping plane o…

5ec1058

…rigin

Merge branch 'clipping-planes-terrain-and-highlight-edges' into clipp…

48190c7

…ing-planes-terrain

Add ClippingPlaneCollection

443f73a

Fixed scaling in demo

910e1c6

Optimize globe tile loading with clipping planes

6d67925

Cleanup docs

086a03b

Updated CHANGES.md

af21899

Tweak doc

7487847

pjcozzi requested review from pjcozzi and lilleyse November 22, 2017 02:17

ggetz added 2 commits November 22, 2017 10:44

Merge remote-tracking branch 'cesium/clip-planes-master' into clippin…

ea2d8da

…g-planes-terrain

Update Sandcastle example, fix sandcastle images

227698c

pjcozzi reviewed Dec 1, 2017

View reviewed changes

lilleyse reviewed Dec 1, 2017

View reviewed changes

ggetz added 4 commits December 1, 2017 11:03

Cleanup, update specs

47e0ec3

Fix plane geometry surface

5680c34

Draw commands in specs

21c96e9

Model and PlaneGeometry Specs

3b069ca

Cleanup

82a2780

lilleyse reviewed Dec 1, 2017

View reviewed changes

i3dm tileset in specs

ca40033

ggetz added 2 commits December 1, 2017 17:10

Added i3dm tilesets

31079f7

i3dm specs

b83e288

lilleyse reviewed Dec 1, 2017

View reviewed changes

Add instanced tileset to sandcastle example

2d855b8

hpinkos reviewed Dec 2, 2017

View reviewed changes

Tweak sandcastle example

c8cd6fa

lilleyse merged commit 66d6e05 into CesiumGS:clip-planes-master Dec 4, 2017

This was referenced Dec 4, 2017

Clipping Planes #6026

Merged

How to push ground? #4129

Closed

Clipping Planes- Terrain & optimizations, and/or option, plane primitives, edge highlighting #5996

Clipping Planes- Terrain & optimizations, and/or option, plane primitives, edge highlighting #5996

Conversation

ggetz commented Nov 21, 2017 • edited Loading

cesium-concierge commented Nov 21, 2017

ggetz commented Nov 21, 2017 • edited Loading

pjcozzi commented Nov 22, 2017

pjcozzi commented Nov 22, 2017

ggetz commented Nov 22, 2017

pjcozzi commented Nov 24, 2017

pjcozzi commented Nov 24, 2017

lilleyse commented Dec 1, 2017

Choose a reason for hiding this comment

pjcozzi commented Dec 1, 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

ggetz commented Dec 1, 2017

ggetz commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Dec 1, 2017

ggetz commented Dec 1, 2017

lilleyse commented Dec 1, 2017

lilleyse commented Dec 1, 2017

ggetz commented Dec 1, 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

lilleyse commented Dec 4, 2017

ggetz commented Dec 4, 2017

lilleyse commented Dec 4, 2017

ggetz commented Nov 21, 2017 •

edited

Loading

ggetz commented Nov 21, 2017 •

edited

Loading