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.

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

add pickObject API in layer #2798

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@jianhuang01
Copy link
Contributor

jianhuang01 commented Mar 14, 2019

For #2775

Change List

  • add deck instance to context
  • add pickObject in layer
  • update doc
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 14, 2019

Coverage Status

Coverage decreased (-0.1%) to 58.4% when pulling 8d3a2ce on fix-picker-api into eae5ed0 on master.

// Return the closest pickable and visible object at the given screen coordinate.
pickObject({x, y, radius = 0}) {
if (this.context.deck) {
return this.context.deck.pickObject({x, y, radius, layerIds: [this.props.id]});

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 15, 2019

Contributor

I want to be able to specify layerIds in case of composite layer.

Also want the multi object method.

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 15, 2019

Contributor

I want to be able to specify layerIds in case of composite layer.

I think you get them all. As I recall this matches all layers with that prefix, which per convention includes all sub layers of composite layers (their ids are appended to the parent). Excluding some of them is not possible with this mechanism though.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 15, 2019

Contributor

But I might not want all of them.

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 15, 2019

Contributor

Good point. Ideally you should be able to inspect the pick infos after picking is done, but I am not 100% sure we save the id of the origin layer.

But seems weird to open up layerIds in Layer which would allow picking other layers.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 15, 2019

Contributor

Weird yes, but I don't think there are any security issues.

This comment has been minimized.

Copy link
@jianhuang01

jianhuang01 Mar 15, 2019

Author Contributor

The deck instance is still there, you can use this.context.deck.pickObject() as the old way this.context.layerManager.pickObject() if there is special requirement.

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 15, 2019

Contributor

Alternatively we can accept a list of sublayer ids (i.e. layer ids minus the parent layer id, for example http://deck.gl/#/documentation/deckgl-api-reference/layers/geojson-layer?section=sub-layers)

This comment has been minimized.

Copy link
@jianhuang01

jianhuang01 Mar 15, 2019

Author Contributor

Alternatively we can accept a list of sublayer ids (i.e. layer ids minus the parent layer id, for example http://deck.gl/#/documentation/deckgl-api-reference/layers/geojson-layer?section=sub-layers)

we can add another pickObject API in composite layer class with extra subLayerIds prop

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 15, 2019

Contributor

Just another 2 cents:

If the worry is that this.context.deck.pickObject() is too ugly/low level, how about just adding an accessor for this.context.deck?

class Layer {
  get deck() { return this.context.deck; }
  onClick() {
    this.deck.pickObject(...);
  }
}

This way, there are no new methods to document, test, and maintain.

Then we make any enhancements to layer filtering in the global deck method so as to benefit all use cases.

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 15, 2019

Contributor

Just another 2 cents:

If the worry is that this.context.deck.pickObject() is too ugly/low level, how about just adding an accessor for this.context.deck?

class Layer {
  get deck() { return this.context.deck; }
  onClick() {
    this.deck.pickObject(...);
  }
}

This way, there are no new methods to document, test, and maintain.

Then we make any enhancements to layer filtering in the global deck method so as to benefit all use cases.

Agree 100% 💯

@@ -288,6 +288,14 @@ export default class Layer extends Component {
return index;
}

// Return the closest pickable and visible object at the given screen coordinate.
pickObject({x, y, radius = 0}) {

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 15, 2019

Contributor
Suggested change
pickObject({x, y, radius = 0}) {
pickObject(opts) {
// Return the closest pickable and visible object at the given screen coordinate.
pickObject({x, y, radius = 0}) {
if (this.context.deck) {
return this.context.deck.pickObject({x, y, radius, layerIds: [this.props.id]});

This comment has been minimized.

Copy link
@georgios-uber

georgios-uber Mar 15, 2019

Contributor
Suggested change
return this.context.deck.pickObject({x, y, radius, layerIds: [this.props.id]});
return this.context.deck.pickObject(Object.assign({}, opts, {radius: 0, layerIds: [this.props.id]});

@jianhuang01 jianhuang01 deleted the fix-picker-api branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.