fix uint8 clipping plane partial update bug, add spec

CesiumGS · May 9, 2018 · b5cd192 · b5cd192
1 parent eda6e52
commit b5cd192
Show file tree

Hide file tree

Showing 3 changed files with 128 additions and 5 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -20,7 +20,7 @@ Change Log
 
 ##### Fixes :wrench:
 * Fixed a bug causing custom TilingScheme classes to not be able to use a GeographicProjection. [#6524](https://github.com/AnalyticalGraphicsInc/cesium/pull/6524)
-* Fixed a bug causing intermittent crashes with clippin planes due to uninitialized textures. [#6566](https://github.com/AnalyticalGraphicsInc/cesium/issues/6566)
+* Fixed a bug causing intermittent crashes with clippin planes due to uninitialized textures. [#6576](https://github.com/AnalyticalGraphicsInc/cesium/pull/6576)
 
 ### 1.45 - 2018-05-01
 

diff --git a/Source/Scene/ClippingPlaneCollection.js b/Source/Scene/ClippingPlaneCollection.js
@@ -503,16 +503,22 @@ define([
         }
         if (!this._multipleDirtyPlanes) {
             // partial updates possible
-            var offsetY = Math.floor(dirtyIndex / clippingPlanesTexture.width);
-            var offsetX = Math.floor(dirtyIndex - offsetY * clippingPlanesTexture.width);
+            var offsetX = 0;
+            var offsetY = 0;
             if (useFloatTexture) {
+                offsetY = Math.floor(dirtyIndex / clippingPlanesTexture.width);
+                offsetX = Math.floor(dirtyIndex - offsetY * clippingPlanesTexture.width);
+
                 packPlanesAsFloats(this, dirtyIndex, dirtyIndex + 1);
                 clippingPlanesTexture.copyFrom({
                     width : 1,
                     height : 1,
                     arrayBufferView : this._float32View
                 }, offsetX, offsetY);
             } 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/Specs/Scene/ClippingPlaneCollectionSpec.js b/Specs/Scene/ClippingPlaneCollectionSpec.js
@@ -233,10 +233,15 @@ defineSuite([
             expect(packedTexture.width).toEqual(4);
             expect(packedTexture.height).toEqual(2);
 
+            // Reach capacity
             clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
             clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
-            clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
+            clippingPlanes.update(scene.frameState);
+
+            expect(packedTexture.isDestroyed()).toBe(false);
 
+            // Exceed capacity
+            clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
             clippingPlanes.update(scene.frameState);
 
             expect(packedTexture.isDestroyed()).toBe(true);
@@ -261,6 +266,56 @@ defineSuite([
             clippingPlanes.destroy();
             scene.destroyForSpecs();
         });
+
+        it('performs partial updates when only a single plane has changed and full texture updates otherwise', function() {
+            var scene = createScene();
+            var gl = scene.frameState.context._gl;
+            var copyWidth;
+            var copyHeight;
+            spyOn(gl, 'texSubImage2D').and.callFake(function(target, level, xoffset, yoffset, width, height, format, type, arrayBufferView) {
+                copyWidth = width;
+                copyHeight = height;
+            });
+
+            clippingPlanes = new ClippingPlaneCollection({
+                planes : planes,
+                enabled : false,
+                edgeColor : Color.RED,
+                modelMatrix : transform
+            });
+
+            clippingPlanes.update(scene.frameState);
+
+            // Two RGBA uint8 clipping planes consume 4 pixels of texture, allocation to be double that
+            var packedTexture = clippingPlanes.texture;
+            expect(packedTexture.width).toEqual(4);
+            expect(packedTexture.height).toEqual(2);
+
+            var targetPlane = new ClippingPlane(Cartesian3.UNIT_X, 1.0);
+            clippingPlanes.add(targetPlane);
+            clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
+            clippingPlanes.update(scene.frameState);
+
+            // Haven't hit limit yet
+            expect(packedTexture.isDestroyed()).toBe(false);
+
+            // Addition of two planes, expect a full texture update
+            expect(gl.texSubImage2D.calls.count()).toEqual(1);
+            expect(copyWidth).toEqual(packedTexture.width);
+            expect(copyHeight).toEqual(packedTexture.height);
+
+            // Move target plane for partial update
+            targetPlane.distance += 1.0;
+            clippingPlanes.update(scene.frameState);
+
+            expect(packedTexture.isDestroyed()).toBe(false);
+            expect(gl.texSubImage2D.calls.count()).toEqual(2);
+            expect(copyWidth).toEqual(2);
+            expect(copyHeight).toEqual(1);
+
+            clippingPlanes.destroy();
+            scene.destroyForSpecs();
+        });
     });
 
     describe('float texture mode', function() {
@@ -365,10 +420,15 @@ defineSuite([
             expect(packedTexture.width).toEqual(2);
             expect(packedTexture.height).toEqual(2);
 
+            // Reach capacity
             clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
             clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
-            clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
+            clippingPlanes.update(scene.frameState);
+
+            expect(packedTexture.isDestroyed()).toBe(false);
 
+            // Exceed capacity
+            clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
             clippingPlanes.update(scene.frameState);
 
             expect(packedTexture.isDestroyed()).toBe(true);
@@ -393,6 +453,63 @@ defineSuite([
             clippingPlanes.destroy();
             scene.destroyForSpecs();
         });
+
+        it('performs partial updates when only a single plane has changed and full texture updates otherwise', function() {
+            var scene = createScene();
+
+            if (!ClippingPlaneCollection.useFloatTexture(scene._context)) {
+                // Don't fail just because float textures aren't supported
+                scene.destroyForSpecs();
+                return;
+            }
+
+            var gl = scene.frameState.context._gl;
+            var copyWidth;
+            var copyHeight;
+            spyOn(gl, 'texSubImage2D').and.callFake(function(target, level, xoffset, yoffset, width, height, format, type, arrayBufferView) {
+                copyWidth = width;
+                copyHeight = height;
+            });
+
+            clippingPlanes = new ClippingPlaneCollection({
+                planes : planes,
+                enabled : false,
+                edgeColor : Color.RED,
+                modelMatrix : transform
+            });
+
+            clippingPlanes.update(scene.frameState);
+
+            // Two RGBA Float clipping planes consume 2 pixels of texture, allocation to be double that
+            var packedTexture = clippingPlanes.texture;
+            expect(packedTexture.width).toEqual(2);
+            expect(packedTexture.height).toEqual(2);
+
+            var targetPlane = new ClippingPlane(Cartesian3.UNIT_X, 1.0);
+            clippingPlanes.add(targetPlane);
+            clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0));
+            clippingPlanes.update(scene.frameState);
+
+            // Haven't hit limit yet
+            expect(packedTexture.isDestroyed()).toBe(false);
+
+            // Addition of two planes, expect a full texture update
+            expect(gl.texSubImage2D.calls.count()).toEqual(1);
+            expect(copyWidth).toEqual(packedTexture.width);
+            expect(copyHeight).toEqual(packedTexture.height);
+
+            // Move target plane for partial update
+            targetPlane.distance += 1.0;
+            clippingPlanes.update(scene.frameState);
+
+            expect(packedTexture.isDestroyed()).toBe(false);
+            expect(gl.texSubImage2D.calls.count()).toEqual(2);
+            expect(copyWidth).toEqual(1);
+            expect(copyHeight).toEqual(1);
+
+            clippingPlanes.destroy();
+            scene.destroyForSpecs();
+        });
     });
 
     it('does not perform texture updates if the planes are unchanged', function() {