Merge pull request #7168 from OmarShehata/clipping-crash

Fix clipping planes crash when adding plane to empty collection
CesiumGS · Oct 26, 2018 · ff3db12 · ff3db12
2 parents cceb56c + 27c2efa
commit ff3db12
Show file tree

Hide file tree

Showing 5 changed files with 52 additions and 9 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -22,6 +22,7 @@ Change Log
 * Fixed a crash when using `BingMapsGeocoderService` [#7143](https://github.com/AnalyticalGraphicsInc/cesium/issues/7143)
 * Fixed entity visibility issue related to setting an entity show property and altering or adding entity geometry [#7156](https://github.com/AnalyticalGraphicsInc/cesium/pull/7156)
 * Fixed accuracy of rotation matrix generated by `VelocityOrientationProperty` [#6641](https://github.com/AnalyticalGraphicsInc/cesium/pull/6641)
+* Fixed clipping plane crash when adding a plane to an empty collection. [#7168](https://github.com/AnalyticalGraphicsInc/cesium/pull/7168)
 
 ### 1.50 - 2018-10-01
 

diff --git a/Source/Scene/ClippingPlaneCollection.js b/Source/Scene/ClippingPlaneCollection.js
@@ -470,10 +470,8 @@ define([
 
     function computeTextureResolution(pixelsNeeded, result) {
         var maxSize = ContextLimits.maximumTextureSize;
-        var width = Math.min(pixelsNeeded, maxSize);
-        var height = Math.ceil(pixelsNeeded / width);
-        result.x = Math.max(width, 1);
-        result.y = Math.max(height, 1);
+        result.x = Math.min(pixelsNeeded, maxSize);
+        result.y = Math.ceil(pixelsNeeded / result.x);
         return result;
     }
 
@@ -495,7 +493,6 @@ define([
         // In RGBA UNSIGNED_BYTE, A plane is a float in [0, 1) packed to RGBA and an Oct32 quantized normal,
         // so 8 bits or 2 pixels in RGBA.
         var pixelsNeeded = useFloatTexture ? this.length : this.length * 2;
-        var requiredResolution = computeTextureResolution(pixelsNeeded, textureResolutionScratch);
 
         if (defined(clippingPlanesTexture)) {
             var currentPixelCount = clippingPlanesTexture.width * clippingPlanesTexture.height;
@@ -508,10 +505,17 @@ define([
                 pixelsNeeded < 0.25 * currentPixelCount) {
                     clippingPlanesTexture.destroy();
                     clippingPlanesTexture = undefined;
+                    this._clippingPlanesTexture = undefined;
                 }
         }
 
+        // If there are no clipping planes, there's nothing to update.
+        if (this.length === 0) {
+            return;
+        }
+
         if (!defined(clippingPlanesTexture)) {
+            var requiredResolution = computeTextureResolution(pixelsNeeded, textureResolutionScratch);
             // Allocate twice as much space as needed to avoid frequent texture reallocation.
             // Allocate in the Y direction, since texture may be as wide as context texture support.
             requiredResolution.y *= 2;
@@ -572,7 +576,6 @@ define([
             } else {
                 offsetY = Math.floor((dirtyIndex * 2) / clippingPlanesTexture.width);
                 offsetX = Math.floor((dirtyIndex * 2) - offsetY * clippingPlanesTexture.width);
-
                 packPlanesAsUint8(this, dirtyIndex, dirtyIndex + 1);
                 clippingPlanesTexture.copyFrom({
                     width : 2,

diff --git a/Source/Scene/GlobeSurfaceShaderSet.js b/Source/Scene/GlobeSurfaceShaderSet.js
@@ -147,7 +147,7 @@ define([
                     (colorCorrect << 22);
 
         var currentClippingShaderState = 0;
-        if (defined(clippingPlanes)) {
+        if (defined(clippingPlanes) && clippingPlanes.length > 0) {
             currentClippingShaderState = enableClippingPlanes ? clippingPlanes.clippingPlanesState : 0;
         }
         var surfaceShader = surfaceTile.surfaceShader;

diff --git a/Source/Scene/Model.js b/Source/Scene/Model.js
@@ -1150,7 +1150,7 @@ define([
 
     function isClippingEnabled(model) {
         var clippingPlanes = model._clippingPlanes;
-        return defined(clippingPlanes) && clippingPlanes.enabled;
+        return defined(clippingPlanes) && clippingPlanes.enabled && clippingPlanes.length !== 0;
     }
 
     /**
@@ -4344,7 +4344,7 @@ define([
             // Regenerate shaders if ClippingPlaneCollection state changed or it was removed
             var clippingPlanes = this._clippingPlanes;
             var currentClippingPlanesState = 0;
-            if (defined(clippingPlanes) && clippingPlanes.enabled) {
+            if (defined(clippingPlanes) && clippingPlanes.enabled && clippingPlanes.length > 0) {
                 var clippingPlaneOffsetMatrix = defaultValue(this.clippingPlaneOffsetMatrix, modelMatrix);
                 Matrix4.multiply(context.uniformState.view3D, clippingPlaneOffsetMatrix, this._clippingPlaneModelViewMatrix);
                 currentClippingPlanesState = clippingPlanes.clippingPlanesState;

diff --git a/Specs/Scene/ClippingPlaneCollectionSpec.js b/Specs/Scene/ClippingPlaneCollectionSpec.js
@@ -219,6 +219,21 @@ defineSuite([
             scene.destroyForSpecs();
         });
 
+        it('only creates texture when planes are added', function() {
+            var scene = createScene();
+
+            clippingPlanes = new ClippingPlaneCollection();
+            clippingPlanes.update(scene.frameState);
+            expect(clippingPlanes.texture).toBeUndefined();
+
+            clippingPlanes.add(planes[0]);
+            clippingPlanes.update(scene.frameState);
+
+            expect(clippingPlanes.texture).toBeDefined();
+            expect(isNaN(clippingPlanes.texture.width)).toBe(false);
+            expect(isNaN(clippingPlanes.texture.height)).toBe(false);
+        });
+
         it('update fills the clipping plane texture with packed planes', function() {
             var scene = createScene();
 
@@ -399,6 +414,30 @@ defineSuite([
             scene.destroyForSpecs();
         });
 
+        it('only creates texture when planes are added', function() {
+            var scene = createScene();
+
+            if (!ClippingPlaneCollection.useFloatTexture(scene.context)) {
+                // Don't fail just because float textures aren't supported
+                scene.destroyForSpecs();
+                return;
+            }
+
+            clippingPlanes = new ClippingPlaneCollection();
+            clippingPlanes.update(scene.frameState);
+            expect(clippingPlanes.texture).toBeUndefined();
+
+            clippingPlanes.add(planes[0]);
+            clippingPlanes.update(scene.frameState);
+
+            expect(clippingPlanes.texture).toBeDefined();
+            expect(isNaN(clippingPlanes.texture.width)).toBe(false);
+            expect(isNaN(clippingPlanes.texture.height)).toBe(false);
+
+            clippingPlanes.destroy();
+            scene.destroyForSpecs();
+        });
+
         it('update fills the clipping plane texture with packed planes', function() {
             var scene = createScene();