From b5cd1924c780bb5c3e78cf0c98f3657098151f07 Mon Sep 17 00:00:00 2001 From: Kangning Li Date: Wed, 9 May 2018 10:32:03 -0400 Subject: [PATCH] fix uint8 clipping plane partial update bug, add spec --- CHANGES.md | 2 +- Source/Scene/ClippingPlaneCollection.js | 10 +- Specs/Scene/ClippingPlaneCollectionSpec.js | 121 ++++++++++++++++++++- 3 files changed, 128 insertions(+), 5 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index bde2c946856a..6ace091e1bc4 100644 --- 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 index be233a2c9f72..bcb7e0e44da3 100644 --- a/Source/Scene/ClippingPlaneCollection.js +++ b/Source/Scene/ClippingPlaneCollection.js @@ -503,9 +503,12 @@ 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, @@ -513,6 +516,9 @@ define([ 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 index b3079cef5e37..bd6717110818 100644 --- 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() {