Increase max clipping planes using textures #6201

likangning93 · 2018-02-09T18:35:19Z

Storing the clipping planes in textures leads to a few caveats:

precision problems with compression to RGBA means plane transformations happen in the shader now instead of on the CPU side
- so it's a bit more shader math
- but nice benefit is that the texture doesn't get updated as the view changes - yay!
attaching a Texture gl resource to ClippingPlaneCollection means it's no longer super practical to clone it onto each model in a 3D Tileset backed by gltf, so now they share the tileset's reference to a single ClippingPlaneCollection
Texture also means ClippingPlaneCollection now has to be destroyed, but since a Model may either have a reference to its own ClippingPlaneCollection or a Cesium3DTileset's ClippingPlaneCollection, the destroy function is a little weird and might need discussion.

There's also a new Sandcastle that I used for debugging. Performance predictably isn't great with all 2048 planes and gets worse as you get closer to the model or use a 3D Tileset. But it works! Even my laptop with Atom Z3700 Series Intel Graphics will gamely throw up a frame or two every couple seconds.

Fixes #6184
Fixes #6048

…lanes by texture

…ets and update-as-needed

…s on terrain

cesium-concierge · 2018-02-09T18:35:20Z

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

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

🌍 🌎 🌏

mramato · 2018-02-09T18:45:14Z

There's also a new Sandcastle that I used for debugging. Performance predictably isn't great with all 2048 planes and gets worse as you get closer to the model or use a 3D Tileset. But it works! Even my laptop with Atom Z3700 Series Intel Graphics will gamely throw up a frame or two every couple seconds.

This doesn't sound like something we should ship to end users. We have a development directory in Sandcastle gallery for this kind of stuff, I would recommend moving it there.

pjcozzi · 2018-02-09T19:06:23Z

Please bump to me for a final review when ready. In the meantime, one comment:

precision problems with compression to RGBA means plane transformations happen in the shader now instead of on the CPU side

We can likely use float textures when available and emulate the 64-bit floating subtraction - which is generally the main cause of jitter - just like we do for vertex positions with RTE rendering. @bagnell or @lilleyse can point you to example code.

likangning93 · 2018-02-09T19:12:37Z

We can likely use float textures when available and emulate the 64-bit floating subtraction - which is generally the main cause of jitter - just like we do for vertex positions with RTE rendering.

It's kind of nice putting pre-transform planes into the texture though, since we don't have to run CPU-side packing math every frame or do a gl call to update the texture due to view changes.

Post-transform planes are also tricky with 3D Tilesets because tiles can have their own matrices - we'd be doing a lot more texture updates in that case, so performance could be really bad.

pjcozzi · 2018-02-09T19:31:42Z

@likangning93 I don't have the bandwidth to work on this right now, but if there is a precision issue please discuss with @bagnell or @lilleyse as I'm confident there is an approach where we could get higher precision and still keep all the work in the shader.

pjcozzi · 2018-02-10T16:45:31Z

Part of User-defined clipping planes #5765
Does this fix Clipping planes broken in IE #6048?
Does this fix Model pick shaders do not use clipping planes #6184?

lilleyse · 2018-02-14T17:00:05Z

Apps/Sandcastle/gallery/development/Many Clipping Planes.html

