From eda6e5201249010622f411953db8d627917c24a5 Mon Sep 17 00:00:00 2001 From: Kangning Li Date: Tue, 8 May 2018 16:02:41 -0400 Subject: [PATCH 1/5] fix crash due to undefined clipping planes texture --- CHANGES.md | 1 + Source/Scene/ClippingPlaneCollection.js | 30 ++++++++++- Source/Scene/GlobeSurfaceShaderSet.js | 2 +- Source/Scene/Model.js | 8 +-- Source/Scene/PointCloud3DTileContent.js | 2 +- Source/Scene/getClippingFunction.js | 27 ++++++---- Specs/Scene/ClippingPlaneCollectionSpec.js | 59 ++++++++++++++++------ 7 files changed, 96 insertions(+), 33 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index eae7d53fa1a4..bde2c946856a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -20,6 +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) ### 1.45 - 2018-05-01 diff --git a/Source/Scene/ClippingPlaneCollection.js b/Source/Scene/ClippingPlaneCollection.js index f4ef53be5f8b..be233a2c9f72 100644 --- a/Source/Scene/ClippingPlaneCollection.js +++ b/Source/Scene/ClippingPlaneCollection.js @@ -459,7 +459,8 @@ define([ if (!defined(clippingPlanesTexture)) { // Allocate twice as much space as needed to avoid frequent texture reallocation. - requiredResolution.x *= 2; + // Allocate in the Y direction, since texture may be as wide as context texture support. + requiredResolution.y *= 2; var sampler = new Sampler({ wrapS : TextureWrap.CLAMP_TO_EDGE, @@ -665,6 +666,33 @@ define([ return context.floatingPointTexture; }; + /** + * Function for getting the clipping plane collection's texture resolution. + * If the ClippingPlaneCollection hasn't been updated, returns the resolution that will be + * allocated based on the current plane count. + * + * @param {ClippingPlaneCollection} clippingPlaneCollection The clipping plane collection + * @param {Context} context The rendering context + * @param {Cartesian2} result A Cartesian2 for the result. + * @returns {Cartesian2} The required resolution. + * @private + */ + ClippingPlaneCollection.getTextureResolution = function(clippingPlaneCollection, context, result) { + var texture = clippingPlaneCollection.texture; + if (defined(texture)) { + result.x = texture.width; + result.y = texture.height; + return result; + } + + var pixelsNeeded = ClippingPlaneCollection.useFloatTexture(context) ? clippingPlaneCollection.length : clippingPlaneCollection.length * 2; + var requiredResolution = computeTextureResolution(pixelsNeeded, result); + + // Allocate twice as much space as needed to avoid frequent texture reallocation. + requiredResolution.y *= 2; + return requiredResolution; + }; + /** * Returns true if this object was destroyed; otherwise, false. *

