Clipping planes tweaks #6327

likangning93 · 2018-03-13T14:52:54Z

cesium-concierge · 2018-03-13T14:52:55Z

@likangning93, thanks for the pull request! Maintainers, we have a signed CLA from @likangning93, 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.

🌍 🌎 🌏

likangning93 · 2018-03-13T16:01:17Z

Discussion offline, might be better to move the "many clipping planes" demo to Terrain Clipping Planes and remove the drawing portion.

lilleyse · 2018-03-13T16:17:41Z

It might make sense to move ClippingPlane to Scene as well.

…ove ClippingPlane to Scene

lilleyse · 2018-03-13T18:57:27Z

I'm not sure when the bug was introduced, but in 3D Tiles ClippingPlanes if you select the model and then uncheck Enable edge styling there is an error in the console: Uncaught TypeError: Cannot set property 'edgeWidth' of undefined (on line 200)

It looks like this is just a bug in the Sandcastle code.

lilleyse · 2018-03-13T20:19:38Z

Apps/Sandcastle/gallery/Terrain Clipping Planes.html


+var demos = ['Cesium Man', 'St. Helens'];


For consistency rename to exampleTypes.

lilleyse · 2018-03-13T20:20:15Z

Apps/Sandcastle/gallery/Terrain Clipping Planes.html

+
+    // Create centerpoints for each clipping plane
+    var clippingPlanes = [];
+    for (var i = 0; i < pointCount; i++) {


Style: rename pointCount to pointsLength.
Change i++ to ++i

lilleyse · 2018-03-13T20:21:15Z

Apps/Sandcastle/gallery/Terrain Clipping Planes.html

+
+    var pointCount = points.length;
+
+    // Create centerpoints for each clipping plane


Change centerpoints to center points.

lilleyse · 2018-03-13T20:22:48Z

Apps/Sandcastle/gallery/Terrain Clipping Planes.html

 };
 var toolbar = document.getElementById('toolbar');
 Cesium.knockout.track(viewModel);
 Cesium.knockout.applyBindings(viewModel, toolbar);

+// For tracking state when switching demos
+var clippingPlanesEnabled = true;


This one isn't being used.

lilleyse · 2018-03-13T20:24:10Z

CHANGES.md