+
+var modelUrl = '../../SampleData/models/CesiumAir/Cesium_Air.glb';
+var agiHqUrl = Cesium.CesiumIon.createResource(1458, { accessToken: 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJqdGkiOiIxYmJiNTAxOC1lOTg5LTQzN2EtODg1OC0zMWJjM2IxNGNlYmMiLCJpZCI6NDQsImFzc2V0cyI6WzE0NThdLCJpYXQiOjE0OTkyNjM4MjB9.1WKijRa-ILkmG6utrhDWX6rDgasjD7dZv-G5ZyCmkKg' });
+var instancedUrl = '../../../Specs/Data/Cesium3DTiles/Instanced/InstancedOrientation/';


Make the same changes here as Apps/Sandcastle/gallery/3D Tiles Clipping Planes.html in https://github.com/AnalyticalGraphicsInc/cesium/pull/6218/files

lilleyse · 2018-02-14T17:01:23Z

Apps/Sandcastle/gallery/development/Many Clipping Planes.html

@@ -0,0 +1,255 @@
+<!DOCTYPE html>


For consistency Many Clipping Planes.jpg should be 225x150.

lilleyse · 2018-02-14T17:04:07Z

Source/Core/ClippingPlaneCollection.js

+
+        // Avoid wastefully re-uploading textures when the clipping planes don't change between frames
+        var previousTextureBytes = new ArrayBuffer(ClippingPlaneCollection.TEXTURE_WIDTH * ClippingPlaneCollection.TEXTURE_WIDTH * 4);
+        previousTextureBytes[0] = 1;


Is this to trigger that it has changed on the first frame?

I think the idea was to make sure the texture gets updated with a buffer full of zeros if that's the clipping plane texture produced, but on second thought that's probably impossible so this isn't necessary.

lilleyse · 2018-02-14T17:09:45Z

Source/Core/ClippingPlaneCollection.js

+        this._lengthRangeUnion = new Cartesian4();
+
+        // Packed uniform for plane count and denormalization parameters
+        this._lengthRange = new Cartesian3();


Why is there both _lengthRange and _lengthRangeUnion?

globe and point cloud shaders get regenerated if the ClippingPlaneCollection is flipped from union to intersection, while Model.js just puts a branch in the shader. I think @pjcozzi mentioned changing Model.js to also regenerate shaders on the fly, though. Should that be in a separate PR?

It don't think it hurts if all systems use the vec4 version for now.

Yeah, Model changes will be in another PR.

lilleyse · 2018-02-14T17:13:36Z

Source/Core/ClippingPlaneCollection.js

+         *
+         * @type {Cartesian3}
+         */
+        lengthRange : {


Same comment.

lilleyse · 2018-02-14T19:52:48Z

Source/Scene/PointCloud3DTileContent.js

@@ -1272,26 +1287,13 @@ define([

        var context = frameState.context;

+        this._defaultTexture = context.defaultTexture;


Does the content need to hold onto this? It seems like u_clippingPlanes can just get it from the frame state.

lilleyse · 2018-02-14T19:58:58Z

Source/Core/ClippingPlaneCollection.js

+     * @type {number}
+     * @constant
+     */
+    ClippingPlaneCollection.TEXTURE_WIDTH = CesiumMath.nextPowerOfTwo(Math.ceil(Math.sqrt(ClippingPlaneCollection.MAX_CLIPPING_PLANES * 2)));


I wonder if it would make more sense to make this a 1D texture, at least it would simplify some shader code. I can't seem to load https://webglstats.com/webgl right now but we can check what % of devices support 2048 pixel textures.

It might still be better to keep it as a POT though. I think I remember something about those being more efficient for GPU memory or something, although I also vaguely remember someone saying something about how that's not necessarily true anymore...

We try to use 1D textures for the 3D Tiles batch table, so there is some precedent there. But it doesn't matter much in the end.

lilleyse · 2018-02-14T20:04:02Z

Specs/Scene/ModelSpec.js

+                    new Plane(Cartesian3.UNIT_X, 0.0)
+                ]
+             });
+             model.clippingPlanes = clippingPlanes;


Fix indentation

lilleyse · 2018-02-14T20:05:12Z

Specs/Scene/PointCloud3DTileContentSpec.js

            expect(scene).notToRender(color);

            tileset.clippingPlanes.unionClippingRegions = false;

+            expect(scene).toRenderAndCall(function(rgba) {
+                console.log(rgba);
+            });


What's with all the console.logs in here?

this might be a "whoops" sort of thing...

lilleyse · 2018-02-14T20:06:44Z

Apps/Sandcastle/gallery/development/Many Clipping Planes.html

@@ -0,0 +1,255 @@
+<!DOCTYPE html>


I really like this demo and feel like it could be incorporated into the main clipping planes demo. If that's annoying to do it could be a standalone demo in the main folder.

@mramato recommended moving it to Development since it's more for debugging and performance testing than demonstrating the feature. @pjcozzi also mentioned at one point that we might do actual clipping cylinders, at which point this demo kind of becomes a "wrong" way to achieve the effect.

I really like this demo

Much appreciated though!

I'm not sure about this being in development; I just spent awhile looking for it. At the least, there should be a simple example showing using a lot of clipping planes.

mramato · 2018-02-14T21:06:37Z

Sorry if this was already discussed, but ClippingPlaneCollection.isSupported should be deprecated instead of removed without warning. (Just have the deprecated version return true).

likangning93 · 2018-02-15T20:15:56Z

@lilleyse updated, but as a quick heads-up (as discussed offline) I'm going to take a look at how much work it'll be to include a float texture version as primary and move the RGBA version to fallback.

pjcozzi · 2018-02-15T21:18:49Z

CHANGES.md

@@ -5,6 +5,7 @@ Change Log

 ##### Deprecated :hourglass_flowing_sand:
 * In the `Resource` class, `addQueryParameters` and `addTemplateValues` have been deprecated and will be removed in Cesium 1.45. Please use `setQueryParameters` and `setTemplateValues` instead.
+* `ClippingPlaneCollection` is now supported in Internet Explorer, so `ClippingPlaneCollection.isSupported` has been deprectaed and will be removed in Cesium 1.45.


Please submit an issue to remind us to do this.

pjcozzi · 2018-02-15T21:19:24Z

CHANGES.md

@@ -5,6 +5,7 @@ Change Log

 ##### Deprecated :hourglass_flowing_sand:
 * In the `Resource` class, `addQueryParameters` and `addTemplateValues` have been deprecated and will be removed in Cesium 1.45. Please use `setQueryParameters` and `setTemplateValues` instead.
+* `ClippingPlaneCollection` is now supported in Internet Explorer, so `ClippingPlaneCollection.isSupported` has been deprectaed and will be removed in Cesium 1.45.


Also mention in CHANGES.md that #6184 is fixed.

lilleyse · 2018-02-15T21:25:34Z

Apps/Sandcastle/gallery/development/Many Clipping Planes.html

+        })
+            .then(function() {
+                tileset.clippingPlanes.modelMatrix = Cesium.Transforms.eastNorthUpToFixedFrame(tileset.boundingSphere.center);
+            });


