Skip to content

Commit

Permalink
Merge pull request #6576 from likangning93/fixClippingPlaneCrash
Browse files Browse the repository at this point in the history
fix crash due to undefined clipping planes texture
  • Loading branch information
lilleyse authored May 25, 2018
2 parents 0afe61c + 4305545 commit 679df9f
Show file tree
Hide file tree
Showing 11 changed files with 299 additions and 35 deletions.
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Change Log
* Fixed incorrect 3D Tiles statistics when a tile fails during processing. [#6558](https:/AnalyticalGraphicsInc/cesium/pull/6558)
* Fixed race condition causing intermittent crash when changing geometry show value [#3061](https:/AnalyticalGraphicsInc/cesium/issues/3061)
* `ProviderViewModel`s with no category are displayed in an untitled group in `BaseLayerPicker` instead of being labeled as `'Other'` [#6574](https:/AnalyticalGraphicsInc/cesium/pull/6574)
* Fixed a bug causing intermittent crashes with clipping planes due to uninitialized textures. [#6576](https:/AnalyticalGraphicsInc/cesium/pull/6576)
* Added a workaround for clipping planes causing a picking shader compilation failure for gltf models and 3D Tilesets in Internet Explorer [#6575](https:/AnalyticalGraphicsInc/cesium/issues/6575)
* Allowed Bing Maps servers with a subpath (instead of being at the root) to work correctly. [#6597](https:/AnalyticalGraphicsInc/cesium/pull/6597)
* Added support for loading of Draco compressed glTF assets in IE11 [#6404](https:/AnalyticalGraphicsInc/cesium/issues/6404)
Expand Down
6 changes: 6 additions & 0 deletions Source/Scene/Batched3DModel3DTileContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,12 @@ define([
this._model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined;
}

// 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 !== tilesetClippingPlanes) {
this._model._clippingPlanes = tilesetClippingPlanes;
}

this._model.update(frameState);

// If any commands were pushed, add derived commands
Expand Down
40 changes: 37 additions & 3 deletions Source/Scene/ClippingPlaneCollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -502,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,
Expand Down Expand Up @@ -665,6 +672,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.
* <br /><br />
Expand Down
2 changes: 1 addition & 1 deletion Source/Scene/GlobeSurfaceShaderSet.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 6 additions & 0 deletions Source/Scene/Instanced3DModel3DTileContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,12 @@ define([
// Link/Dereference directly to avoid ownership checks.
model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined;
}

// 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;
}
}

this._modelInstanceCollection.update(frameState);
Expand Down
8 changes: 4 additions & 4 deletions Source/Scene/Model.js
Original file line number Diff line number Diff line change
Expand Up @@ -2043,7 +2043,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);
Expand All @@ -2066,7 +2066,7 @@ define([
}

if (addClippingPlaneCode) {
pickFS = modifyShaderForClippingPlanes(pickFS, clippingPlaneCollection);
pickFS = modifyShaderForClippingPlanes(pickFS, clippingPlaneCollection, context);
}

if (!FeatureDetection.isInternetExplorer()) {
Expand Down Expand Up @@ -3960,9 +3960,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' +
Expand Down
2 changes: 1 addition & 1 deletion Source/Scene/PointCloud3DTileContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -1087,7 +1087,7 @@ define([
'uniform mat4 u_clippingPlanesMatrix; \n' +
'uniform vec4 u_clippingPlanesEdgeStyle; \n';
fs += '\n';
fs += getClippingFunction(clippingPlanes);
fs += getClippingFunction(clippingPlanes, context);
fs += '\n';
}

Expand Down
27 changes: 17 additions & 10 deletions Source/Scene/getClippingFunction.js
Original file line number Diff line number Diff line change
@@ -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' +
Expand Down Expand Up @@ -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' +
Expand Down
33 changes: 33 additions & 0 deletions Specs/Scene/Batched3DModel3DTileContentSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
Loading

0 comments on commit 679df9f

Please sign in to comment.