Merge pull request #8546 from AnalyticalGraphicsInc/8321/polyline_crash

8321/polyline crash
CesiumGS · Jan 23, 2020 · 3f35764 · 3f35764
2 parents 4ac1ae5 + 080a3fa
commit 3f35764
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 21 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -41,6 +41,7 @@ Change Log
 * Fixed a bug where toggling point cloud classification visibility would result in a grey screen on Linux / Nvidia [#8538](https://github.com/AnalyticalGraphicsInc/cesium/pull/8538)
 * Fixed a bug where a point in a `PointPrimitiveCollection` is rendered in the middle of the screen instead of being clipped. [#8542](https://github.com/AnalyticalGraphicsInc/cesium/pull/8542)
 * Fixed a crash when deleting and re-creating polylines from CZML. `ReferenceProperty` now returns undefined when the target entity or property does not exist, instead of throwing. [#8544](https://github.com/AnalyticalGraphicsInc/cesium/pull/8544)
+* Fixed a bug where rapidly updating a PolylineCollection could result in an instanceIndex is out of range error [#8546](https://github.com/AnalyticalGraphicsInc/cesium/pull/8546)
 
 ### 1.65.0 - 2020-01-06
 

diff --git a/Source/Scene/Polyline.js b/Source/Scene/Polyline.js
@@ -285,6 +285,19 @@ import Material from './Material.js';
             }
         },
 
+        /**
+         * Gets the destruction status of this polyline
+         * @memberof Polyline.prototype
+         * @type {Boolean}
+         * @default false
+         * @private
+         */
+        isDestroyed : {
+            get : function() {
+                return !defined(this._polylineCollection);
+            }
+        },
+
         /**
          * Gets or sets the condition specifying at what distance from the camera that this polyline will be displayed.
          * @memberof Polyline.prototype

diff --git a/Source/Scene/PolylineCollection.js b/Source/Scene/PolylineCollection.js
@@ -257,13 +257,6 @@ import SceneMode from './SceneMode.js';
      */
     PolylineCollection.prototype.remove = function(polyline) {
         if (this.contains(polyline)) {
-            this._polylines[polyline._index] = undefined; // Removed later
-
-            var polylineUpdateIndex = this._polylinesToUpdate.indexOf(polyline);
-            if (polylineUpdateIndex !== -1) {
-                this._polylinesToUpdate.splice(polylineUpdateIndex, 1);
-            }
-
             this._polylinesRemoved = true;
             this._createVertexArray = true;
             this._createBatchTable = true;
@@ -1049,27 +1042,31 @@ import SceneMode from './SceneMode.js';
     function removePolylines(collection) {
         if (collection._polylinesRemoved) {
             collection._polylinesRemoved = false;
-
-            var polylines = [];
+            var definedPolylines = [];
+            var definedPolylinesToUpdate = [];
+            var polyIndex = 0;
+            var polyline;
 
             var length = collection._polylines.length;
-            for ( var i = 0, j = 0; i < length; ++i) {
-                var polyline = collection._polylines[i];
-                if (defined(polyline)) {
-                    polyline._index = j++;
-                    polylines.push(polyline);
+            for (var i = 0; i < length; ++i) {
+                polyline = collection._polylines[i];
+                if (!polyline.isDestroyed) {
+                    polyline._index = polyIndex++;
+                    definedPolylinesToUpdate.push(polyline);
+                    definedPolylines.push(polyline);
                 }
             }
 
-            collection._polylines = polylines;
+            collection._polylines = definedPolylines;
+            collection._polylinesToUpdate = definedPolylinesToUpdate;
         }
     }
 
     function releaseShaders(collection) {
         var polylines = collection._polylines;
         var length = polylines.length;
         for ( var i = 0; i < length; ++i) {
-            if (defined(polylines[i])) {
+            if (!polylines[i].isDestroyed) {
                 var bucket = polylines[i]._bucket;
                 if (defined(bucket)) {
                     bucket.shaderProgram = bucket.shaderProgram && bucket.shaderProgram.destroy();
@@ -1098,7 +1095,7 @@ import SceneMode from './SceneMode.js';
         var polylines = collection._polylines;
         var length = polylines.length;
         for ( var i = 0; i < length; ++i) {
-            if (defined(polylines[i])) {
+            if (!polylines[i].isDestroyed) {
                 polylines[i]._destroy();
             }
         }

diff --git a/Specs/Scene/PolylineCollectionSpec.js b/Specs/Scene/PolylineCollectionSpec.js
@@ -316,31 +316,32 @@ describe('Scene/PolylineCollection', function() {
         expect(polylines.length).toEqual(0);
     });
 
-    it('removes a polyline from the updated list when removed', function() {
+    it('removal of polyline from polyLinesToUpdate is deferred until scene is updated', function() {
         var firstPolyline = polylines.add();
         var secondPolyline = polylines.add();
 
         firstPolyline.width = 4;
         secondPolyline.width = 5;
 
         expect(polylines._polylinesToUpdate.length).toEqual(2);
-
         polylines.remove(secondPolyline);
+        polylines.update(scene.frameState);
 
         expect(polylines._polylinesToUpdate.length).toEqual(1);
     });
 
-    it('only adds polyline to the update list once', function() {
+    it('removal of polyline from polylinesToUpdate after polyline is made dirty multiple times', function() {
         var firstPolyline = polylines.add();
         var secondPolyline = polylines.add();
 
         firstPolyline.width = 4;
         secondPolyline.width = 5;
-        secondPolyline.width = 7;
+        secondPolyline.width = 7; // Making the polyline dirty twice shouldn't affect the length of `_polylinesToUpdate`
 
         expect(polylines._polylinesToUpdate.length).toEqual(2);
 
         polylines.remove(secondPolyline);
+        polylines.update(scene.frameState);
 
         expect(polylines._polylinesToUpdate.length).toEqual(1);
     });