Usually in Cesium code we condense this to:

instancedUrl.then(function(resource) { return loadTileset(resource); }).then(function() { tileset.clippingPlanes.modelMatrix = Cesium.Transforms.eastNorthUpToFixedFrame(tileset.boundingSphere.center); });

lilleyse · 2018-02-15T21:27:49Z

Source/Core/ClippingPlaneCollection.js

+     * @type {number}
+     * @constant
+     */
+    ClippingPlaneCollection.TEXTURE_WIDTH = CesiumMath.nextPowerOfTwo(Math.ceil(Math.sqrt(ClippingPlaneCollection.MAX_CLIPPING_PLANES * 2)));


Bump.

Edit: add @private.

lilleyse · 2018-02-15T21:31:25Z

Source/Core/ClippingPlaneCollection.js

+     * @type {number}
+     * @constant
+     */
+    ClippingPlaneCollection.TEXTURE_WIDTH = CesiumMath.nextPowerOfTwo(Math.ceil(Math.sqrt(ClippingPlaneCollection.MAX_CLIPPING_PLANES * 2)));


We try to use 1D textures for the 3D Tiles batch table, so there is some precedent there. But it doesn't matter much in the end.

lilleyse · 2018-02-15T21:33:45Z

Source/Core/ClippingPlaneCollection.js

     * @private
