diff --git a/CHANGES.md b/CHANGES.md index 3c548aabda30..b36df8b3ac74 100644 --- 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 index 332565e32b4f..adf626c6f6a4 100644 --- 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 index 9d6a3565955a..fe395326c663 100644 --- 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,21 +68,21 @@ 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); @@ -90,15 +90,15 @@ describe('DataSources/ReferenceProperty', function() { 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(); }); });