From 360c4c439e72317a15139f9aa1334e9f6d0d5578 Mon Sep 17 00:00:00 2001 From: Mike Macaulay Date: Mon, 10 Feb 2014 10:04:53 -0600 Subject: [PATCH 1/8] added dontRecomposite --- .../CompositeDynamicObjectCollection.js | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/Source/DynamicScene/CompositeDynamicObjectCollection.js b/Source/DynamicScene/CompositeDynamicObjectCollection.js index e811a07d9147..dc0ea70c2c53 100644 --- a/Source/DynamicScene/CompositeDynamicObjectCollection.js +++ b/Source/DynamicScene/CompositeDynamicObjectCollection.js @@ -264,7 +264,7 @@ define(['../Core/createGuid', * @exception {DeveloperError} collection is required. * @exception {DeveloperError} index, if supplied, must be greater than or equal to zero and less than or equal to the number of collections. */ - CompositeDynamicObjectCollection.prototype.addCollection = function(collection, index) { + CompositeDynamicObjectCollection.prototype.addCollection = function(collection, index, dontRecomposite) { var hasIndex = defined(index); //>>includeStart('debug', pragmas.debug); if (!defined(collection)) { @@ -285,8 +285,10 @@ define(['../Core/createGuid', } else { this._collections.splice(index, 0, collection); } + if(!dontRecomposite){ + recomposite(this); + } - recomposite(this); }; /** @@ -298,11 +300,13 @@ define(['../Core/createGuid', * @returns {Boolean} true if the collection was in the composite and was removed, * false if the collection was not in the composite. */ - CompositeDynamicObjectCollection.prototype.removeCollection = function(collection) { + CompositeDynamicObjectCollection.prototype.removeCollection = function(collection, dontRecomposite) { var index = this._collections.indexOf(collection); if (index !== -1) { this._collections.splice(index, 1); - recomposite(this); + if(!dontRecomposite){ + recomposite(this); + } return true; } return false; @@ -463,6 +467,10 @@ define(['../Core/createGuid', recomposite(this); }; + CompositeDynamicObjectCollection.prototype.recomposite = function() { + recomposite(this); + }; + /** * Prevents {@link DynamicObjectCollection#collectionChanged} events from being raised * until a corresponding call is made to {@link DynamicObjectCollection#resumeEvents}, at which From e0af0feac2c70dfe1aaf5d55e898afe53bcc38cb Mon Sep 17 00:00:00 2001 From: Mike Macaulay Date: Sat, 15 Feb 2014 16:35:54 -0600 Subject: [PATCH 2/8] SuspendEvents also suspends recompositing on CompositeDynamicObjectCollection Issue: https://github.com/AnalyticalGraphicsInc/cesium/issues/1452 --- .../CompositeDynamicObjectCollection.js | 122 +++++++++++------- .../CompositeDynamicObjectCollectionSpec.js | 31 +++++ 2 files changed, 104 insertions(+), 49 deletions(-) diff --git a/Source/DynamicScene/CompositeDynamicObjectCollection.js b/Source/DynamicScene/CompositeDynamicObjectCollection.js index dc0ea70c2c53..fbd58f42ed5c 100644 --- a/Source/DynamicScene/CompositeDynamicObjectCollection.js +++ b/Source/DynamicScene/CompositeDynamicObjectCollection.js @@ -142,66 +142,69 @@ define(['../Core/createGuid', } function recomposite(that) { - var collections = that._collections; - var collectionsLength = collections.length; + that.shouldRecomposite = true; + if (that._suspendCount === 0) { + var collections = that._collections; + var collectionsLength = collections.length; - var collectionsCopy = that._collectionsCopy; - var collectionsCopyLength = collectionsCopy.length; + var collectionsCopy = that._collectionsCopy; + var collectionsCopyLength = collectionsCopy.length; - var i; - var object; - var objects; - var iObjects; - var collection; - var composite = that._composite; - var newObjects = new DynamicObjectCollection(); - var eventHash = that._eventHash; - var collectionId; - - for (i = 0; i < collectionsCopyLength; i++) { - collection = collectionsCopy[i]; - collection.collectionChanged.removeEventListener(CompositeDynamicObjectCollection.prototype._onCollectionChanged, that); - objects = collection.getObjects(); - collectionId = collection.id; - for (iObjects = objects.length - 1; iObjects > -1; iObjects--) { - object = objects[iObjects]; - unsubscribeFromDynamicObject(that, eventHash, collectionId, object); + var i; + var object; + var objects; + var iObjects; + var collection; + var composite = that._composite; + var newObjects = new DynamicObjectCollection(); + var eventHash = that._eventHash; + var collectionId; + + for (i = 0; i < collectionsCopyLength; i++) { + collection = collectionsCopy[i]; + collection.collectionChanged.removeEventListener(CompositeDynamicObjectCollection.prototype._onCollectionChanged, that); + objects = collection.getObjects(); + collectionId = collection.id; + for (iObjects = objects.length - 1; iObjects > -1; iObjects--) { + object = objects[iObjects]; + unsubscribeFromDynamicObject(that, eventHash, collectionId, object); + } } - } - for (i = collectionsLength - 1; i >= 0; i--) { - collection = collections[i]; - collection.collectionChanged.addEventListener(CompositeDynamicObjectCollection.prototype._onCollectionChanged, that); + for (i = collectionsLength - 1; i >= 0; i--) { + collection = collections[i]; + collection.collectionChanged.addEventListener(CompositeDynamicObjectCollection.prototype._onCollectionChanged, that); - //Merge all of the existing objects. - objects = collection.getObjects(); - collectionId = collection.id; - for (iObjects = objects.length - 1; iObjects > -1; iObjects--) { - object = objects[iObjects]; - subscribeToDynamicObject(that, eventHash, collectionId, object); + //Merge all of the existing objects. + objects = collection.getObjects(); + collectionId = collection.id; + for (iObjects = objects.length - 1; iObjects > -1; iObjects--) { + object = objects[iObjects]; + subscribeToDynamicObject(that, eventHash, collectionId, object); - var compositeObject = newObjects.getById(object.id); - if (!defined(compositeObject)) { - compositeObject = composite.getById(object.id); + var compositeObject = newObjects.getById(object.id); if (!defined(compositeObject)) { - compositeObject = new DynamicObject(object.id); - } else { - clean(compositeObject); + compositeObject = composite.getById(object.id); + if (!defined(compositeObject)) { + compositeObject = new DynamicObject(object.id); + } else { + clean(compositeObject); + } + newObjects.add(compositeObject); } - newObjects.add(compositeObject); + compositeObject.merge(object); } - compositeObject.merge(object); } - } - that._collectionsCopy = collections.slice(0); + that._collectionsCopy = collections.slice(0); - composite.suspendEvents(); - composite.removeAll(); - var newObjectsArray = newObjects.getObjects(); - for (i = 0; i < newObjectsArray.length; i++) { - composite.add(newObjectsArray[i]); + composite.suspendEvents(); + composite.removeAll(); + var newObjectsArray = newObjects.getObjects(); + for (i = 0; i < newObjectsArray.length; i++) { + composite.add(newObjectsArray[i]); + } + composite.resumeEvents(); } - composite.resumeEvents(); return true; } @@ -220,11 +223,13 @@ define(['../Core/createGuid', */ var CompositeDynamicObjectCollection = function(collections) { this._composite = new DynamicObjectCollection(); + this._suspendCount = 0; this._collections = defined(collections) ? collections.slice() : []; this._collectionsCopy = []; this._id = createGuid(); this._eventHash = {}; recomposite(this); + this.shouldRecomposite = false; }; defineProperties(CompositeDynamicObjectCollection.prototype, { @@ -476,18 +481,22 @@ define(['../Core/createGuid', * until a corresponding call is made to {@link DynamicObjectCollection#resumeEvents}, at which * point a single event will be raised that covers all suspended operations. * This allows for many items to be added and removed efficiently. + * While events are suspended, recompositing of the collections will + * also be suspended, as this can be a costly operation. * This function can be safely called multiple times as long as there * are corresponding calls to {@link DynamicObjectCollection#resumeEvents}. * @memberof CompositeDynamicObjectCollection */ CompositeDynamicObjectCollection.prototype.suspendEvents = function() { + this._suspendCount++; this._composite.suspendEvents(); }; /** * Resumes raising {@link DynamicObjectCollection#collectionChanged} events immediately * when an item is added or removed. Any modifications made while while events were suspended - * will be triggered as a single event when this function is called. + * will be triggered as a single event when this function is called. This function also ensures + * the collection is recomposited if events are also resumed. * This function is reference counted and can safely be called multiple times as long as there * are corresponding calls to {@link DynamicObjectCollection#resumeEvents}. * @memberof CompositeDynamicObjectCollection @@ -495,7 +504,22 @@ define(['../Core/createGuid', * @exception {DeveloperError} resumeEvents can not be called before suspendEvents. */ CompositeDynamicObjectCollection.prototype.resumeEvents = function() { + //>>includeStart('debug', pragmas.debug); + if (this._suspendCount === 0) { + throw new DeveloperError('resumeEvents can not be called before suspendEvents.'); + } + //>>includeEnd('debug'); + + this._suspendCount--; + // recomposite before triggering events (but only if required for performance) that might depend on a composited collection + if(this.shouldRecomposite && this._suspendCount === 0){ + recomposite(this); + this.shouldRecomposite = false; + + } + this._composite.resumeEvents(); + }; /** diff --git a/Specs/DynamicScene/CompositeDynamicObjectCollectionSpec.js b/Specs/DynamicScene/CompositeDynamicObjectCollectionSpec.js index 6ce3de381629..7b283efba900 100644 --- a/Specs/DynamicScene/CompositeDynamicObjectCollectionSpec.js +++ b/Specs/DynamicScene/CompositeDynamicObjectCollectionSpec.js @@ -553,6 +553,37 @@ defineSuite([ expect(composite2.getById(id).position).toBe(dynamicObject2.position); }); + it('suspend events suspends recompositing', function () { + var id = 'test'; + var collection1 = new DynamicObjectCollection(); + var dynamicObject1 = new DynamicObject(id); + collection1.add(dynamicObject1); + + var collection2 = new DynamicObjectCollection(); + var dynamicObject2 = new DynamicObject(id); + collection2.add(dynamicObject2); + //Add collections in reverse order to lower numbers of priority + var composite = new CompositeDynamicObjectCollection(); + composite.addCollection(collection2); + + // suspend events + composite.suspendEvents(); + composite.addCollection(collection1); + + // add a billboard + var compositeObject = composite.getById(id); + dynamicObject1.billboard = new DynamicBillboard(); + dynamicObject1.billboard.show = new ConstantProperty(false); + // should be undefined because we haven't recomposited + expect(compositeObject.billboard).toBeUndefined(); + // resume events + composite.resumeEvents(); + + expect(compositeObject.billboard.show).toBe(dynamicObject1.billboard.show); + + + }); + it('prevents names from colliding between property events and object events', function() { var id = 'test'; var collection1 = new DynamicObjectCollection(); From 543f47a1aa5e63f24ceafddb2cd8d42555c92dec Mon Sep 17 00:00:00 2001 From: Mike Macaulay Date: Sat, 15 Feb 2014 16:51:30 -0600 Subject: [PATCH 3/8] make `shouldRecomposite` 'private' as `_shouldRecomposite` --- Source/DynamicScene/CompositeDynamicObjectCollection.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Source/DynamicScene/CompositeDynamicObjectCollection.js b/Source/DynamicScene/CompositeDynamicObjectCollection.js index fbd58f42ed5c..3eb41f6c15c1 100644 --- a/Source/DynamicScene/CompositeDynamicObjectCollection.js +++ b/Source/DynamicScene/CompositeDynamicObjectCollection.js @@ -142,7 +142,7 @@ define(['../Core/createGuid', } function recomposite(that) { - that.shouldRecomposite = true; + that._shouldRecomposite = true; if (that._suspendCount === 0) { var collections = that._collections; var collectionsLength = collections.length; @@ -229,7 +229,7 @@ define(['../Core/createGuid', this._id = createGuid(); this._eventHash = {}; recomposite(this); - this.shouldRecomposite = false; + this._shouldRecomposite = false; }; defineProperties(CompositeDynamicObjectCollection.prototype, { @@ -512,9 +512,9 @@ define(['../Core/createGuid', this._suspendCount--; // recomposite before triggering events (but only if required for performance) that might depend on a composited collection - if(this.shouldRecomposite && this._suspendCount === 0){ + if(this._shouldRecomposite && this._suspendCount === 0){ recomposite(this); - this.shouldRecomposite = false; + this._shouldRecomposite = false; } From 198d9c02cea52ca99ee02087c468ad0ca7fe9fd2 Mon Sep 17 00:00:00 2001 From: Mike Macaulay Date: Sat, 15 Feb 2014 17:02:51 -0600 Subject: [PATCH 4/8] performance fixes --- .../CompositeDynamicObjectCollection.js | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/Source/DynamicScene/CompositeDynamicObjectCollection.js b/Source/DynamicScene/CompositeDynamicObjectCollection.js index 3eb41f6c15c1..52fe2ed0cf8b 100644 --- a/Source/DynamicScene/CompositeDynamicObjectCollection.js +++ b/Source/DynamicScene/CompositeDynamicObjectCollection.js @@ -266,10 +266,9 @@ define(['../Core/createGuid', * @param {Number} [index] the index to add the collection at. If omitted, the collection will * added on top of all existing collections. * - * @exception {DeveloperError} collection is required. * @exception {DeveloperError} index, if supplied, must be greater than or equal to zero and less than or equal to the number of collections. */ - CompositeDynamicObjectCollection.prototype.addCollection = function(collection, index, dontRecomposite) { + CompositeDynamicObjectCollection.prototype.addCollection = function(collection, index) { var hasIndex = defined(index); //>>includeStart('debug', pragmas.debug); if (!defined(collection)) { @@ -290,10 +289,8 @@ define(['../Core/createGuid', } else { this._collections.splice(index, 0, collection); } - if(!dontRecomposite){ - recomposite(this); - } + recomposite(this); }; /** @@ -305,13 +302,11 @@ define(['../Core/createGuid', * @returns {Boolean} true if the collection was in the composite and was removed, * false if the collection was not in the composite. */ - CompositeDynamicObjectCollection.prototype.removeCollection = function(collection, dontRecomposite) { + CompositeDynamicObjectCollection.prototype.removeCollection = function(collection) { var index = this._collections.indexOf(collection); if (index !== -1) { this._collections.splice(index, 1); - if(!dontRecomposite){ - recomposite(this); - } + recomposite(this); return true; } return false; @@ -353,8 +348,6 @@ define(['../Core/createGuid', * @memberof CompositeDynamicObjectCollection * * @param {Number} index the index to retrieve. - * - * @exception {DeveloperError} index is required. */ CompositeDynamicObjectCollection.prototype.getCollection = function(index) { //>>includeStart('debug', pragmas.debug); @@ -472,10 +465,6 @@ define(['../Core/createGuid', recomposite(this); }; - CompositeDynamicObjectCollection.prototype.recomposite = function() { - recomposite(this); - }; - /** * Prevents {@link DynamicObjectCollection#collectionChanged} events from being raised * until a corresponding call is made to {@link DynamicObjectCollection#resumeEvents}, at which From 68a9169c769645dcc55a85000ad47306978fcac3 Mon Sep 17 00:00:00 2001 From: Mike Macaulay Date: Mon, 3 Mar 2014 13:04:59 -0600 Subject: [PATCH 5/8] Iterate over _collections vs _collectionsCopy --- .../CompositeDynamicObjectCollection.js | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/Source/DynamicScene/CompositeDynamicObjectCollection.js b/Source/DynamicScene/CompositeDynamicObjectCollection.js index 52fe2ed0cf8b..a0fc934cf37a 100644 --- a/Source/DynamicScene/CompositeDynamicObjectCollection.js +++ b/Source/DynamicScene/CompositeDynamicObjectCollection.js @@ -1,19 +1,19 @@ /*global define*/ define(['../Core/createGuid', - '../Core/defined', - '../Core/defineProperties', - '../Core/DeveloperError', - '../Core/Math', - './DynamicObject', - './DynamicObjectCollection' - ], function( - createGuid, - defined, - defineProperties, - DeveloperError, - CesiumMath, - DynamicObject, - DynamicObjectCollection) { + '../Core/defined', + '../Core/defineProperties', + '../Core/DeveloperError', + '../Core/Math', + './DynamicObject', + './DynamicObjectCollection' +], function( + createGuid, + defined, + defineProperties, + DeveloperError, + CesiumMath, + DynamicObject, + DynamicObjectCollection) { "use strict"; var propertyIdScratch = new Array(3); @@ -33,7 +33,7 @@ define(['../Core/createGuid', var composite = that._composite; var compositeObject = composite.getById(id); var compositeProperty = compositeObject[propertyName]; - var collections = that._collectionsCopy; + var collections = that._collections; var collectionsLength = collections.length; for ( var q = collectionsLength - 1; q >= 0; q--) { var object = collections[q].getById(dynamicObject.id); @@ -63,7 +63,7 @@ define(['../Core/createGuid', unsubscribeFromProperty(eventHash, collectionId, dynamicObject, propertyName); subscribeToProperty(that, eventHash, collectionId, dynamicObject, propertyName, dynamicObject[propertyName]); - var collections = that._collectionsCopy; + var collections = that._collections; var collectionsLength = collections.length; var firstTime = true; for ( var q = collectionsLength - 1; q >= 0; q--) { @@ -290,7 +290,7 @@ define(['../Core/createGuid', this._collections.splice(index, 0, collection); } - recomposite(this); + recomposite(this); }; /** From 41b59dd06d3c22e0a9551e942af1a5d45c67d087 Mon Sep 17 00:00:00 2001 From: Mike Macaulay Date: Mon, 3 Mar 2014 13:33:42 -0600 Subject: [PATCH 6/8] fixed formatting --- .../CompositeDynamicObjectCollection.js | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/Source/DynamicScene/CompositeDynamicObjectCollection.js b/Source/DynamicScene/CompositeDynamicObjectCollection.js index 5e209d76932a..a7fcdc595f1b 100644 --- a/Source/DynamicScene/CompositeDynamicObjectCollection.js +++ b/Source/DynamicScene/CompositeDynamicObjectCollection.js @@ -1,19 +1,19 @@ /*global define*/ define(['../Core/createGuid', - '../Core/defined', - '../Core/defineProperties', - '../Core/DeveloperError', - '../Core/Math', - './DynamicObject', - './DynamicObjectCollection' + '../Core/defined', + '../Core/defineProperties', + '../Core/DeveloperError', + '../Core/Math', + './DynamicObject', + './DynamicObjectCollection' ], function( - createGuid, - defined, - defineProperties, - DeveloperError, - CesiumMath, - DynamicObject, - DynamicObjectCollection) { + createGuid, + defined, + defineProperties, + DeveloperError, + CesiumMath, + DynamicObject, + DynamicObjectCollection) { "use strict"; var propertyIdScratch = new Array(3); @@ -530,7 +530,7 @@ define(['../Core/createGuid', * @memberof CompositeDynamicObjectCollection * * @param {Object} id The id of the object to retrieve. - * @returns {DynamicObject} The object with the provided id or undefined if the id did not exist in the collection. + * @returns {DynamicObject} The object with the provided id or undefined if the id did not exist in the collection is required. */ CompositeDynamicObjectCollection.prototype.getById = function(id) { return this._composite.getById(id); From b08c7cdacfca898a31845838a5d18f9d003bb3d7 Mon Sep 17 00:00:00 2001 From: Mike Macaulay Date: Mon, 3 Mar 2014 13:34:10 -0600 Subject: [PATCH 7/8] fixed formatting -again --- Source/DynamicScene/CompositeDynamicObjectCollection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/DynamicScene/CompositeDynamicObjectCollection.js b/Source/DynamicScene/CompositeDynamicObjectCollection.js index a7fcdc595f1b..4adc369dbc6f 100644 --- a/Source/DynamicScene/CompositeDynamicObjectCollection.js +++ b/Source/DynamicScene/CompositeDynamicObjectCollection.js @@ -6,7 +6,7 @@ define(['../Core/createGuid', '../Core/Math', './DynamicObject', './DynamicObjectCollection' -], function( + ], function( createGuid, defined, defineProperties, From 0bfd0f275b3cb99e8be05f6f2db2ddc1602b4ea2 Mon Sep 17 00:00:00 2001 From: Mike Macaulay Date: Mon, 3 Mar 2014 13:35:16 -0600 Subject: [PATCH 8/8] removed extra comment --- Source/DynamicScene/CompositeDynamicObjectCollection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/DynamicScene/CompositeDynamicObjectCollection.js b/Source/DynamicScene/CompositeDynamicObjectCollection.js index 4adc369dbc6f..fcdd33f8bfef 100644 --- a/Source/DynamicScene/CompositeDynamicObjectCollection.js +++ b/Source/DynamicScene/CompositeDynamicObjectCollection.js @@ -530,7 +530,7 @@ define(['../Core/createGuid', * @memberof CompositeDynamicObjectCollection * * @param {Object} id The id of the object to retrieve. - * @returns {DynamicObject} The object with the provided id or undefined if the id did not exist in the collection is required. + * @returns {DynamicObject} The object with the provided id or undefined if the id did not exist in the collection. */ CompositeDynamicObjectCollection.prototype.getById = function(id) { return this._composite.getById(id);