Skip to content

Feature/selectable entity #3176

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

Closed
wants to merge 5 commits into from
Closed

Feature/selectable entity #3176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f1d207e
Adds the 'selectable' member to the Entity class
bradAnderson58 Nov 6, 2015
cf55ad5
Fixed Typos
bradAnderson58 Nov 6, 2015
eb5ecd5
jsHint fix
bradAnderson58 Nov 6, 2015
aa54270
Added Unit Tests for Selectable Entities
bradAnderson58 Dec 4, 2015
e3fa7b0
jsHint fix
bradAnderson58 Dec 4, 2015
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
5 changes: 5 additions & 0 deletions Source/DataSources/CzmlDataSource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,10 @@ define([
function processName(entity, packet, entityCollection, sourceUri) {
entity.name = defaultValue(packet.name, entity.name);
}

function processSelectable(entity, packet, entityCollection, sourceUri) {
entity.selectable = defaultValue(packet.selectable, true);
}

function processDescription(entity, packet, entityCollection, sourceUri) {
var descriptionData = packet.description;
Expand Down Expand Up @@ -1633,6 +1637,7 @@ define([
processLabel, //
processModel, //
processName, //
processSelectable,
processDescription, //
processPath, //
processPoint, //
Expand Down
24 changes: 24 additions & 0 deletions Source/DataSources/Entity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ define([
* @param {String} [options.name] A human readable name to display to users. It does not have to be unique.
* @param {TimeIntervalCollection} [options.availability] The availability, if any, associated with this object.
* @param {Boolean} [options.show] A boolean value indicating if the entity and its children are displayed.
* @param {Boolean} [options.selectable] A boolean value indicating whether the entity is selectable. Default true.
* @param {Property} [options.description] A string Property specifying an HTML description for this entity.
* @param {PositionProperty} [options.position] A Property specifying the entity position.
* @param {Property} [options.orientation] A Property specifying the entity orientation.
Expand Down Expand Up @@ -128,6 +129,7 @@ define([
this._definitionChanged = new Event();
this._name = options.name;
this._show = defaultValue(options.show, true);
this._selectable = defaultValue(options.selectable, true);
this._parent = undefined;
this._propertyNames = ['billboard', 'box', 'corridor', 'cylinder', 'description', 'ellipse', //
'ellipsoid', 'label', 'model', 'orientation', 'path', 'point', 'polygon', //
Expand Down Expand Up @@ -269,6 +271,28 @@ define([
this._definitionChanged.raiseEvent(this, 'show', value, !value);
}
},

/**
* Gets or sets whether this entity is selectabled or not
* @memberof Entity.prototype
* @type {Boolean}
*/
selectable: {
get: function() {
return this._selectable;
},
set: function(value) {
if (!defined(value)) {
throw new DeveloperError('value is required');
}

if (value === this._selectable) {
return;
}
this._selectable = value;
}
},

/**
* Gets whether this entity is being displayed, taking into account
* the visibility of any ancestor entities.
Expand Down
3 changes: 3 additions & 0 deletions Source/Renderer/PickFramebuffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ define([

var object = context.getObjectByPickColor(colorScratch);
if (defined(object)) {
if (object.id && (typeof object.id === 'object')) {
return (object.id.selectable) ? object : undefined;
Copy link
Contributor

@pjcozzi pjcozzi Dec 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mramato is there a better place to put this higher in the stack?

Copy link
Contributor

@mramato mramato Dec 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradAnderson58 the problem here is that id is not always guaranteed to be an Entity instance, so we can't rely on this check here.

We don't have a top-level Entity picking API right now, so this would have to be done in user code. We want to add one though, DataSourceDisplay would probably be a good place for that.

I think the bigger problem is that this can't be implemented efficiently without such functionality at the primitive level. We basically would have to have every primitive have the selectable property, then keep a similar check here. However, I don't think selectable is the right approach for all use cases. I would rather see a pickPriority property as discussed in #1592. It would be a number where <=0 means do not pick and higher numbers means "pick this first". This has drill pick ramifications but shouldn't be too hard optimize for most use cases. If we added this feature at the primitive level, we would then have to thread it through up to the Entity API and visualizers.

If @pjcozzi is on board with changing the Primitive API and you are interested in working on this, I can write up some more details instructions about what I think has to happen.

Also, sorry for taking so long to look at this PR.

Copy link
Contributor

@pjcozzi pjcozzi Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am for the pickPriority approach.

Copy link
Author

@bradAnderson58 bradAnderson58 Jan 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey guys, sorry I took so long to respond to this, the email just got lost in the sauce around the holidays. I would definitely be interested in working on this. I agree that the pickPriority option is preferrable to the selectable. I think I was in a bit of an application-specific mindset there.
I will dig around in the DataSourceDisplay and such to get a better idea of whats going on there while I await instructions.
Again, sorry for taking so long to respond.

Copy link
Contributor

@pjcozzi pjcozzi Jun 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradAnderson58 did you get a chance to look at this?

}
return object;
}
}
Expand Down
14 changes: 14 additions & 0 deletions Specs/DataSources/CzmlDataSourceSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2091,4 +2091,18 @@ defineSuite([
expect(error.message).toContain('CZML version information invalid. It is expected to be a property on the document object in the <Major>.<Minor> version format.');
});
});

it('sets selectable according to the CZML field', function() {
var packet = {
position : {
cartesian : [1.0, 2.0, 3.0]
},
selectable : false
};

var dataSource = new CzmlDataSource();
dataSource.load(makePacket(packet));
var entity = dataSource.entities.values[0];
expect(entity.selectable).toBe(false);
});
});
3 changes: 3 additions & 0 deletions Specs/DataSources/EntitySpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,13 @@ defineSuite([
expect(entity.viewFrom).toBeUndefined();
expect(entity.wall).toBeUndefined();
expect(entity.entityCollection).toBeUndefined();
expect(entity.selectable).toBe(true);

var options = {
id : 'someId',
name : 'bob',
show : false,
selectable: false,
availability : new TimeIntervalCollection(),
parent : new Entity(),
customProperty : {},
Expand Down Expand Up @@ -113,6 +115,7 @@ defineSuite([
expect(entity.id).toEqual(options.id);
expect(entity.name).toEqual(options.name);
expect(entity.show).toBe(options.show);
expect(entity.selectable).toBe(options.selectable);
expect(entity.availability).toBe(options.availability);
expect(entity.parent).toBe(options.parent);
expect(entity.customProperty).toBe(options.customProperty);
Expand Down
20 changes: 20 additions & 0 deletions Specs/Widgets/Viewer/ViewerSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@
defineSuite([
'Widgets/Viewer/Viewer',
'Core/Cartesian3',
'Core/Cartesian2',
'Core/ClockRange',
'Core/ClockStep',
'Core/EllipsoidTerrainProvider',
'Core/JulianDate',
'Core/Matrix4',
'Core/WebMercatorProjection',
'Core/Rectangle',
'DataSources/ConstantPositionProperty',
'DataSources/ConstantProperty',
'DataSources/DataSourceClock',
Expand Down Expand Up @@ -36,12 +38,14 @@ defineSuite([
], function(
Viewer,
Cartesian3,
Cartesian2,
ClockRange,
ClockStep,
EllipsoidTerrainProvider,
JulianDate,
Matrix4,
WebMercatorProjection,
Rectangle,
ConstantPositionProperty,
ConstantProperty,
DataSourceClock,
Expand Down Expand Up @@ -899,6 +903,22 @@ defineSuite([
});
});
});

it('cannot select an entity when selectable is false', function() {
var viewer = createViewer(container);

viewer.entities.add({
rectangle: {
coordinates: Rectangle.fromDegrees(-50.0, -50.0, 50.0, 50.0)
},
selectable: false
});

var pickedObject = viewer.scene.pick(new Cartesian2(0, 0));
expect(pickedObject).toBeUndefined();

viewer.destroy();
});

it('does not crash when tracking an object with a position property whose value is undefined.', function() {
viewer = createViewer(container);
Expand Down