+     */
+    ClippingPlaneCollection.prototype.destroy = function() {


Round out the doc for this. For example:

/** * Destroys the WebGL resources held by this object. Destroying an object allows for deterministic * release of WebGL resources, instead of relying on the garbage collector to destroy this object. * <br /><br /> * Once an object is destroyed, it should not be used; calling any function other than * <code>isDestroyed</code> will result in a {@link DeveloperError} exception. Therefore, * assign the return value (<code>undefined</code>) to the object as done in the example. * * @returns {undefined} * * @exception {DeveloperError} This object was destroyed, i.e., destroy() was called. * * * @example * clippingPlanes = clippingPlanes && clippingPlanes .destroy(); * * @see ClippingPlaneCollection#isDestroyed */

Also remove @private. See #5615 (comment)

lilleyse · 2018-02-15T21:40:39Z

Source/Core/ClippingPlaneCollection.js

-    ClippingPlaneCollection.prototype.checkDestroy = function(owner) {
-        if (defined(this.owner) && owner !== this.owner) {
-            return this;
+    ClippingPlaneCollection.setOwnership = function(clippingPlaneCollection, newOwner, key) {


Place setOwnership and isSupported above the destroy functions, which are usually last.

lilleyse · 2018-02-15T21:48:56Z

Source/Scene/Model.js

@@ -512,11 +512,15 @@ define([
        this.colorBlendAmount = defaultValue(options.colorBlendAmount, 0.5);

        /**
-         * The {@link ClippingPlaneCollection} used to selectively disable rendering the model. Clipping planes are not currently supported in Internet Explorer.
+         * The {ClippingPlaneCollection} used to selectively disable rendering the model. Clipping planes are not currently supported in Internet Explorer.


Remove any occurrences of Clipping planes are not currently supported in Internet Explorer.

lilleyse · 2018-02-15T21:49:46Z

Source/Scene/Model.js

         *
         * @type {ClippingPlaneCollection}
         */
-        this.clippingPlanes = options.clippingPlanes;
+        this._clippingPlanes = undefined;


Documentation is not needed for underscored variables.

lilleyse · 2018-02-15T21:51:36Z

Source/Scene/Model.js

+        this._clippingPlanes = undefined;
+
+        var clippingPlanes = options.clippingPlanes;
+        ClippingPlaneCollection.setOwnership(clippingPlanes, this, '_clippingPlanes');


Instead of these two lines, you could just call the setter directly.

this.clippingPlanes = options.clippingPlanes

lilleyse · 2018-02-15T21:51:49Z

Source/Scene/Model.js

+        },
+
+        /**
+         * The {ClippingPlaneCollection} used to selectively disable rendering the model.


{@link ClippingPlaneCollection}

lilleyse · 2018-02-15T21:54:50Z

Source/Scene/Model.js

+        // Only destroy the ClippingPlaneCollection if this is the owner - if this model is part of a Cesium3DTileset,
+        // _clippingPlanes references a ClippingPlaneCollection that this model does not own.
+        var clippingPlaneCollection = this._clippingPlanes;
+        if (defined(clippingPlaneCollection) && !clippingPlaneCollection.isDestroyed() && clippingPlaneCollection._owner === this) {


Perhaps add a getter for ClippingPlaneCollection.owner so it doesn't have to access a private var.

pjcozzi · 2018-02-28T20:22:27Z

@likangning93 I don't recall specifics but there is probably some precision loose, which may not matter in general, and certainty not for a fallback case when FLOAT is not supported.

lilleyse · 2018-03-01T20:33:28Z

Apps/Sandcastle/gallery/Terrain Clipping Planes.html

@@ -40,7 +40,7 @@
    'use strict';
 //Sandcastle_Begin
 // Use clipping planes to selectively hide parts of the globe surface.
-// Clipping planes are not currently supported in Internet Explorer.
+//


Remove trailing \\.

lilleyse · 2018-03-01T20:34:55Z

CHANGES.md

@@ -10,6 +10,8 @@ Change Log

 ##### Deprecated :hourglass_flowing_sand:
 * In the `Resource` class, `addQueryParameters` and `addTemplateValues` have been deprecated and will be removed in Cesium 1.45. Please use `setQueryParameters` and `setTemplateValues` instead.
+* `ClippingPlaneCollection` is now supported in Internet Explorer, so `ClippingPlaneCollection.isSupported` has been deprecated and will be removed in Cesium 1.43.


1.43 should now be 1.45.

lilleyse · 2018-03-01T20:35:25Z

CHANGES.md

@@ -19,6 +21,11 @@ Change Log
  * Added `put`, `patch`, `delete`, `options` and `head` methods, so it can be used for all XHR requests.
  * Added `preserveQueryParameters` parameter to `getDerivedResource`, to allow us to append query parameters instead of always replacing them.
  * Added `setQueryParameters` and `appendQueryParameters` to allow for better handling of query strings.
+* Enable terrain in the `CesiumViewer` demo application [#6198](https://github.com/AnalyticalGraphicsInc/cesium/pull/6198)


It looks like this was added accidentally.

lilleyse · 2018-03-01T20:40:59Z

Source/Core/Cartesian4.js

+     * Unpacks a float packed using Cartesian4.packFloat.
+     *
+     * @param {Cartesian4} packedFloat A Cartesian4 containing a float packed to 4 values representable using uint8.
+     */


Add the return variable to the doc.

lilleyse · 2018-03-01T21:13:20Z

Source/Core/ClippingPlane.js

+     * @param {Function} onChangeCallback Callback to take the Plane's index on update.
+     */
+    function ClippingPlane(normal, distance, onChangeCallback) {
+


Remove whitespace.

lilleyse · 2018-03-02T20:29:12Z

Source/Scene/Model.js

+
+        // If clipping planes/model color inactive:
+        // - destroy _rendererResources.*programs
+        // - relink _rendererResources.*programs to _cachedRendererResources


If a model has clipping planes and then a minute later has its color set, what happens to the clipping planes-only shader?

Ah... I guess this should always be running destroyIfNotCached

lilleyse · 2018-03-02T20:31:48Z

Source/Scene/Model.js

@@ -4380,9 +4517,23 @@ define([
        }

        releaseCachedGltf(this);
+        this._sourcePrograms = undefined;
+        this._sourceShaders = undefined;


Also set quantizedVertexShaders to undefined?

lilleyse · 2018-03-02T20:34:06Z

Source/Scene/Model.js

        var program = programs[id];

+        var clippingPlaneCollection = model.clippingPlanes;
+        var addClippingPlaneCode = !firstBuild && needsClippingCode(model);


This needs a similar check for color.

Maybe irrelevant based on the comment above.

lilleyse · 2018-03-02T20:43:30Z

Source/Scene/getClippingFunction.js

+        Check.typeOf.object('clippingPlaneCollection', clippingPlaneCollection);
+        //>>includeEnd('debug');
+        var unionClippingRegions = clippingPlaneCollection.unionClippingRegions;
+        var clippingPlaneCount = clippingPlaneCollection.length;


clippingPlaneCount -> clippingPlanesLenth

lilleyse · 2018-03-02T20:48:06Z

Specs/Core/ClippingPlaneCollectionSpec.js

+
+            if (!ClippingPlaneCollection.useFloatTexture(scene._context)) {
+                // Don't fail just because float textures aren't supported
+                return;


Make sure to delete the scene before returning. Fix throughout the file.

likangning93 · 2018-03-08T01:30:51Z

@lilleyse this is ready for another look

lilleyse

Getting close...

lilleyse · 2018-03-08T14:41:48Z

Source/Core/ClippingPlane.js

+        defined,
+        defineProperties,
+        DeveloperError,
+        Plane


DeveloperError and Plane aren't used.

lilleyse · 2018-03-08T14:42:12Z

Source/Core/ClippingPlane.js

+     * A Plane in Hessian Normal form to be used with ClippingPlaneCollection.
+     * Compatible with mathematics functions in Plane.js
+     *
+     * @constructor


Add @alias ClippingPlane so this class shows up in the documentation.

lilleyse · 2018-03-08T14:42:36Z

Source/Core/ClippingPlane.js

+     * @param {ClippingPlane} clippingPlane The ClippingPlane to be cloned
+     * @param {ClippingPlane} [result] The object on which to store the cloned parameters.
+     */
+    ClippingPlane.clone = function(clippingPlane, result) {


Add @returns.

lilleyse · 2018-03-08T14:52:23Z

Source/Core/ClippingPlaneCollection.js

+        return this._unionClippingRegions ? this._planes.length : -this._planes.length;
+    };
+
+    function computeTextureResolution(pixelsNeeded, result) {


Move this function closer to the are that calls (above update).

lilleyse · 2018-03-08T15:12:13Z

Source/Scene/Cesium3DTile.js

+        if (currentClippingPlanesState !== this._clippingPlanesState) {
+            this._clippingPlanesState = currentClippingPlanesState;
+            this._content.clippingPlanesDirty = true;
+        }


Maybe add the clipping planes code to a separate function updateClippingPlanes? Just to help keep update super simple. Though this._content.clippingPlanesDirty = false; // reset after content update will have to stay here.

lilleyse · 2018-03-08T15:16:51Z

Source/Scene/Cesium3DTile.js

+        if (currentClippingPlanesState !== this._clippingPlanesState) {
+            this._clippingPlanesState = currentClippingPlanesState;
+            this._content.clippingPlanesDirty = true;
+        }


Maybe clippingPlanesDirty should be a property of Cesium3DTile instead. I think that will be cleaner overall.

so very much cleaner
esp. looking at Composite3DTileContent

lilleyse · 2018-03-08T15:22:00Z

Source/Scene/Model.js

@@ -511,12 +514,20 @@ define([
         */
        this.colorBlendAmount = defaultValue(options.colorBlendAmount, 0.5);

+        this.colorShadingEnabled = isColorShadingEnabled(this);


Change to this._colorShadingEnabled

lilleyse · 2018-03-08T15:26:44Z

Source/Scene/Model.js

         */
-        this.clippingPlanes = options.clippingPlanes;
+        this.changedShadersOnLastUpdate = false;


This flag feels wrong to have. Could ModelInstanceCollection instead look to see if model commands are dirty?

lilleyse · 2018-03-08T15:28:05Z

Source/Scene/Model.js

@@ -1870,44 +1914,104 @@ define([
    };

    CreateProgramJob.prototype.execute = function() {
-        createProgram(this.id, this.model, this.context);
+        createProgram(this.id, this.model, this.context, true);


createProgram no longer takes the last argument.

likangning93 · 2018-03-08T20:06:40Z

@lilleyse updated!

likangning93 · 2018-03-09T19:05:26Z

Source/Scene/getClippingFunction.js

+            '    vec3 clipNormal = vec3(0.0);\n' +
+            '    vec3 clipPosition = vec3(0.0);\n' +
+            '    float clipAmount = 0.0;\n' +
+            '    float pixelWidth = czm_metersPerPixel(position);\n' +


@lilleyse dependence on this function causes edge styling to break in orthographic view.
Just setting it to a constant when we switch to orthographic seems fine, but that will need shader recompilation.
Separate PR later?

Discussed offline, should be a quick fix.

likangning93 · 2018-03-09T19:59:31Z

@lilleyse ready!

lilleyse · 2018-03-09T20:28:42Z

This looks ready to go to me.

As a side note, the ability to regenerate model shaders now is pretty nice!

hpinkos · 2018-03-09T20:30:24Z

@likangning93 open an issue to remove the deprecated things

likangning93 · 2018-03-09T20:31:32Z

Here: #6317

hpinkos · 2018-03-09T20:34:07Z

Source/Core/ClippingPlaneCollection.js

     */
    ClippingPlaneCollection.isSupported = function() {
-        return !FeatureDetection.isInternetExplorer();
+        return true;


This should have a deprecationWarning

@likangning93 did you add the deprecation warning here?

hpinkos · 2018-03-09T20:36:22Z

Source/Core/ClippingPlaneCollection.js

+            };
+            plane.index = newPlaneIndex;
+        } else {
+            this._containsUntrackablePlanes = true;


This should print a deprecationWarning too

more use cases for a "whoops" reaction for github...

pjcozzi

@likangning93 this looks pretty good and that you surely learned a lot about the rendering engine. I only have the bandwidth for this one review so please iterate with @lilleyse for any tweaks, and ping me if there is something that you two think you really need me for. Thanks!

pjcozzi · 2018-03-11T14:19:47Z

Apps/Sandcastle/gallery/development/Many Clipping Planes.html

+        var dir = new Cesium.Cartesian3();
+        dir.x = 1.0;
+        dir.y = Math.tan(angle);
+        if (angle > 1.57079632679) {


Here and below, avoid hardcoding values. There are PI constants here: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/Math.js#L321

pjcozzi · 2018-03-11T14:23:10Z

Apps/Sandcastle/gallery/development/Many Clipping Planes.html

@@ -0,0 +1,255 @@
+<!DOCTYPE html>


I'm not sure about this being in development; I just spent awhile looking for it. At the least, there should be a simple example showing using a lot of clipping planes.

pjcozzi · 2018-03-11T14:24:29Z

Source/Core/Cartesian4.js

+     * @param {Cartesian4} [result] The Cartesian4 that will contain the packed float.
+     * @returns {Cartesian4} A Cartesian4 representing the float packed to values in x, y, z, and w.
+     */
+    Cartesian4.packFloat = function(value, result) {


Here and below, add to CHANGES.md or mark these @private.

pjcozzi · 2018-03-11T14:25:51Z

Source/Core/ClippingPlane.js

+    'use strict';
+
+    /**
+     * A Plane in Hessian Normal form to be used with ClippingPlaneCollection.


Use {@link ClippingPlaneCollection}.

Check out the Documentation Guide if you need a refresher on best practices.

pjcozzi · 2018-03-11T14:27:18Z

Source/Core/ClippingPlane.js

+     * in the direction of the normal; if negative, the origin is in the half-space
+     * opposite to the normal; if zero, the plane passes through the origin.
+     */
+    function ClippingPlane(normal, distance) {


normal and distance are required, right? Shouldn't this throw an exception if they are not defined?

Potentially acceptable but borderline shady if this relies on UpdateChangedCartesian3's constructor function to validation normal.

pjcozzi · 2018-03-11T14:56:27Z

Source/Scene/Model.js

            'void main() \n' +
            '{ \n' +
            '    gltf_clip_main(); \n' +
-            '    if (gltf_clippingPlanesLength > 0) \n' +
+            '    float clipDistance = clip(gl_FragCoord, gltf_clippingPlanes, gltf_clippingPlanesMatrix);' +


Is this specific to glTF or is there a reasonable way to encapsulate this so the same GLSL code/function is used for the globe shadre as well?

We could re-use this in PointCloud3DTileContent, but the logic is baked into GlobeFS.glsl and enabled/disabled using injected defines.

I'll go ahead and move it out, it'll be handy in case at some point we want to make all things clippable.

pjcozzi · 2018-03-11T14:58:08Z

Source/Scene/getClippingFunction.js

+    'use strict';
+
+    /**
+     * Gets the glsl functions needed to retrieve clipping planes from a ClippingPlaneCollection's texture.


"GLSL"

Here, below, and throughout.

pjcozzi · 2018-03-11T15:00:23Z

Source/Scene/getUnpackFloatFunction.js

+     */
+    function getUnpackFloatFunction(functionName) {
+        if (!defined(functionName)) {
+            functionName = 'unpackFloat';


Why does this ever need another name and why can't it just be a built-in czm_ GLSL function?

pjcozzi · 2018-03-11T15:01:33Z

Source/Scene/getClippingFunction.js

@@ -0,0 +1,168 @@
+define([


Does this belong in Renderer or Scene? Probably wherever the plane collection lives.

pjcozzi · 2018-03-11T15:02:42Z

Source/Shaders/Builtin/Functions/transformPlane.glsl

@@ -0,0 +1,8 @@
+vec4 czm_transformPlane(mat4 transform, vec4 clippingPlane) {


Should the parameter order be (plane, transform)? Whatever is consistent.

pjcozzi · 2018-03-11T16:47:40Z

Also a disclaimer that I did not look at the tests.

likangning93 added 9 commits January 30, 2018 09:59

proof-of-concept oct32 normals and in-shader transform for clipping p…

0317622

…lanes by texture

proof-of-concept for sharing ClippingPlaneCollections over b3dm tiles…

44a2662

…ets and update-as-needed

working with i3dm and pts

3f40dc2

merged in master

4b7b0ea

add attenuation to demo

15e3a1e

clipping planes working with globe

a0a9706

fix problem with globe rendering before clipping planes are updated

a5e6ff4

packed new clipping plane params to fewer uniforms

a42261a

updated tests, fixed a bug with changing union mode for clippingPlane…

5d3165e

…s on terrain

add gallery image and update changes

e2fb890

move many clipping planes sandcastle to development

cf1b6da

fix issue with clipped models still getting picked

f7d078e

lilleyse reviewed Feb 14, 2018

View reviewed changes

likangning93 added 2 commits February 14, 2018 15:41

added support for IE/Edge by fixing a bug in windowToEyeCoordinates.glsl

8ba4a65

Merge branch 'master' into clippingPlanesByTexture

d484bb3

likangning93 mentioned this pull request Feb 14, 2018

Clipping planes broken in IE #6048

Closed

lilleyse and others added 3 commits February 14, 2018 16:29

Shader style

c917871

PR feedback

b61f98f

merged master into clippingPlanesByTexture

c699d4b

pjcozzi reviewed Feb 15, 2018

View reviewed changes

lilleyse reviewed Feb 15, 2018

View reviewed changes

change float packing to do what BatchTable does

e20d1ec

lilleyse reviewed Mar 2, 2018

View reviewed changes

likangning93 added 3 commits March 7, 2018 12:53

PR comments

c3c10f0

propagate clippingPlanesDirty flag through composite tiles

bc8b313

merged in master

983585e

lilleyse reviewed Mar 8, 2018

View reviewed changes

PR feedback

a05c225

likangning93 force-pushed the clippingPlanesByTexture branch from 28e5141 to a05c225 Compare March 8, 2018 19:44

Style tweak

4e705de

lilleyse force-pushed the clippingPlanesByTexture branch from dab78e3 to 4e705de Compare March 9, 2018 15:28

likangning93 commented Mar 9, 2018

View reviewed changes

likangning93 added 2 commits March 9, 2018 14:37

fixed clipping plane edge styling in orthographic

5185523

merged in master

0f04e46

update CHANGES.md

31c548b

lilleyse merged commit 6d55f29 into CesiumGS:master Mar 9, 2018

lilleyse deleted the clippingPlanesByTexture branch March 9, 2018 20:29

hpinkos reviewed Mar 9, 2018

View reviewed changes

likangning93 mentioned this pull request Mar 9, 2018

Added deprecation warnings for ClippingPlaneCollection #6318

Merged

pjcozzi reviewed Mar 11, 2018

View reviewed changes

likangning93 mentioned this pull request Mar 13, 2018

Clipping planes tweaks #6327

Merged

likangning93 mentioned this pull request May 9, 2018

fix crash due to undefined clipping planes texture #6576

Merged

		@@ -1272,26 +1287,13 @@ define([

		var context = frameState.context;

		this._defaultTexture = context.defaultTexture;

		@@ -0,0 +1,8 @@
		vec4 czm_transformPlane(mat4 transform, vec4 clippingPlane) {

Increase max clipping planes using textures #6201

Increase max clipping planes using textures #6201

Conversation

likangning93 commented Feb 9, 2018 • edited by pjcozzi Loading

cesium-concierge commented Feb 9, 2018

mramato commented Feb 9, 2018

pjcozzi commented Feb 9, 2018

likangning93 commented Feb 9, 2018

pjcozzi commented Feb 9, 2018

pjcozzi commented Feb 10, 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

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 Feb 14, 2018

likangning93 commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Feb 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

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

pjcozzi commented Feb 28, 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

Choose a reason for hiding this comment

likangning93 commented Mar 8, 2018

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

likangning93 Mar 8, 2018 • 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

likangning93 commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likangning93 commented Mar 9, 2018

lilleyse commented Mar 9, 2018

hpinkos commented Mar 9, 2018

likangning93 commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likangning93 commented Feb 9, 2018 •

edited by pjcozzi

Loading

lilleyse Feb 15, 2018 •

edited

Loading

likangning93 Mar 8, 2018 •

edited

Loading