Change ReferenceProperty to return undefined when asked for values and the target entity (or target property) does not exist.

shunter · shunter · commit 111f83e79175 · 2020-01-15T17:11:12.000-05:00
This differs from the previous behavior, which was to throw. Throwing can lead to situations where it is effectively impossible to delete and re-create a property which is referenced.

In general, visualizers have always needed to be written to expect the values to properties to suddenly become undefined, which they generally deal with by hiding or removing the related primitives.
diff --git a/CHANGES.md b/CHANGES.md
@@ -25,6 +25,7 @@ Change Log
 * Reduced Cesium bundle size by avoiding unnecessarily importing `Cesium3DTileset` in `Picking.js` [#8532](https://github.com/AnalyticalGraphicsInc/cesium/pull/8532)
 * Fixed WebGL warning message about `EXT_float_blend` being implicitly enabled. [#8534](https://github.com/AnalyticalGraphicsInc/cesium/pull/8534)
 * 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 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)
 
 ### 1.65.0 - 2020-01-06
 
diff --git a/Source/DataSources/ReferenceProperty.js b/Source/DataSources/ReferenceProperty.js
@@ -2,50 +2,37 @@ import defined from '../Core/defined.js';
 import defineProperties from '../Core/defineProperties.js';
 import DeveloperError from '../Core/DeveloperError.js';
 import Event from '../Core/Event.js';
-import RuntimeError from '../Core/RuntimeError.js';
 import Property from './Property.js';
 
-    function resolveEntity(that) {
-        var entityIsResolved = true;
-        if (that._resolveEntity) {
-            var targetEntity = that._targetCollection.getById(that._targetId);
+    function resolve(that) {
+        var targetProperty = that._targetProperty;
 
-            if (defined(targetEntity)) {
-                targetEntity.definitionChanged.addEventListener(ReferenceProperty.prototype._onTargetEntityDefinitionChanged, that);
-                that._targetEntity = targetEntity;
-                that._resolveEntity = false;
-            } else {
-                //The property has become detached.  It has a valid value but is not currently resolved to an entity in the collection
-                targetEntity = that._targetEntity;
-                entityIsResolved = false;
-            }
+        if (!defined(targetProperty)) {
+            var targetEntity = that._targetEntity;
 
             if (!defined(targetEntity)) {
-                throw new RuntimeError('target entity "' + that._targetId + '" could not be resolved.');
-            }
-        }
-        return entityIsResolved;
-    }
+                targetEntity = that._targetCollection.getById(that._targetId);
 
-    function resolve(that) {
-        var targetProperty = that._targetProperty;
+                if (!defined(targetEntity)) {
+                    // target entity not found
+                    that._targetEntity = that._targetProperty = undefined;
+                    return;
+                }
 
-        if (that._resolveProperty) {
-            var entityIsResolved = resolveEntity(that);
+                // target entity was found. listen for changes to entity definition
+                targetEntity.definitionChanged.addEventListener(ReferenceProperty.prototype._onTargetEntityDefinitionChanged, that);
+                that._targetEntity = targetEntity;
+            }
 
-            var names = that._targetPropertyNames;
+            // walk the list of property names and resolve properties
+            var targetPropertyNames = that._targetPropertyNames;
             targetProperty = that._targetEntity;
-            var length = names.length;
-            for (var i = 0; i < length && defined(targetProperty); i++) {
-                targetProperty = targetProperty[names[i]];
+            for (var i = 0, len = targetPropertyNames.length; i < len && defined(targetProperty); ++i) {
+                targetProperty = targetProperty[targetPropertyNames[i]];
             }
 
-            if (defined(targetProperty)) {
-                that._targetProperty = targetProperty;
-                that._resolveProperty = !entityIsResolved;
-            } else if (!defined(that._targetProperty)) {
-                throw new RuntimeError('targetProperty "' + that._targetId + '.' + names.join('.') + '" could not be resolved.');
-            }
+            // target property may or may not be defined, depending on if it was found
+            that._targetProperty = targetProperty;
         }
 
         return targetProperty;
@@ -118,8 +105,6 @@ import Property from './Property.js';
         this._targetProperty = undefined;
         this._targetEntity = undefined;
         this._definitionChanged = new Event();
-        this._resolveEntity = true;
-        this._resolveProperty = true;
 
         targetCollection.collectionChanged.addEventListener(ReferenceProperty.prototype._onCollectionChanged, this);
     }
@@ -157,7 +142,8 @@ import Property from './Property.js';
          */
         referenceFrame : {
             get : function() {
-                return resolve(this).referenceFrame;
+                var target = resolve(this);
+                return defined(target) ? target.referenceFrame : undefined;
             }
         },
         /**
@@ -267,7 +253,8 @@ import Property from './Property.js';
      * @returns {Object} The modified result parameter or a new instance if the result parameter was not supplied.
      */
     ReferenceProperty.prototype.getValue = function(time, result) {
-        return resolve(this).getValue(time, result);
+        var target = resolve(this);
+        return defined(target) ? target.getValue(time, result) : undefined;
     };
 
     /**
@@ -280,7 +267,8 @@ import Property from './Property.js';
      * @returns {Cartesian3} The modified result parameter or a new instance if the result parameter was not supplied.
      */
     ReferenceProperty.prototype.getValueInReferenceFrame = function(time, referenceFrame, result) {
-        return resolve(this).getValueInReferenceFrame(time, referenceFrame, result);
+        var target = resolve(this);
+        return defined(target) ? target.getValueInReferenceFrame(time, referenceFrame, result) : undefined;
     };
 
     /**
@@ -291,7 +279,8 @@ import Property from './Property.js';
      * @returns {String} The type of material.
      */
     ReferenceProperty.prototype.getType = function(time) {
-        return resolve(this).getType(time);
+        var target = resolve(this);
+        return defined(target) ? target.getType(time) : undefined;
     };
 
     /**
@@ -326,27 +315,21 @@ import Property from './Property.js';
     };
 
     ReferenceProperty.prototype._onTargetEntityDefinitionChanged = function(targetEntity, name, value, oldValue) {
-        if (this._targetPropertyNames[0] === name) {
-            this._resolveProperty = true;
+        if (defined(this._targetProperty) && this._targetPropertyNames[0] === name) {
+            this._targetProperty = undefined;
             this._definitionChanged.raiseEvent(this);
         }
     };
 
     ReferenceProperty.prototype._onCollectionChanged = function(collection, added, removed) {
         var targetEntity = this._targetEntity;
-        if (defined(targetEntity)) {
-            if (removed.indexOf(targetEntity) !== -1) {
-                targetEntity.definitionChanged.removeEventListener(ReferenceProperty.prototype._onTargetEntityDefinitionChanged, this);
-                this._resolveEntity = true;
-                this._resolveProperty = true;
-            } else if (this._resolveEntity) {
-                //If targetEntity is defined but resolveEntity is true, then the entity is detached
-                //and any change to the collection needs to incur an attempt to resolve in order to re-attach.
-                //without this if block, a reference that becomes re-attached will not signal definitionChanged
-                resolve(this);
-                if (!this._resolveEntity) {
-                    this._definitionChanged.raiseEvent(this);
-                }
+        if (defined(targetEntity) && removed.indexOf(targetEntity) !== -1) {
+            targetEntity.definitionChanged.removeEventListener(ReferenceProperty.prototype._onTargetEntityDefinitionChanged, this);
+            this._targetEntity = this._targetProperty = undefined;
+        } else if (!defined(targetEntity)) {
+            targetEntity = resolve(this);
+            if (defined(targetEntity)) {
+                this._definitionChanged.raiseEvent(this);
             }
         }
     };
diff --git a/Specs/DataSources/ReferencePropertySpec.js b/Specs/DataSources/ReferencePropertySpec.js
@@ -58,7 +58,7 @@ describe('DataSources/ReferenceProperty', function() {
         var collection = new EntityCollection();
         collection.add(testObject);
 
-        //Basic property resolution
+        // Basic property resolution
         var property = ReferenceProperty.fromString(collection, 'testId#billboard.scale');
         expect(property.referenceFrame).toBeUndefined();
         expect(property.isConstant).toEqual(true);
@@ -68,37 +68,37 @@ describe('DataSources/ReferenceProperty', function() {
         var listener = jasmine.createSpy('listener');
         property.definitionChanged.addEventListener(listener);
 
-        //Change to exist target property is reflected in reference.
+        // A change to exist target property is reflected in reference.
         testObject.billboard.scale.setValue(6);
         expect(listener).toHaveBeenCalledWith(property);
         expect(property.isConstant).toEqual(true);
         expect(property.getValue(time)).toEqual(6);
         listener.calls.reset();
 
-        //Assignment of new leaf property to existing target is reflected in reference.
+        // Assignment of new leaf property to existing target is reflected in reference.
         testObject.billboard.scale = new ConstantProperty(7);
         expect(listener).toHaveBeenCalledWith(property);
         expect(property.isConstant).toEqual(true);
         expect(property.getValue(time)).toEqual(7);
         listener.calls.reset();
 
-        //Assignment of non-leaf property to existing target is reflected in reference.
+        // Assignment of non-leaf property to existing target is reflected in reference.
         testObject.billboard = new BillboardGraphics();
         testObject.billboard.scale = new ConstantProperty(8);
         expect(listener).toHaveBeenCalledWith(property);
         expect(property.isConstant).toEqual(true);
         expect(property.getValue(time)).toEqual(8);
         listener.calls.reset();
 
-        //Removing an object should cause the reference to be severed but maintain last value
+        // Removing an object should cause the reference to be severed.
         collection.remove(testObject);
 
-        expect(listener).not.toHaveBeenCalledWith();
+        expect(listener).not.toHaveBeenCalled();
         expect(property.isConstant).toEqual(true);
-        expect(property.getValue(time)).toEqual(8);
+        expect(property.getValue(time)).toBeUndefined();
         listener.calls.reset();
 
-        //adding a new object should re-wire the reference.
+        // Adding a new object should re-wire the reference.
         var testObject2 = new Entity({
             id : 'testId'
         });
@@ -108,6 +108,18 @@ describe('DataSources/ReferenceProperty', function() {
         expect(listener).toHaveBeenCalledWith(property);
         expect(property.isConstant).toEqual(true);
         expect(property.getValue(time)).toEqual(9);
+
+        // setting the target property to undefined should cause the reference to be severed.
+        testObject2.billboard.scale = undefined;
+        expect(listener).toHaveBeenCalledWith(property);
+        expect(property.isConstant).toEqual(true);
+        expect(property.getValue(time)).toBeUndefined();
+
+        // Assigning a valid property should re-connect the reference.
+        testObject2.billboard.scale = new ConstantProperty(10);
+        expect(listener).toHaveBeenCalledWith(property);
+        expect(property.isConstant).toEqual(true);
+        expect(property.getValue(time)).toEqual(10);
     });
 
     it('works with position properties', function() {
@@ -119,12 +131,18 @@ describe('DataSources/ReferenceProperty', function() {
         var collection = new EntityCollection();
         collection.add(testObject);
 
-        //Basic property resolution
+        // Basic property resolution
         var property = ReferenceProperty.fromString(collection, 'testId#position');
         expect(property.isConstant).toEqual(true);
         expect(property.referenceFrame).toEqual(ReferenceFrame.FIXED);
         expect(property.getValue(time)).toEqual(testObject.position.getValue(time));
         expect(property.getValueInReferenceFrame(time, ReferenceFrame.INERTIAL)).toEqual(testObject.position.getValueInReferenceFrame(time, ReferenceFrame.INERTIAL));
+
+        property = ReferenceProperty.fromString(collection, 'nonExistent#position');
+        expect(property.isConstant).toEqual(true);
+        expect(property.referenceFrame).toBeUndefined();
+        expect(property.getValue(time)).toBeUndefined();
+        expect(property.getValueInReferenceFrame(time, ReferenceFrame.INERTIAL)).toBeUndefined();
     });
 
     it('works with material properties', function() {
@@ -137,11 +155,17 @@ describe('DataSources/ReferenceProperty', function() {
         var collection = new EntityCollection();
         collection.add(testObject);
 
-        //Basic property resolution
+        // Basic property resolution
         var property = ReferenceProperty.fromString(collection, 'testId#testMaterial');
         expect(property.isConstant).toEqual(true);
         expect(property.getType(time)).toEqual(testObject.testMaterial.getType(time));
         expect(property.getValue(time)).toEqual(testObject.testMaterial.getValue(time));
+
+        property = ReferenceProperty.fromString(collection, 'nonExistent#testMaterial');
+        expect(property.isConstant).toEqual(true);
+        expect(property.referenceFrame).toBeUndefined();
+        expect(property.getType(time)).toBeUndefined();
+        expect(property.getValue(time)).toBeUndefined();
     });
 
     it('equals works', function() {
@@ -151,19 +175,19 @@ describe('DataSources/ReferenceProperty', function() {
         var right = ReferenceProperty.fromString(entityCollection, 'objectId#foo.bar');
         expect(left.equals(right)).toEqual(true);
 
-        //collection differs
+        // collection differs
         right = ReferenceProperty.fromString(new EntityCollection(), 'objectId#foo.bar');
         expect(left.equals(right)).toEqual(false);
 
-        //target id differs
+        // target id differs
         right = ReferenceProperty.fromString(entityCollection, 'otherObjectId#foo.bar');
         expect(left.equals(right)).toEqual(false);
 
-        //number of sub-properties differ
+        // number of sub-properties differ
         right = ReferenceProperty.fromString(entityCollection, 'objectId#foo');
         expect(left.equals(right)).toEqual(false);
 
-        //sub-properties of same length differ
+        // sub-properties of same length differ
         right = ReferenceProperty.fromString(entityCollection, 'objectId#foo.baz');
         expect(left.equals(right)).toEqual(false);
     });
@@ -195,6 +219,34 @@ describe('DataSources/ReferenceProperty', function() {
         expect(listener).not.toHaveBeenCalled();
     });
 
+    it('attaches to a target entity created later', function() {
+        var collection = new EntityCollection();
+
+        var property = ReferenceProperty.fromString(collection, 'testId#billboard.scale');
+        expect(property.resolvedProperty).toBeUndefined();
+
+        var listener = jasmine.createSpy('listener');
+        property.definitionChanged.addEventListener(listener);
+
+        var otherObject = new Entity({
+            id : 'other'
+        });
+        collection.add(otherObject);
+
+        expect(listener).not.toHaveBeenCalled();
+        expect(property.resolvedProperty).toBeUndefined();
+
+        var testObject = new Entity({
+            id : 'testId'
+        });
+        testObject.billboard = new BillboardGraphics();
+        testObject.billboard.scale = new ConstantProperty(5);
+        collection.add(testObject);
+
+        expect(listener).toHaveBeenCalledWith(property);
+        expect(property.resolvedProperty).toBe(testObject.billboard.scale);
+    });
+
     it('constructor throws with undefined targetCollection', function() {
         expect(function() {
             return new ReferenceProperty(undefined, 'objectid', ['property']);
@@ -251,15 +303,13 @@ describe('DataSources/ReferenceProperty', function() {
         }).toThrowDeveloperError();
     });
 
-    it('throws RuntimeError if targetId can not be resolved', function() {
+    it('getValue returns undefined if target entity can not be resolved', function() {
         var collection = new EntityCollection();
         var property = ReferenceProperty.fromString(collection, 'testId#foo.bar');
-        expect(function() {
-            property.getValue(time);
-        }).toThrowRuntimeError();
+        expect(property.getValue(time)).toBeUndefined();
     });
 
-    it('throws RuntimeError if property can not be resolved', function() {
+    it('getValue returns undefined if target property can not be resolved', function() {
         var collection = new EntityCollection();
 
         var testObject = new Entity({
@@ -268,12 +318,10 @@ describe('DataSources/ReferenceProperty', function() {
         collection.add(testObject);
 
         var property = ReferenceProperty.fromString(collection, 'testId#billboard');
-        expect(function() {
-            property.getValue(time);
-        }).toThrowRuntimeError();
+        expect(property.getValue(time)).toBeUndefined();
     });
 
-    it('throws RuntimeError if sub-property can not be resolved', function() {
+    it('getValue returns undefined if sub-property of target property can not be resolved', function() {
         var collection = new EntityCollection();
 
         var testObject = new Entity({
@@ -283,8 +331,6 @@ describe('DataSources/ReferenceProperty', function() {
         collection.add(testObject);
 
         var property = ReferenceProperty.fromString(collection, 'testId#billboard.foo');
-        expect(function() {
-            property.getValue(time);
-        }).toThrowRuntimeError();
+        expect(property.getValue(time)).toBeUndefined();
     });
 });