diff --git a/Source/Scene/GlobeSurfaceShaderSet.js b/Source/Scene/GlobeSurfaceShaderSet.js index 49ed43895ddc..5a7a7e819bb7 100644 --- a/Source/Scene/GlobeSurfaceShaderSet.js +++ b/Source/Scene/GlobeSurfaceShaderSet.js @@ -123,7 +123,7 @@ define([ var fs = this.baseFragmentShaderSource.clone(); if (currentClippingShaderState !== 0) { - fs.sources.unshift(getClippingFunction(clippingPlanes)); // Need to go before GlobeFS + fs.sources.unshift(getClippingFunction(clippingPlanes, frameState.context)); // Need to go before GlobeFS } vs.defines.push(quantizationDefine); diff --git a/Source/Scene/Model.js b/Source/Scene/Model.js index c53098437a4d..4ab26fb5e4e8 100644 --- a/Source/Scene/Model.js +++ b/Source/Scene/Model.js @@ -2035,7 +2035,7 @@ define([ finalFS = Model._modifyShaderForColor(finalFS, model._hasPremultipliedAlpha); } if (addClippingPlaneCode) { - finalFS = modifyShaderForClippingPlanes(finalFS, clippingPlaneCollection); + finalFS = modifyShaderForClippingPlanes(finalFS, clippingPlaneCollection, context); } var drawVS = modifyShader(vs, id, model._vertexShaderLoaded); @@ -2055,7 +2055,7 @@ define([ } if (addClippingPlaneCode) { - pickFS = modifyShaderForClippingPlanes(pickFS, clippingPlaneCollection); + pickFS = modifyShaderForClippingPlanes(pickFS, clippingPlaneCollection, context); } pickVS = ModelUtility.modifyVertexShaderForLogDepth(pickVS, toClipCoordinatesGLSL); @@ -3947,9 +3947,9 @@ define([ } } - function modifyShaderForClippingPlanes(shader, clippingPlaneCollection) { + function modifyShaderForClippingPlanes(shader, clippingPlaneCollection, context) { shader = ShaderSource.replaceMain(shader, 'gltf_clip_main'); - shader += Model._getClippingFunction(clippingPlaneCollection) + '\n'; + shader += Model._getClippingFunction(clippingPlaneCollection, context) + '\n'; shader += 'uniform sampler2D gltf_clippingPlanes; \n' + 'uniform mat4 gltf_clippingPlanesMatrix; \n' + diff --git a/Source/Scene/PointCloud3DTileContent.js b/Source/Scene/PointCloud3DTileContent.js index 78638b9d62d3..60bf9564429b 100644 --- a/Source/Scene/PointCloud3DTileContent.js +++ b/Source/Scene/PointCloud3DTileContent.js @@ -1126,7 +1126,7 @@ define([ 'uniform mat4 u_clippingPlanesMatrix; \n' + 'uniform vec4 u_clippingPlanesEdgeStyle; \n'; fs += '\n'; - fs += getClippingFunction(clippingPlanes); + fs += getClippingFunction(clippingPlanes, context); fs += '\n'; } diff --git a/Source/Scene/getClippingFunction.js b/Source/Scene/getClippingFunction.js index 902eb9b87b7e..c694f7648b26 100644 --- a/Source/Scene/getClippingFunction.js +++ b/Source/Scene/getClippingFunction.js @@ -1,36 +1,43 @@ define([ + '../Core/Cartesian2', '../Core/Check', - '../Renderer/PixelDatatype' + '../Renderer/PixelDatatype', + './ClippingPlaneCollection' ], function( + Cartesian2, Check, - PixelDatatype) { + PixelDatatype, + ClippingPlaneCollection) { 'use strict'; + var textureResolutionScratch = new Cartesian2(); /** * Gets the GLSL functions needed to retrieve clipping planes from a ClippingPlaneCollection's texture. * * @param {ClippingPlaneCollection} clippingPlaneCollection ClippingPlaneCollection with a defined texture. + * @param {Context} context The current rendering context. * @returns {String} A string containing GLSL functions for retrieving clipping planes. * @private */ - function getClippingFunction(clippingPlaneCollection) { + function getClippingFunction(clippingPlaneCollection, context) { //>>includeStart('debug', pragmas.debug); Check.typeOf.object('clippingPlaneCollection', clippingPlaneCollection); + Check.typeOf.object('context', context); //>>includeEnd('debug'); var unionClippingRegions = clippingPlaneCollection.unionClippingRegions; var clippingPlanesLength = clippingPlaneCollection.length; - var texture = clippingPlaneCollection.texture; - var usingFloatTexture = texture.pixelDatatype === PixelDatatype.FLOAT; - var width = texture.width; - var height = texture.height; + var usingFloatTexture = ClippingPlaneCollection.useFloatTexture(context); + var textureResolution = ClippingPlaneCollection.getTextureResolution(clippingPlaneCollection, context, textureResolutionScratch); + var width = textureResolution.x; + var height = textureResolution.y; var functions = usingFloatTexture ? getClippingPlaneFloat(width, height) : getClippingPlaneUint8(width, height); functions += '\n'; - functions += unionClippingRegions ? clippingFunctionUnion(usingFloatTexture, clippingPlanesLength) : clippingFunctionIntersect(usingFloatTexture, clippingPlanesLength); + functions += unionClippingRegions ? clippingFunctionUnion(clippingPlanesLength) : clippingFunctionIntersect(clippingPlanesLength); return functions; } - function clippingFunctionUnion(usingFloatTexture, clippingPlanesLength) { + function clippingFunctionUnion(clippingPlanesLength) { var functionString = 'float clip(vec4 fragCoord, sampler2D clippingPlanes, mat4 clippingPlanesMatrix)\n' + '{\n' + @@ -66,7 +73,7 @@ define([ return functionString; } - function clippingFunctionIntersect(usingFloatTexture, clippingPlanesLength) { + function clippingFunctionIntersect(clippingPlanesLength) { var functionString = 'float clip(vec4 fragCoord, sampler2D clippingPlanes, mat4 clippingPlanesMatrix)\n' + '{\n' + diff --git a/Specs/Scene/ClippingPlaneCollectionSpec.js b/Specs/Scene/ClippingPlaneCollectionSpec.js index 0ebd498eaa3d..b3079cef5e37 100644 --- a/Specs/Scene/ClippingPlaneCollectionSpec.js +++ b/Specs/Scene/ClippingPlaneCollectionSpec.js @@ -158,8 +158,8 @@ defineSuite([ expect(packedTexture).toBeDefined(); // Two RGBA uint8 clipping planes consume 4 pixels of texture, allocation to be double that - expect(packedTexture.width).toEqual(8); - expect(packedTexture.height).toEqual(1); + expect(packedTexture.width).toEqual(4); + expect(packedTexture.height).toEqual(2); expect(packedTexture.pixelFormat).toEqual(PixelFormat.RGBA); expect(packedTexture.pixelDatatype).toEqual(PixelDatatype.UNSIGNED_BYTE); @@ -230,8 +230,8 @@ defineSuite([ var packedTexture = clippingPlanes.texture; // Two RGBA uint8 clipping planes consume 4 pixels of texture, allocation to be double that - expect(packedTexture.width).toEqual(8); - expect(packedTexture.height).toEqual(1); + expect(packedTexture.width).toEqual(4); + expect(packedTexture.height).toEqual(2); clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0)); clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0)); @@ -243,8 +243,8 @@ defineSuite([ packedTexture = clippingPlanes.texture; // Five RGBA uint8 clipping planes consume 10 pixels of texture, allocation to be double that - expect(packedTexture.width).toEqual(20); - expect(packedTexture.height).toEqual(1); + expect(packedTexture.width).toEqual(10); + expect(packedTexture.height).toEqual(2); clippingPlanes.removeAll(); clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0)); @@ -255,8 +255,8 @@ defineSuite([ packedTexture = clippingPlanes.texture; // One RGBA uint8 clipping plane consume 2 pixels of texture, allocation to be double that - expect(packedTexture.width).toEqual(4); - expect(packedTexture.height).toEqual(1); + expect(packedTexture.width).toEqual(2); + expect(packedTexture.height).toEqual(2); clippingPlanes.destroy(); scene.destroyForSpecs(); @@ -284,8 +284,8 @@ defineSuite([ var packedTexture = clippingPlanes.texture; expect(packedTexture).toBeDefined(); - expect(packedTexture.width).toEqual(4); - expect(packedTexture.height).toEqual(1); + expect(packedTexture.width).toEqual(2); + expect(packedTexture.height).toEqual(2); expect(packedTexture.pixelFormat).toEqual(PixelFormat.RGBA); expect(packedTexture.pixelDatatype).toEqual(PixelDatatype.FLOAT); @@ -362,8 +362,8 @@ defineSuite([ var packedTexture = clippingPlanes.texture; // Two RGBA float clipping planes consume 2 pixels of texture, allocation to be double that - expect(packedTexture.width).toEqual(4); - expect(packedTexture.height).toEqual(1); + expect(packedTexture.width).toEqual(2); + expect(packedTexture.height).toEqual(2); clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0)); clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0)); @@ -375,8 +375,8 @@ defineSuite([ packedTexture = clippingPlanes.texture; // Five RGBA float clipping planes consume 5 pixels of texture, allocation to be double that - expect(packedTexture.width).toEqual(10); - expect(packedTexture.height).toEqual(1); + expect(packedTexture.width).toEqual(5); + expect(packedTexture.height).toEqual(2); clippingPlanes.removeAll(); clippingPlanes.add(new ClippingPlane(Cartesian3.UNIT_X, 1.0)); @@ -387,8 +387,8 @@ defineSuite([ packedTexture = clippingPlanes.texture; // One RGBA float clipping plane consume 1 pixels of texture, allocation to be double that - expect(packedTexture.width).toEqual(2); - expect(packedTexture.height).toEqual(1); + expect(packedTexture.width).toEqual(1); + expect(packedTexture.height).toEqual(2); clippingPlanes.destroy(); scene.destroyForSpecs(); @@ -596,4 +596,31 @@ defineSuite([ clippingPlanes.remove(holdThisPlane); expect(clippingPlanes.clippingPlanesState).toEqual(1); }); + + it('provides a function for checking the texture resolution', function() { + spyOn(ClippingPlaneCollection, 'useFloatTexture').and.returnValue(false); + + var scene = createScene(); + clippingPlanes = new ClippingPlaneCollection({ + planes : planes, + enabled : false, + edgeColor : Color.RED, + modelMatrix : transform + }); + + // Predicted resolution before texture has been allocated + var predictedResolution = ClippingPlaneCollection.getTextureResolution(clippingPlanes, scene.frameState.context, new Cartesian2()); + + expect(predictedResolution.x).toEqual(4); + expect(predictedResolution.y).toEqual(2); + + clippingPlanes.update(scene.frameState); + var actualResolution = ClippingPlaneCollection.getTextureResolution(clippingPlanes, scene.frameState.context, new Cartesian2()); + + expect(predictedResolution.x).toEqual(actualResolution.x); + expect(predictedResolution.y).toEqual(actualResolution.y); + + clippingPlanes.destroy(); + scene.destroyForSpecs(); + }); }); From 7f4589848cb966dff5b38d989d7621306cfabfdb Mon Sep 17 00:00:00 2001 From: Kangning Li Date: Wed, 9 May 2018 10:32:03 -0400 Subject: [PATCH 2/5] 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..fc54699062e5 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 clipping 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() { From f2e3fc4ffbf93691048491ea008aa8dfeb08d8df Mon Sep 17 00:00:00 2001 From: Kangning Li Date: Wed, 23 May 2018 14:04:49 -0400 Subject: [PATCH 3/5] add check in Model-based 3D Tiles for tileset clipping planes reassignment --- Source/Scene/Batched3DModel3DTileContent.js | 6 ++++ Source/Scene/Instanced3DModel3DTileContent.js | 6 ++++ .../Scene/Batched3DModel3DTileContentSpec.js | 33 +++++++++++++++++++ .../Instanced3DModel3DTileContentSpec.js | 33 +++++++++++++++++++ 4 files changed, 78 insertions(+) diff --git a/Source/Scene/Batched3DModel3DTileContent.js b/Source/Scene/Batched3DModel3DTileContent.js index 2f6f2a350dd0..2bdb88848e36 100644 --- a/Source/Scene/Batched3DModel3DTileContent.js +++ b/Source/Scene/Batched3DModel3DTileContent.js @@ -513,6 +513,12 @@ define([ this._model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined; } + // If the model references a destroyed ClippingPlaneCollection due to the tileset's collection being replaced with a + // ClippingPlaneCollection that gives this tile the same clipping status, update the model to use the new ClippingPlaneCollection. + if (defined(tilesetClippingPlanes) && defined(this._model._clippingPlanes) && this._model._clippingPlanes.isDestroyed()) { + this._model._clippingPlanes = tilesetClippingPlanes; + } + this._model.update(frameState); // If any commands were pushed, add derived commands diff --git a/Source/Scene/Instanced3DModel3DTileContent.js b/Source/Scene/Instanced3DModel3DTileContent.js index 01f65513809f..e5cfa735c4c3 100644 --- a/Source/Scene/Instanced3DModel3DTileContent.js +++ b/Source/Scene/Instanced3DModel3DTileContent.js @@ -520,6 +520,12 @@ define([ // Link/Dereference directly to avoid ownership checks. model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined; } + + // If the model references a destroyed ClippingPlaneCollection due to the tileset's collection being replaced with a + // ClippingPlaneCollection that gives this tile the same clipping status, update the model to use the new ClippingPlaneCollection. + if (defined(tilesetClippingPlanes) && defined(model._clippingPlanes) && model._clippingPlanes.isDestroyed()) { + model._clippingPlanes = tilesetClippingPlanes; + } } this._modelInstanceCollection.update(frameState); diff --git a/Specs/Scene/Batched3DModel3DTileContentSpec.js b/Specs/Scene/Batched3DModel3DTileContentSpec.js index b1ac299a4efb..cb9ebbf4a892 100644 --- a/Specs/Scene/Batched3DModel3DTileContentSpec.js +++ b/Specs/Scene/Batched3DModel3DTileContentSpec.js @@ -319,6 +319,39 @@ defineSuite([ }); }); + it('Links model to tileset clipping planes if tileset clipping planes are reassigned', function() { + return Cesium3DTilesTester.loadTileset(scene, withBatchTableUrl).then(function(tileset) { + var tile = tileset._root; + var model = tile.content._model; + + expect(model.clippingPlanes).toBeUndefined(); + + var clippingPlaneCollection = new ClippingPlaneCollection({ + planes : [ + new ClippingPlane(Cartesian3.UNIT_X, 0.0) + ] + }); + tileset.clippingPlanes = clippingPlaneCollection; + clippingPlaneCollection.update(scene.frameState); + tile.update(tileset, scene.frameState); + + expect(model.clippingPlanes).toBeDefined(); + expect(model.clippingPlanes).toBe(tileset.clippingPlanes); + + var newClippingPlaneCollection = new ClippingPlaneCollection({ + planes : [ + new ClippingPlane(Cartesian3.UNIT_X, 0.0) + ] + }); + tileset.clippingPlanes = newClippingPlaneCollection; + newClippingPlaneCollection.update(scene.frameState); + expect(model.clippingPlanes).not.toBe(tileset.clippingPlanes); + + tile.update(tileset, scene.frameState); + expect(model.clippingPlanes).toBe(tileset.clippingPlanes); + }); + }); + it('rebuilds Model shaders when clipping planes change', function() { spyOn(Model, '_getClippingFunction').and.callThrough(); diff --git a/Specs/Scene/Instanced3DModel3DTileContentSpec.js b/Specs/Scene/Instanced3DModel3DTileContentSpec.js index c3626c773e57..dbc68181d84c 100644 --- a/Specs/Scene/Instanced3DModel3DTileContentSpec.js +++ b/Specs/Scene/Instanced3DModel3DTileContentSpec.js @@ -334,6 +334,39 @@ defineSuite([ }); }); + it('Links model to tileset clipping planes if tileset clipping planes are reassigned', function() { + return Cesium3DTilesTester.loadTileset(scene, withBatchTableUrl).then(function(tileset) { + var tile = tileset._root; + var model = tile.content._modelInstanceCollection._model; + + expect(model.clippingPlanes).toBeUndefined(); + + var clippingPlaneCollection = new ClippingPlaneCollection({ + planes : [ + new ClippingPlane(Cartesian3.UNIT_X, 0.0) + ] + }); + tileset.clippingPlanes = clippingPlaneCollection; + clippingPlaneCollection.update(scene.frameState); + tile.update(tileset, scene.frameState); + + expect(model.clippingPlanes).toBeDefined(); + expect(model.clippingPlanes).toBe(tileset.clippingPlanes); + + var newClippingPlaneCollection = new ClippingPlaneCollection({ + planes : [ + new ClippingPlane(Cartesian3.UNIT_X, 0.0) + ] + }); + tileset.clippingPlanes = newClippingPlaneCollection; + newClippingPlaneCollection.update(scene.frameState); + expect(model.clippingPlanes).not.toBe(tileset.clippingPlanes); + + tile.update(tileset, scene.frameState); + expect(model.clippingPlanes).toBe(tileset.clippingPlanes); + }); + }); + it('rebuilds Model shaders when clipping planes change', function() { spyOn(Model, '_getClippingFunction').and.callThrough(); From 82bfff7d3a6548627f41e2229b41028db9f7ab7f Mon Sep 17 00:00:00 2001 From: Kangning Li Date: Fri, 25 May 2018 09:53:49 -0400 Subject: [PATCH 4/5] reference check for clipping planes on b3dm and i3dm tiles instead of destroyed check --- Source/Scene/Batched3DModel3DTileContent.js | 4 ++-- Source/Scene/Instanced3DModel3DTileContent.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Source/Scene/Batched3DModel3DTileContent.js b/Source/Scene/Batched3DModel3DTileContent.js index 2bdb88848e36..9fb502c1e2d1 100644 --- a/Source/Scene/Batched3DModel3DTileContent.js +++ b/Source/Scene/Batched3DModel3DTileContent.js @@ -513,9 +513,9 @@ define([ this._model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined; } - // If the model references a destroyed ClippingPlaneCollection due to the tileset's collection being replaced with a + // If the model references a different ClippingPlaneCollection due to the tileset's collection being replaced with a // ClippingPlaneCollection that gives this tile the same clipping status, update the model to use the new ClippingPlaneCollection. - if (defined(tilesetClippingPlanes) && defined(this._model._clippingPlanes) && this._model._clippingPlanes.isDestroyed()) { + if (defined(tilesetClippingPlanes) && defined(this._model._clippingPlanes) && this._model._clippingPlanes !== tilesetClippingPlanes) { this._model._clippingPlanes = tilesetClippingPlanes; } diff --git a/Source/Scene/Instanced3DModel3DTileContent.js b/Source/Scene/Instanced3DModel3DTileContent.js index e5cfa735c4c3..20bab51e8e7a 100644 --- a/Source/Scene/Instanced3DModel3DTileContent.js +++ b/Source/Scene/Instanced3DModel3DTileContent.js @@ -523,7 +523,7 @@ define([ // If the model references a destroyed ClippingPlaneCollection due to the tileset's collection being replaced with a // ClippingPlaneCollection that gives this tile the same clipping status, update the model to use the new ClippingPlaneCollection. - if (defined(tilesetClippingPlanes) && defined(model._clippingPlanes) && model._clippingPlanes.isDestroyed()) { + if (defined(tilesetClippingPlanes) && defined(model._clippingPlanes) && model._clippingPlanes !== tilesetClippingPlanes) { model._clippingPlanes = tilesetClippingPlanes; } } From 4305545db086f8972b00093c15930eba4f0af426 Mon Sep 17 00:00:00 2001 From: Kangning Li Date: Fri, 25 May 2018 13:13:37 -0400 Subject: [PATCH 5/5] comment fix --- Source/Scene/Instanced3DModel3DTileContent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Scene/Instanced3DModel3DTileContent.js b/Source/Scene/Instanced3DModel3DTileContent.js index 20bab51e8e7a..5354eca55985 100644 --- a/Source/Scene/Instanced3DModel3DTileContent.js +++ b/Source/Scene/Instanced3DModel3DTileContent.js @@ -521,7 +521,7 @@ define([ model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined; } - // If the model references a destroyed ClippingPlaneCollection due to the tileset's collection being replaced with a + // If the model references a different ClippingPlaneCollection due to the tileset's collection being replaced with a // ClippingPlaneCollection that gives this tile the same clipping status, update the model to use the new ClippingPlaneCollection. if (defined(tilesetClippingPlanes) && defined(model._clippingPlanes) && model._clippingPlanes !== tilesetClippingPlanes) { model._clippingPlanes = tilesetClippingPlanes;