Fix 1.67 ground polygon regression

mramato · mramato · commit 1be7513f3323 · 2020-03-19T10:52:00.000-04:00
The performance improvements #8630 did not take into account the limitation that ground primitives that overlap cannot be batched together. This change uses the `RectangleCollisionChecker` already implemented for `StaticGroundGeometryMaterialBatch` and performs the same check in `StaticGroundGeometryColorBatch`.
diff --git a/Source/DataSources/StaticGroundGeometryColorBatch.js b/Source/DataSources/StaticGroundGeometryColorBatch.js
@@ -8,17 +8,17 @@ import ShowGeometryInstanceAttribute from '../Core/ShowGeometryInstanceAttribute
 import GroundPrimitive from '../Scene/GroundPrimitive.js';
 import BoundingSphereState from './BoundingSphereState.js';
 import Property from './Property.js';
+import RectangleCollisionChecker from '../Core/RectangleCollisionChecker.js';
 
     var colorScratch = new Color();
     var distanceDisplayConditionScratch = new DistanceDisplayCondition();
     var defaultDistanceDisplayCondition = new DistanceDisplayCondition();
 
-    function Batch(primitives, classificationType, color, key, zIndex) {
+    function Batch(primitives, classificationType, color, zIndex) {
         this.primitives = primitives;
         this.zIndex = zIndex;
         this.classificationType = classificationType;
         this.color = color;
-        this.key = key;
         this.createPrimitive = false;
         this.waitingOnCreate = false;
         this.primitive = undefined;
@@ -31,13 +31,19 @@ import Property from './Property.js';
         this.showsUpdated = new AssociativeArray();
         this.itemsToRemove = [];
         this.isDirty = false;
+        this.rectangleCollisionCheck = new RectangleCollisionChecker();
     }
 
+    Batch.prototype.overlapping = function(rectangle) {
+        return this.rectangleCollisionCheck.collides(rectangle);
+    };
+
     Batch.prototype.add = function(updater, instance) {
         var id = updater.id;
         this.createPrimitive = true;
         this.geometry.set(id, instance);
         this.updaters.set(id, updater);
+        this.rectangleCollisionCheck.insert(id, instance.geometry.rectangle);
         if (!updater.hasConstantFill || !updater.fillMaterialProperty.isConstant || !Property.isConstant(updater.distanceDisplayConditionProperty)) {
             this.updatersWithAttributes.set(id, updater);
         } else {
@@ -52,8 +58,10 @@ import Property from './Property.js';
 
     Batch.prototype.remove = function(updater) {
         var id = updater.id;
+        var geometryInstance = this.geometry.get(id);
         this.createPrimitive = this.geometry.remove(id) || this.createPrimitive;
         if (this.updaters.remove(id)) {
+            this.rectangleCollisionCheck.remove(id, geometryInstance.geometry.rectangle);
             this.updatersWithAttributes.remove(id);
             var unsubscribe = this.subscriptions.get(id);
             if (defined(unsubscribe)) {
@@ -227,33 +235,39 @@ import Property from './Property.js';
      * @private
      */
     function StaticGroundGeometryColorBatch(primitives, classificationType) {
-        this._batches = new AssociativeArray();
+        this._batches = [];
         this._primitives = primitives;
         this._classificationType = classificationType;
     }
 
     StaticGroundGeometryColorBatch.prototype.add = function(time, updater) {
         var instance = updater.createFillGeometryInstance(time);
         var batches = this._batches;
-        // zIndex is a batch breaker, so we'll use that for the key
         var zIndex = Property.getValueOrDefault(updater.zIndex, 0);
-        var batchKey = zIndex;
         var batch;
-        if (batches.contains(batchKey)) {
-            batch = batches.get(batchKey);
-        } else {
-            batch = new Batch(this._primitives, this._classificationType, instance.attributes.color.value, batchKey, zIndex);
-            batches.set(batchKey, batch);
+        var length = batches.length;
+        for (var i = 0; i < length; ++i) {
+            var item = batches[i];
+            if (item.zIndex === zIndex &&
+                !item.overlapping(instance.geometry.rectangle)) {
+                batch = item;
+                break;
+            }
+        }
+
+        if (!defined(batch)) {
+            batch = new Batch(this._primitives, this._classificationType, instance.attributes.color.value, zIndex);
+            batches.push(batch);
         }
         batch.add(updater, instance);
         return batch;
     };
 
     StaticGroundGeometryColorBatch.prototype.remove = function(updater) {
-        var batchesArray = this._batches.values;
-        var count = batchesArray.length;
+        var batches = this._batches;
+        var count = batches.length;
         for (var i = 0; i < count; ++i) {
-            if (batchesArray[i].remove(updater)) {
+            if (batches[i].remove(updater)) {
                 return;
             }
         }
@@ -266,15 +280,14 @@ import Property from './Property.js';
         //Perform initial update
         var isUpdated = true;
         var batches = this._batches;
-        var batchesArray = batches.values;
-        var batchCount = batchesArray.length;
+        var batchCount = batches.length;
         for (i = 0; i < batchCount; ++i) {
-            isUpdated = batchesArray[i].update(time) && isUpdated;
+            isUpdated = batches[i].update(time) && isUpdated;
         }
 
         //If any items swapped between batches we need to move them
         for (i = 0; i < batchCount; ++i) {
-            var oldBatch = batchesArray[i];
+            var oldBatch = batches[i];
             var itemsToRemove = oldBatch.itemsToRemove;
             var itemsToMoveLength = itemsToRemove.length;
             for (var j = 0; j < itemsToMoveLength; j++) {
@@ -287,27 +300,25 @@ import Property from './Property.js';
         }
 
         //If we moved anything around, we need to re-build the primitive and remove empty batches
-        var batchesArrayCopy = batchesArray.slice();
-        var batchesCopyCount = batchesArrayCopy.length;
-        for (i = 0; i < batchesCopyCount; ++i) {
-            var batch = batchesArrayCopy[i];
+        for (i = batchCount - 1; i >= 0; --i) {
+            var batch = batches[i];
             if (batch.isDirty) {
-                isUpdated = batchesArrayCopy[i].update(time) && isUpdated;
+                isUpdated = batches[i].update(time) && isUpdated;
                 batch.isDirty = false;
             }
             if (batch.geometry.length === 0) {
-                batches.remove(batch.key);
+                batches.splice(i, 1);
             }
         }
 
         return isUpdated;
     };
 
     StaticGroundGeometryColorBatch.prototype.getBoundingSphere = function(updater, result) {
-        var batchesArray = this._batches.values;
-        var batchCount = batchesArray.length;
+        var batches = this._batches;
+        var batchCount = batches.length;
         for (var i = 0; i < batchCount; ++i) {
-            var batch = batchesArray[i];
+            var batch = batches[i];
             if (batch.contains(updater)) {
                 return batch.getBoundingSphere(updater, result);
             }
@@ -317,10 +328,10 @@ import Property from './Property.js';
     };
 
     StaticGroundGeometryColorBatch.prototype.removeAllPrimitives = function() {
-        var batchesArray = this._batches.values;
-        var batchCount = batchesArray.length;
+        var batches = this._batches;
+        var batchCount = batches.length;
         for (var i = 0; i < batchCount; ++i) {
-            batchesArray[i].removeAllPrimitives();
+            batches[i].removeAllPrimitives();
         }
     };
 export default StaticGroundGeometryColorBatch;
diff --git a/Specs/DataSources/StaticGroundGeometryColorBatchSpec.js b/Specs/DataSources/StaticGroundGeometryColorBatchSpec.js
@@ -1,7 +1,6 @@
 import { ApproximateTerrainHeights } from '../../Source/Cesium.js';
 import { Cartesian3 } from '../../Source/Cesium.js';
 import { Color } from '../../Source/Cesium.js';
-import { defaultValue } from '../../Source/Cesium.js';
 import { DistanceDisplayCondition } from '../../Source/Cesium.js';
 import { JulianDate } from '../../Source/Cesium.js';
 import { Math as CesiumMath } from '../../Source/Cesium.js';
@@ -37,10 +36,6 @@ describe('DataSources/StaticGroundGeometryColorBatch', function() {
         ApproximateTerrainHeights._terrainHeights = undefined;
     });
 
-    function computeKey(zIndex) {
-        return defaultValue(zIndex, 0);
-    }
-
     it('updates color attribute after rebuilding primitive', function() {
         if (!GroundPrimitive.isSupported(scene)) {
             return;
@@ -72,13 +67,10 @@ describe('DataSources/StaticGroundGeometryColorBatch', function() {
             var primitive = scene.groundPrimitives.get(0);
             var attributes = primitive.getGeometryInstanceAttributes(entity);
             var red = [255, 0, 0, 255];
-            var redKey = computeKey();
             expect(attributes.color).toEqual(red);
 
-            // Verify we have 1 batch with the key for red
+            // Verify we have 1 batch
             expect(batch._batches.length).toEqual(1);
-            expect(batch._batches.contains(redKey)).toBe(true);
-            expect(batch._batches.get(redKey).key).toEqual(redKey);
 
             entity.ellipse.material = Color.GREEN;
             updater._onEntityPropertyChanged(entity, 'ellipse');
@@ -94,19 +86,55 @@ describe('DataSources/StaticGroundGeometryColorBatch', function() {
                 var primitive = scene.groundPrimitives.get(0);
                 var attributes = primitive.getGeometryInstanceAttributes(entity);
                 var green = [0, 128, 0, 255];
-                var greenKey = computeKey();
                 expect(attributes.color).toEqual(green);
 
                 // Verify we have 1 batch with the key for green
                 expect(batch._batches.length).toEqual(1);
-                expect(batch._batches.contains(greenKey)).toBe(true);
-                expect(batch._batches.get(greenKey).key).toEqual(greenKey);
 
                 batch.removeAllPrimitives();
             });
         });
     });
 
+    it('batches overlapping geometry separately', function() {
+        if (!GroundPrimitive.isSupported(scene)) {
+            return;
+        }
+
+        var batch = new StaticGroundGeometryColorBatch(scene.groundPrimitives, ClassificationType.BOTH);
+        var entity = new Entity({
+            position : new Cartesian3(1234, 5678, 9101112),
+            ellipse : {
+                semiMajorAxis : 2,
+                semiMinorAxis : 1,
+                show : new CallbackProperty(function() {
+                    return true;
+                }, false),
+                material : Color.RED
+            }
+        });
+
+        var entity2 = new Entity({
+            position : new Cartesian3(1234, 5678, 9101112),
+            ellipse : {
+                semiMajorAxis : 2,
+                semiMinorAxis : 1,
+                show : new CallbackProperty(function() {
+                    return true;
+                }, false),
+                material : Color.RED
+            }
+        });
+
+        var updater = new EllipseGeometryUpdater(entity, scene);
+        batch.add(time, updater);
+
+        var updater2 = new EllipseGeometryUpdater(entity2, scene);
+        batch.add(time, updater2);
+
+        expect(batch._batches.length).toEqual(2);
+    });
+
     it('updates with sampled distance display condition out of range', function() {
         var validTime = JulianDate.fromIso8601('2018-02-14T04:10:00+1100');
         var outOfRangeTime = JulianDate.fromIso8601('2018-02-14T04:20:00+1100');
@@ -205,18 +233,6 @@ describe('DataSources/StaticGroundGeometryColorBatch', function() {
         }
 
         var batch = new StaticGroundGeometryColorBatch(scene.groundPrimitives, ClassificationType.BOTH);
-        function buildEntity() {
-            return new Entity({
-                position : new Cartesian3(1234, 5678, 9101112),
-                ellipse : {
-                    semiMajorAxis : 2,
-                    semiMinorAxis : 1,
-                    height : 0,
-                    outline : true,
-                    outlineColor : Color.RED.withAlpha(0.5)
-                }
-            });
-        }
 
         function renderScene() {
             scene.initializeFrame();
@@ -225,8 +241,25 @@ describe('DataSources/StaticGroundGeometryColorBatch', function() {
             return isUpdated;
         }
 
-        var entity1 = buildEntity();
-        var entity2 = buildEntity();
+        var entity1 = new Entity({
+            position : new Cartesian3(1234, 5678, 9101112),
+            ellipse : {
+                semiMajorAxis : 0.2,
+                semiMinorAxis : 0.1,
+                outline : true,
+                outlineColor : Color.RED.withAlpha(0.5)
+            }
+        });
+
+        var entity2 = new Entity({
+            position : new Cartesian3(1234, 4678, 9101112),
+            ellipse : {
+                semiMajorAxis : 0.2,
+                semiMinorAxis : 0.1,
+                outline : true,
+                outlineColor : Color.RED.withAlpha(0.5)
+            }
+        });
 
         var updater1 = new EllipseGeometryUpdater(entity1, scene);
         var updater2 = new EllipseGeometryUpdater(entity2, scene);
@@ -272,18 +305,6 @@ describe('DataSources/StaticGroundGeometryColorBatch', function() {
         }
 
         var batch = new StaticGroundGeometryColorBatch(scene.groundPrimitives, ClassificationType.BOTH);
-        function buildEntity() {
-            return new Entity({
-                position : new Cartesian3(1234, 5678, 9101112),
-                ellipse : {
-                    semiMajorAxis : 2,
-                    semiMinorAxis : 1,
-                    height : 0,
-                    outline : true,
-                    outlineColor : Color.RED.withAlpha(0.5)
-                }
-            });
-        }
 
         function renderScene() {
             scene.initializeFrame();
@@ -292,11 +313,27 @@ describe('DataSources/StaticGroundGeometryColorBatch', function() {
             return isUpdated;
         }
 
-        var entity1 = buildEntity();
+        var entity1 = new Entity({
+            position : new Cartesian3(1234, 5678, 9101112),
+            ellipse : {
+                semiMajorAxis : 0.2,
+                semiMinorAxis : 0.1,
+                outline : true,
+                outlineColor : Color.RED.withAlpha(0.5)
+            }
+        });
         var updater1 = new EllipseGeometryUpdater(entity1, scene);
         batch.add(time, updater1);
 
-        var entity2 = buildEntity();
+        var entity2 = new Entity({
+            position : new Cartesian3(1234, 4678, 9101112),
+            ellipse : {
+                semiMajorAxis : 0.2,
+                semiMinorAxis : 0.1,
+                outline : true,
+                outlineColor : Color.RED.withAlpha(0.5)
+            }
+        });
         var updater2 = new EllipseGeometryUpdater(entity2, scene);
 
         return pollToPromise(renderScene)