@@ -15,6 +15,8 @@ Change Log
  * Added a `ClippingPlane` object to be used with `ClippingPlaneCollection`.
 * Updated `WebMapServiceImageryProvider` so it can take an srs or crs string to pass to the resource query parameters based on the WMS version. [#6223](https://github.com/AnalyticalGraphicsInc/cesium/issues/6223)
 * Added a multi-part CZML example to Sandcastle. [#6320](https://github.com/AnalyticalGraphicsInc/cesium/pull/6320)
+* Added `AttributeCompression.octEncodeToCartesian4` and `AttributeCompression.octDecodeFromCartesian4` which will encode and decode unit-length normal vectors using 4 `uint8` components in a `Cartesian4`.


This shouldn't be mentioned in CHANGES.md because the class is private.

lilleyse · 2018-03-13T20:34:15Z

Source/Scene/ClippingPlane.js

@@ -25,6 +25,9 @@ define([
     * opposite to the normal; if zero, the plane passes through the origin.
     */
    function ClippingPlane(normal, distance) {
+        Check.typeOf.object('normal', normal);
+        Check.typeOf.number('distance', distance);


Wrap these 2 lines in

//>>includeStart('debug', pragmas.debug); //>>includeEnd('debug');

lilleyse · 2018-03-13T20:41:22Z

Source/Scene/ClippingPlane.js

-        z : makeGetterStter('z')
+        x : makeGetterSetter('x'),
+        y : makeGetterSetter('y'),
+        z : makeGetterSetter('z')


I would actually just heed @pjcozzi's advice here and not do the separate makeGetterSetter that I had suggested, just in case. #6201 (comment)

lilleyse · 2018-03-13T20:46:26Z

Source/Scene/ClippingPlaneCollection.js

@@ -90,8 +90,7 @@ define([
            }
        }

-        this._enabled = false;
-        this.enabled = defaultValue(options.enabled, true); // set using setter
+        this._enabled = defaultValue(options.enabled, true);


I noticed that the default value of the setter is documented as false but it should be true.

lilleyse · 2018-03-13T20:59:44Z

Source/Scene/getClippingFunction.js

@@ -1,18 +1,16 @@
 define([


I noticed some bugs in here thanks to Webstorm highlighting:

The indexOf check is definitely a problem.
The second one is cosmetic, but would still be nice to fix. The file has a few occurrences of these issues.

lilleyse · 2018-03-13T21:02:45Z

Specs/Renderer/BuiltinFunctionsSpec.js

@@ -374,7 +374,7 @@ defineSuite([
            '                            0.0, 2.0, 0.0, 0.0,' +


Also add any other new builtins here, including unpackFloat.

lilleyse · 2018-03-14T23:11:50Z

Another thing to fix:

I noticed that when a model is constructed with a color the color is not getting applied, probably because recreateProgram is not getting triggered. It's also possible this worked before createProgram was split into createProgram and recreateProgram. Do whatever seems right to make the fix.

lilleyse · 2018-03-14T23:12:21Z

Example code:

var viewer = new Cesium.Viewer('cesiumContainer');

function createModel(url, height, heading, pitch, roll) {
    var origin = Cesium.Cartesian3.fromDegrees(-123.0744619, 44.0503706, 0.0);
    var modelMatrix = Cesium.Transforms.headingPitchRollToFixedFrame(origin, new Cesium.HeadingPitchRoll());

    var model = viewer.scene.primitives.add(Cesium.Model.fromGltf({
        url : url,
        modelMatrix : modelMatrix,
        minimumPixelSize : 16,
        maximumPixelSize : 256,
        color : Cesium.Color.fromAlpha(Cesium.Color.BLUE, 0.85),
        colorBlendMode: Cesium.ColorBlendMode.HIGHLIGHT,
        silhouetteColor: Cesium.Color.fromAlpha(Cesium.Color.YELLOW, 1.0),
        silhouetteSize: 2.0
    }));

    model.readyPromise.then(function(model) {
        var camera = viewer.camera;

        // Zoom to model
        var controller = viewer.scene.screenSpaceCameraController;
        var r = 2.0 * Math.max(model.boundingSphere.radius, camera.frustum.near);
        controller.minimumZoomDistance = r * 0.5;

        var center = Cesium.Matrix4.multiplyByPoint(model.modelMatrix, model.boundingSphere.center, new Cesium.Cartesian3());
        var heading = Cesium.Math.toRadians(230.0);
        var pitch = Cesium.Math.toRadians(-20.0);
        camera.lookAt(center, new Cesium.HeadingPitchRange(heading, pitch, r * 2.0));
    }).otherwise(function(error){
        window.alert(error);
    });
}

createModel('../../SampleData/models/CesiumMilkTruck/CesiumMilkTruck.glb');

likangning93 · 2018-03-15T01:29:40Z

@lilleyse updated! Color on model construction should be fixed too.

lilleyse · 2018-03-15T14:22:45Z

CHANGES.md

@@ -15,6 +15,7 @@ Change Log
  * Added a `ClippingPlane` object to be used with `ClippingPlaneCollection`.
 * Updated `WebMapServiceImageryProvider` so it can take an srs or crs string to pass to the resource query parameters based on the WMS version. [#6223](https://github.com/AnalyticalGraphicsInc/cesium/issues/6223)
 * Added a multi-part CZML example to Sandcastle. [#6320](https://github.com/AnalyticalGraphicsInc/cesium/pull/6320)
+* Added 3D Tiles use-case to Terrain Clipping Planes Sandcastle


This one can go under the

ClippingPlaneCollection updates #6201

heading.

lilleyse · 2018-03-15T14:23:43Z

Source/Scene/ClippingPlaneCollection.js

+        '../Core/Cartesian4',
+        '../Core/Math',
+        '../Core/Check',
+        '../Scene/ClippingPlane',


This one is out of order.

lilleyse · 2018-03-15T14:25:39Z

Source/Scene/GlobeSurfaceShaderSet.js

@@ -1,18 +1,18 @@
 define([
-        '../Core/ClippingPlaneCollection',
        '../Core/defined',
        '../Core/destroyObject',
        '../Core/TerrainQuantization',
        '../Renderer/ShaderProgram',
        '../Scene/SceneMode',


Unrelated to this PR, but for consistency this one should be changed to ./SceneMode

lilleyse · 2018-03-15T14:29:50Z

Specs/Scene/ClippingPlaneCollectionSpec.js

        'Core/AttributeCompression',
        'Core/BoundingSphere',
        'Core/Cartesian2',
        'Core/Cartesian3',
        'Core/Cartesian4',
-        'Core/ClippingPlane',
+        'Scene/ClippingPlane',


Same issue with Scene/ClippingPlaneCollection and Scene/ClippingPlane here.

lilleyse · 2018-03-15T14:30:08Z

Specs/Scene/ClippingPlaneSpec.js

@@ -1,5 +1,5 @@
 defineSuite([
-        'Core/ClippingPlane',
+        'Scene/ClippingPlane',


Whoops, I thought the define for the class being tested had to be first, but I hadn't looked at, say, the spec for Scene...

@likangning93 you are correct, the define for the class being tested does need to be first (because of Cesium specifics).

Yup sorry, I overlooked this case.

lilleyse · 2018-03-15T14:57:38Z

Just those small things. Otherwise looks ready to go.

likangning93 · 2018-03-15T16:20:29Z

@lilleyse updated!

lilleyse · 2018-03-15T16:50:03Z

Ok looks good. Thanks for all the changes!

likangning93 added 4 commits March 12, 2018 16:04

PR comments

8212a8e

moved ClippingPlaneCollection to Scene

40f9a51

add draw clipping planes sandcastle

939bed5

move oct encoding to 4xuint8 to AttributeCompression

f2c46ae

update CHANGES.md

38b54f0

move 3D Tiles + terrain clipping planes to Terrain Clipping Planes, m…

b739b7e

…ove ClippingPlane to Scene

likangning93 added 2 commits March 13, 2018 15:01

fix console error in 3D Tiles Clipping Planes sandcastle

20f673f

move many clipping planes out of showcases/beginner

e95ce17

lilleyse reviewed Mar 13, 2018

View reviewed changes

PR feedback

ce35098

lilleyse reviewed Mar 15, 2018

View reviewed changes

likangning93 added 2 commits March 15, 2018 11:28

cleanup some requires that were missed

996cd2b

Merge branch 'master' into clippingPlanesTweaks

a51f285

lilleyse merged commit 21f3495 into CesiumGS:master Mar 15, 2018

lilleyse deleted the clippingPlanesTweaks branch March 15, 2018 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clipping planes tweaks #6327

Clipping planes tweaks #6327

likangning93 commented Mar 13, 2018

cesium-concierge commented Mar 13, 2018

likangning93 commented Mar 13, 2018

lilleyse commented Mar 13, 2018

lilleyse commented Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse Mar 13, 2018

lilleyse commented Mar 14, 2018

lilleyse commented Mar 14, 2018

likangning93 commented Mar 15, 2018

lilleyse Mar 15, 2018

lilleyse Mar 15, 2018

lilleyse Mar 15, 2018

lilleyse Mar 15, 2018

lilleyse Mar 15, 2018

likangning93 Mar 15, 2018 •

edited

Loading

mramato Mar 15, 2018

lilleyse Mar 15, 2018

lilleyse commented Mar 15, 2018

likangning93 commented Mar 15, 2018

lilleyse commented Mar 15, 2018


		var pointCount = points.length;

		// Create centerpoints for each clipping plane

Clipping planes tweaks #6327

Clipping planes tweaks #6327

Conversation

likangning93 commented Mar 13, 2018

cesium-concierge commented Mar 13, 2018

likangning93 commented Mar 13, 2018

lilleyse commented Mar 13, 2018

lilleyse commented Mar 13, 2018

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

lilleyse commented Mar 14, 2018

lilleyse commented Mar 14, 2018

likangning93 commented Mar 15, 2018

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

likangning93 Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Mar 15, 2018

likangning93 commented Mar 15, 2018

lilleyse commented Mar 15, 2018

likangning93 Mar 15, 2018 •

edited

Loading