PR feedback

Author: likangning93

Source/Core/ClippingPlane.js

@@ -2,23 +2,20 @@ define([
         './Cartesian3',
         './Check',
         './defined',
-        './defineProperties',
-        './DeveloperError',
-        './Plane'
+        './defineProperties'
     ], function(
         Cartesian3,
         Check,
         defined,
-        defineProperties,
-        DeveloperError,
-        Plane
+        defineProperties
     ) {
     'use strict';
 
     /**
      * A Plane in Hessian Normal form to be used with ClippingPlaneCollection.
      * Compatible with mathematics functions in Plane.js
      *
+     * @alias ClippingPlane
      * @constructor
      *
      * @param {Cartesian3} normal The plane's normal (normalized).
@@ -108,6 +105,7 @@ define([
      * Clones the ClippingPlane without setting its ownership.
      * @param {ClippingPlane} clippingPlane The ClippingPlane to be cloned
      * @param {ClippingPlane} [result] The object on which to store the cloned parameters.
+     * @returns {ClippingPlane} a clone of the input ClippingPlane
      */
     ClippingPlane.clone = function(clippingPlane, result) {
         if (!defined(result)) {

Source/Core/ClippingPlaneCollection.js

@@ -419,6 +419,15 @@ define([
         }
     }
 
+    function computeTextureResolution(pixelsNeeded, result) {
+        var maxSize = ContextLimits.maximumTextureSize;
+        var width = Math.min(pixelsNeeded, maxSize);
+        var height = Math.ceil(pixelsNeeded / width);
+        result.x = width;
+        result.y = height;
+        return result;
+    }
+
     var textureResolutionScratch = new Cartesian2();
     /**
      * Called when {@link Viewer} or {@link CesiumWidget} render the scene to
@@ -637,15 +646,6 @@ define([
         return this._unionClippingRegions ? this._planes.length : -this._planes.length;
     };
 
-    function computeTextureResolution(pixelsNeeded, result) {
-        var maxSize = ContextLimits.maximumTextureSize;
-        var width = Math.min(pixelsNeeded, maxSize);
-        var height = Math.ceil(pixelsNeeded / width);
-        result.x = width;
-        result.y = height;
-        return result;
-    }
-
     /**
      * Sets the owner for the input ClippingPlaneCollection if there wasn't another owner.
      * Destroys the owner's previous ClippingPlaneCollection if setting is successful.

Source/Scene/Batched3DModel3DTileContent.js

@@ -71,11 +71,6 @@ define([
         this._batchIdAttributeName = undefined;
         this._diffuseAttributeOrUniformName = {};
 
-        /**
-         * @inheritdoc Cesium3DTileContent#clippingPlanesDirty
-         */
-        this.clippingPlanesDirty = false;
-
         /**
          * @inheritdoc Cesium3DTileContent#featurePropertiesDirty
          */
@@ -511,7 +506,7 @@ define([
 
         // Update clipping planes
         var tilesetClippingPlanes = this._tileset.clippingPlanes;
-        if (this.clippingPlanesDirty && defined(tilesetClippingPlanes)) {
+        if (this._tile.clippingPlanesDirty && defined(tilesetClippingPlanes)) {
             // Dereference the clipping planes from the model if they are irrelevant.
             // Link/Dereference directly to avoid ownership checks.
             // This will also trigger synchronous shader regeneration to remove or add the clipping plane and color blending code.

Source/Scene/Cesium3DTile.js

@@ -310,6 +310,16 @@ define([
          */
         this._optimChildrenWithinParent = Cesium3DTileOptimizationHint.NOT_COMPUTED;
 
+        /**
+         * Tracks if the tile's relationship with a ClippingPlaneCollection has changed with regards
+         * to the ClippingPlaneCollection's state.
+         *
+         * @type {Boolean}
+         *
+         * @private
+         */
+        this.clippingPlanesDirty = false;
+
         // Members that are updated every frame for tree traversal and rendering optimizations:
         this._distanceToCamera = 0;
         this._visibilityPlaneMask = 0;
@@ -1049,33 +1059,36 @@ define([
         content.update(tileset, frameState);
     }
 
-    /**
-     * Get the draw commands needed to render this tile.
-     *
-     * @private
-     */
-    Cesium3DTile.prototype.update = function(tileset, frameState) {
+    function updateClippingPlanes(tile, tileset) {
         // Compute and compare ClippingPlanes state:
         // - enabled-ness - are clipping planes enabled? is this tile clipped?
         // - clipping plane count
         // - clipping function (union v. intersection)
         var clippingPlanes = tileset.clippingPlanes;
         var currentClippingPlanesState = 0;
-        if (defined(clippingPlanes) && this._isClipped && clippingPlanes.enabled) {
+        if (defined(clippingPlanes) && tile._isClipped && clippingPlanes.enabled) {
             currentClippingPlanesState = clippingPlanes.clippingPlanesState();
         }
-        // If clippingPlaneState for this tile changed, mark content as clippingPlanesDirty
-        if (currentClippingPlanesState !== this._clippingPlanesState) {
-            this._clippingPlanesState = currentClippingPlanesState;
-            this._content.clippingPlanesDirty = true;
+        // If clippingPlaneState for tile changed, mark clippingPlanesDirty so content can update
+        if (currentClippingPlanesState !== tile._clippingPlanesState) {
+            tile._clippingPlanesState = currentClippingPlanesState;
+            tile.clippingPlanesDirty = true;
         }
+    }
 
+    /**
+     * Get the draw commands needed to render this tile.
+     *
+     * @private
+     */
+    Cesium3DTile.prototype.update = function(tileset, frameState) {
         var initCommandLength = frameState.commandList.length;
+        updateClippingPlanes(this, tileset);
         applyDebugSettings(this, tileset, frameState);
         updateContent(this, tileset, frameState);
         this._commandsLength = frameState.commandList.length - initCommandLength;
 
-        this._content.clippingPlanesDirty = false; // reset after content update
+        this.clippingPlanesDirty = false; // reset after content update
     };
 
     var scratchCommandList = [];

Source/Scene/Cesium3DTileContent.js

@@ -33,20 +33,6 @@ define([
          * @private
          */
         this.featurePropertiesDirty = false;
-
-        /**
-         * Gets or sets if the tile's relationship with a ClippingPlaneCollection has changed with regards
-         * to the ClippingPlaneCollection's state.
-         * <p>
-         * This is used to implement the <code>Cesium3DTileContent</code> interface, but is
-         * not part of the public Cesium API.
-         * </p>
-         *
-         * @type {Boolean}
-         *
-         * @private
-         */
-        this.clippingPlanesDirty = false;
     }
 
     defineProperties(Cesium3DTileContent.prototype, {

Source/Scene/Composite3DTileContent.js

@@ -44,11 +44,6 @@ define([
         this._contents = [];
         this._readyPromise = when.defer();
 
-        /**
-         * @inheritdoc Cesium3DTileContent#clippingPlanesDirty
-         */
-        this.clippingPlanesDirty = false;
-
         initialize(this, arrayBuffer, byteOffset, factory);
     }
 
@@ -287,9 +282,7 @@ define([
         var contents = this._contents;
         var length = contents.length;
         for (var i = 0; i < length; ++i) {
-            contents[i].clippingPlanesDirty = this.clippingPlanesDirty;
             contents[i].update(tileset, frameState);
-            contents[i].clippingPlanesDirty = false;
         }
     };
 

Source/Scene/Instanced3DModel3DTileContent.js

@@ -81,11 +81,6 @@ define([
         this._batchTable = undefined;
         this._features = undefined;
 
-        /**
-         * @inheritdoc Cesium3DTileContent#clippingPlanesDirty
-         */
-        this.clippingPlanesDirty = false;
-
         /**
          * @inheritdoc Cesium3DTileContent#featurePropertiesDirty
          */
@@ -520,7 +515,7 @@ define([
         if (defined(model)) {
             // Update for clipping planes
             var tilesetClippingPlanes = this._tileset.clippingPlanes;
-            if (this.clippingPlanesDirty && defined(tilesetClippingPlanes)) {
+            if (this._tile.clippingPlanesDirty && defined(tilesetClippingPlanes)) {
                 // Dereference the clipping planes from the model if they are irrelevant - saves on shading
                 // Link/Dereference directly to avoid ownership checks.
                 model._clippingPlanes = (tilesetClippingPlanes.enabled && this._tile._isClipped) ? tilesetClippingPlanes : undefined;

Source/Scene/Model.js

@@ -514,21 +514,13 @@ define([
          */
         this.colorBlendAmount = defaultValue(options.colorBlendAmount, 0.5);
 
-        this.colorShadingEnabled = isColorShadingEnabled(this);
+        this._colorShadingEnabled = isColorShadingEnabled(this);
 
         this._clippingPlanes = undefined;
         this.clippingPlanes = options.clippingPlanes;
         // Used for checking if shaders need to be regenerated due to clipping plane changes.
         this._clippingPlanesState = 0;
 
-        /**
-         * Flag for indicating to ModelInstanceCollection that shaders were changed, necessitating updates.
-         *
-         * @type {Boolean}
-         * @private
-         */
-        this.changedShadersOnLastUpdate = false;
-
         /**
          * This property is for debugging only; it is not for production use nor is it optimized.
          * <p>
@@ -1914,7 +1906,7 @@ define([
     };
 
     CreateProgramJob.prototype.execute = function() {
-        createProgram(this.id, this.model, this.context, true);
+        createProgram(this.id, this.model, this.context);
     };
 
     ///////////////////////////////////////////////////////////////////////////
@@ -4087,8 +4079,6 @@ define([
      * @exception {RuntimeError} Failed to load external reference.
      */
     Model.prototype.update = function(frameState) {
-        this.changedShadersOnLastUpdate = false;
-
         if (frameState.mode === SceneMode.MORPHING) {
             return;
         }
@@ -4320,8 +4310,8 @@ define([
 
             // Regenerate shaders if color shading changed from last update
             var currentlyColorShadingEnabled = isColorShadingEnabled(this);
-            if (currentlyColorShadingEnabled !== this.colorShadingEnabled) {
-                this.colorShadingEnabled = currentlyColorShadingEnabled;
+            if (currentlyColorShadingEnabled !== this._colorShadingEnabled) {
+                this._colorShadingEnabled = currentlyColorShadingEnabled;
                 shouldRegenerateShaders = true;
             }
 
@@ -4442,8 +4432,6 @@ define([
         var cachedRendererResources = model._cachedRendererResources;
         destroyIfNotCached(rendererResources, cachedRendererResources);
 
-        model.changedShadersOnLastUpdate = true;
-
         if (isClippingEnabled(model) || isColorShadingEnabled(model)) {
             rendererResources.programs = {};
             rendererResources.pickPrograms = {};

Source/Scene/ModelInstanceCollection.js

@@ -871,6 +871,19 @@ define([
         };
     }
 
+    function commandsDirty(model) {
+        var nodeCommands = model._nodeCommands;
+        var length = nodeCommands.length;
+
+        for (var i = 0; i < length; i++) {
+            var nc = nodeCommands[i];
+            if (nc.command.dirty) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     function generateModelCommands(modelInstanceCollection, instancingSupported) {
         modelInstanceCollection._drawCommands = [];
         modelInstanceCollection._pickCommands = [];
@@ -977,7 +990,7 @@ define([
         }
 
         // If the model was set to rebuild shaders during update, rebuild instanced commands.
-        if (model.changedShadersOnLastUpdate) {
+        if (commandsDirty(model)) {
             generateModelCommands(this, instancingSupported);
         }
 

Source/Scene/PointCloud3DTileContent.js

@@ -153,11 +153,6 @@ define([
          */
         this.featurePropertiesDirty = false;
 
-        /**
-         * @inheritdoc Cesium3DTileContent#clippingPlanesDirty
-         */
-        this.clippingPlanesDirty = false;
-
         // Options for geometric error based attenuation
         this._attenuation = false;
         this._geometricErrorScale = undefined;
@@ -1292,7 +1287,7 @@ define([
         }
 
         // update for clipping planes
-        if (this.clippingPlanesDirty) {
+        if (this._tile.clippingPlanesDirty) {
             createShaders(this, frameState, tileset.style);
         }
 

Specs/Scene/ModelInstanceCollectionSpec.js

@@ -550,6 +550,9 @@ defineSuite([
             expect(drawCommand.receiveShadows).toBe(true);
             collection.shadows = ShadowMode.DISABLED;
             scene.renderForSpecs();
+
+            // Expect commands to have been recreated
+            drawCommand = collection._drawCommands[0];
             expect(drawCommand.castShadows).toBe(false);
             expect(drawCommand.receiveShadows).toBe(false);
         });