Skip to content

Commit

Permalink
Merge pull request #8544 from AnalyticalGraphicsInc/referenceProperty…
Browse files Browse the repository at this point in the history 
…ReturnUndefined

Change ReferenceProperty to return undefined instead of throw
  • Loading branch information
@mramato
mramato authored Jan 16, 2020
2 parents 0a45aef + 111f83e commit 619ab84
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 80 deletions.
1 change: 1 addition & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
91 changes: 37 additions & 54 deletions Source/DataSources/ReferenceProperty.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
}
},
/**
Expand Down Expand Up @@ -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;
};

/**
Expand All @@ -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;
};

/**
Expand All @@ -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;
};

/**
Expand Down Expand Up @@ -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);
}
}
};
Expand Down
98 changes: 72 additions & 26 deletions Specs/DataSources/ReferencePropertySpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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'
});
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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);
});
Expand Down Expand Up @@ -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']);
Expand Down Expand Up @@ -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({
Expand All @@ -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({
Expand All @@ -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();
});
});

0 comments on commit 619ab84

Please sign in to comment.