Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change ReferenceProperty to return undefined instead of throw #8544

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Change ReferenceProperty to return undefined instead of throw #8544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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();
});
});