Add `pickingRadius` prop #641

Pessimistress · 2017-05-12T23:58:50Z

Add pickingRadius prop to DeckGL component. The old picking behavior is equivalent to pickingRadius: 0. The closest object to the pointer within pickingRadius pixels is picked.

howtimeflies0 · 2017-05-13T00:11:30Z

Do we need to rerender the picking buffer on every mouse event and API call? Shall we add a needRedraw flag for the picking buffer?

We should not rerender picking buffer on every mouse event.

Pessimistress · 2017-05-13T00:16:30Z

We should not rerender picking buffer on every mouse event

We currently do so for hover and click. Hover does update a uniform selectedPickingColor, which will cause a redraw (though not making any difference for picking in any of the core layers).

howtimeflies0 · 2017-05-13T00:18:57Z

src/lib/draw-and-pick.js

+    pickedColor = pickResult.color;
+    pickedLayer = pickResult.layer;
+    pickedObjectIndex = pickResult.objectIndex;
+    affectedLayers = pickedLayer ? [pickedLayer] : [];


any reason we move this logical out of pickFromBuffer() function now?

The only logic that has been moved out is affectedLayers. pickFromBuffer() and pickFromBufferByBoundingBox() now return similarly shaped objects.

Yep, I already moved this out in the drag events PR because we don't always pick -- drag events that indicate the continuation of a drag event that started on a specific element don't need to re-pick. In that case, the user is assumed to be dragging an element around and so the element picked on dragstart is returned without picking.

howtimeflies0 · 2017-05-13T00:22:33Z

src/lib/layer-manager.js

@@ -173,6 +170,26 @@ export default class LayerManager {
    });
  }

+  pickLayerByBoundingBox({x, y, width, height}) {


I assume this function serves as an extension of the pickLayer() function right above it and it will replace pickLayer(), right?

No. pickLayer() is used by mouse event picking (returns a single info object) and pickLayerByBoundingBox() is used by the query API (returns an array of info objects).

Yeah, I knew that. pickLayer() and pickLayerByBoundingBox() share a lot of code, so do pickLayers() and pickLayersByBoundingBox() in draw-and-pick.js.

I'm wondering if mouse event picking can also be implemented via pickLayerByBoundingBox() with width and height both set to 1. We'll need to handle picking "mode" better.

This is the plan per our last conversation:

add queryRenderedFeatures-style functionality to layer-manager to allow applications to handle events themselves and run picking on an arbitrary point/bbox. deck.gl will (later) use this feature for event-based picking under the hood

ibgreen

This is overall a very nice improvement code wise and the radius prop should give some relief to picking with "fat fingers".

Main concern (in the RFC and in comments here) is on the bounding box selection @shaojingli will add to agenda for next decision meeting.

Ideally the PR would include

some test (at least on the pickingColor utils, but we should be able to do a test picking, exercising this important code in a test case would be really valuable)
a bench that tests picking perf on large areas (CPU traversal)

Those additions would really round out this PR. Regardless, before merging, we should have

general documentation
what's new
CHANGELOG
updates in place as well.

ibgreen · 2017-05-13T00:05:33Z

examples/layer-browser/src/app.js

@@ -102,6 +102,12 @@ class App extends PureComponent {
    setImmutableDataSamples(settings.immutable);
  }

+  _onQueryFeatures() {


Should we call it onQueryElements or onQueryObjects for consistency? We should review or documentation of the data prop and use the same nomenclature for the individual objects...

This isn't a handler, is it? I thought the intent here was to respond to an input event by calling queryElements manually from within the input event handler. In that case, should just be queryElements() {}, no 'on'.

We agreed this path would serve as a complement to the onClick / onHover callback-style interaction that we already have, to allow app devs more flexibility with how they handle input events.

Agreed with @ibgreen this should be elements.

ibgreen · 2017-05-13T00:07:02Z

examples/layer-browser/src/app.js

@@ -220,6 +227,10 @@ class App extends PureComponent {
        { this._renderMap() }
        { !MAPBOX_ACCESS_TOKEN && this._renderNoTokenWarning() }
        <div id="control-panel">
+          <div>
+            <h4 />
+            <button onClick={this._onQueryFeatures} >Query Rendered Features</button>


Features => Elements/Objects etc

ibgreen · 2017-05-13T00:09:37Z

src/lib/draw-and-pick.js

@@ -206,11 +195,122 @@ export function pickLayers(gl, {
 }
 /* eslint-enable max-depth, max-statements */

+export function pickLayersByBoundingBox(gl, {


See my concern in the RFC. This will not catch hidden objects which I think it expected in an area selection operation. But maybe I am missing the use case. So depending on what we decide, we should ask ourselves if it is worth including a possibly flawed picking function.

@ibgreen sorry, I'm having trouble finding this in the RFC. What does it mean for an element to be "hidden"? We should try to follow the DOM model for this -- if it's "turned off" / "hidden", it's not selectable. If its opacity is 0, it is selectable. I'm not clear on exactly how this maps to deck.gl (or even if it does), nor am I clear on how we'd handle the opacity === 0 case.

@ibgreen sorry, I'm having trouble finding this in the RFC.

As mentioned, this is discussed in the comments in the RFC. But I will explain again:

Let's say you have 2 objects. But you are so zoomed out that they all render on a single pixel. You select a generous bounding box around them, expected the list of selected objects to include both objects. But only one of them will detected by this picking approach.

Now repeat the thought experiment with more pixels and more objects. As long as there is a risk of one object overlapping with another, there is a risk that this picking method does not correctly identify all objects within the region, as it depends on a pixel representing that object actually being rendered into the framebuffer.

@ericsoco I believe @ibgreen means "occluded" in "hidden". So, if an element is completely occluded. Users won't expect to select it by pointing at it, but they will expect to select it by box selecting.

Right, I understand now. Maybe already covered in the RFC (reading now) but this would require us to maintain coordinates/bounds of all elements in model space...

ibgreen · 2017-05-13T00:12:42Z

src/lib/draw-and-pick.js

+
+  // Traverse all pixels in picking results and find the one closest to the supplied
+  // [deviceX, deviceY]
+  let minDistanceToCenter = deviceRadius;


Would be nice to have a bench that tested the perf on this on a relatively big area.

This was a concern of mine too...

ibgreen · 2017-05-13T00:13:32Z

src/lib/draw-and-pick.js

+      if (pickResult) {
+        const dx = deviceBox.x + col - deviceX;
+        const dy = deviceBox.y + row - deviceY;
+        const d = Math.sqrt(dx * dx + dy * dy);


Often possible and cheaper to not draw roots and just compare squares. E.g. calling the variable minSquaredDistance.

ibgreen · 2017-05-13T00:16:05Z

src/react/deckgl.js

@@ -146,7 +159,12 @@ export default class DeckGL extends React.Component {
      return;
    }
    const {event: {offsetX: x, offsetY: y}} = event;
-    const selectedInfos = this.layerManager.pickLayer({x, y, mode: 'click'});
+    const selectedInfos = this.layerManager.pickLayer({


Assign radius before the call and keep as one line?

ibgreen · 2017-05-13T00:16:11Z

src/react/deckgl.js

@@ -161,7 +179,12 @@ export default class DeckGL extends React.Component {
      return;
    }
    const {event: {offsetX: x, offsetY: y}} = event;
-    const selectedInfos = this.layerManager.pickLayer({x, y, mode: 'hover'});
+    const selectedInfos = this.layerManager.pickLayer({


ibgreen · 2017-05-13T00:16:19Z

src/react/deckgl.js

@@ -201,7 +224,12 @@ export default class DeckGL extends React.Component {
    }

    if (mode) {
-      const selectedInfos = this.layerManager.pickLayer({x, y, mode});
+      const selectedInfos = this.layerManager.pickLayer({


ibgreen · 2017-05-13T00:17:46Z

src/react/deckgl.js

@@ -78,6 +80,17 @@ export default class DeckGL extends React.Component {
    this._updateLayers(nextProps);
  }

+  /* Public API */
+  elementsInScope(topLeft, bottomRight) {


To support the review, write up the doc pages for the public APIs, either as temp comment here or in the doc structure.

ibgreen · 2017-05-13T00:20:22Z

src/lib/draw-and-pick.js

+
+    for (let i = 0; i < pickedColors.length; i += 4) {
+      // Decode picked color
+      const pickedLayerIndex = pickedColors[i + 3] - 1;


Can we update the utilities to handle this, e.g:

const {pickedLayerIndex, pickedColor} = decodePickingColor(pickedColors[...])

howtimeflies0 · 2017-05-13T00:27:49Z

src/lib/draw-and-pick.js

+        const dx = deviceBox.x + col - deviceX;
+        const dy = deviceBox.y + row - deviceY;
+        const d = Math.sqrt(dx * dx + dy * dy);
+        if (d <= minDistanceToCenter) {


This comparison should be implemented using distance squared, instead of distance to save that super expansive sqrt operations

howtimeflies0 · 2017-05-13T00:28:51Z

src/lib/draw-and-pick.js

+        const dx = deviceBox.x + col - deviceX;
+        const dy = deviceBox.y + row - deviceY;
+        const d = Math.sqrt(dx * dx + dy * dy);
+        if (d <= minDistanceToCenter) {


distance comparison is better done using squared values to save on those super expansive Math.sqrt() operation

Alternatively, is there an algo we can use that starts at the center of the bounding box and moves outward until it hits a pick, then bail? If we're only trying to find one result, closest to center..

@ericsoco maybe out of the scope of this PR but a voronoi layer would do that, and it is almost free (a few lines tweak based on the scatterplot layer) in deck.gl.

howtimeflies0 · 2017-05-13T00:45:19Z

src/lib/draw-and-pick.js

+        if (d <= minDistanceToCenter) {
+          minDistanceToCenter = d;
+          closestResultToCenter = pickResult;
+          pickResult.x = deviceBox.x + col;


x, y values are the location of the closest pixel of the picked element not the location of mouse pointer. We probably want to have better names for these two variables to help the people reading the code and also have them documented.

howtimeflies0

Generally a very good PR. Let's present this PR to the team on our next frameworks meeting before we land it. During this time, feel free to add some tests and docs to repo

ericsoco · 2017-05-15T16:41:49Z

src/lib/draw-and-pick.js

-  const deviceY = gl.canvas.height - y * pixelRatio;
+  const deviceX = Math.round(x * pixelRatio);
+  const deviceY = Math.round(gl.canvas.height - y * pixelRatio);
+  const deviceRadius = Math.round(radius * pixelRatio);


In my local pass at this feature (you made it further than I did), I added deviceW / deviceH, to enable marquee selection. Is there any benefit to a circular selection instead of a rectangular one? What's the specific use case for circular other than "fat-finger"?

Ok, reading further I see you're using a box despite the name radius here. Maybe note here that radius really means "half-size"? Or maybe it's fine.

ericsoco · 2017-05-15T17:04:21Z

src/lib/draw-and-pick.js

+  }
+  /* eslint-enable max-depth */
+
+  return closestResultToCenter || {


So if I'm reading this correctly, we only return one picked element? What if the intent is to marquee-select all elements within bounds?

ericsoco · 2017-05-15T17:15:18Z

src/react/deckgl.js

@@ -38,6 +38,7 @@ const propTypes = {
  effects: PropTypes.arrayOf(PropTypes.instanceOf(Effect)),
  gl: PropTypes.object,
  debug: PropTypes.bool,
+  pickingRadius: PropTypes.number,


Out of curiosity, what use cases do you imagine for exposing this? Couldn't hurt, I suppose, just thinking out loud

gnavvy · 2017-05-15T19:09:09Z

Add elementsInScope(topLeft, bottomRight) API to DeckGL component. The method returns an array of unique objects inside the supplied box.

Would you consider to support lasso selection with this elementsInScope() API later?
or to add another lower level API that takes in an array of path points?

Add pickingRadius prop to DeckGL component. The old picking behavior is equivalent to pickingRadius: 0. The closest object to the pointer within pickingRadius pixels is picked.

How do we handle picking for mouse position within the pickingRadius for more than one point now?
(will check the code)
The pickingRadius can be extended later for more general voronoi-based pickings.
pickingRadius: 0 => old picking behavior
pickingRadius: a number => the closest point within the radius
pickingRadius: Infinity => the closest point within the viewport

add picking by bounding box

571a8df

Pessimistress requested review from ericsoco, ibgreen and howtimeflies0 May 12, 2017 23:58

howtimeflies0 reviewed May 13, 2017

View reviewed changes

ibgreen reviewed May 13, 2017

View reviewed changes

howtimeflies0 reviewed May 13, 2017

View reviewed changes

ericsoco reviewed May 15, 2017

View reviewed changes

Pessimistress changed the title ~~Add picking by bounding box~~ Add pickingRadius prop May 15, 2017

Xiaoji Chen added 2 commits May 15, 2017 12:57

Remove elementInScope feature

abd9261

update docs

c66aedb

howtimeflies0 approved these changes May 17, 2017

View reviewed changes

Pessimistress merged commit 9220eb6 into master May 17, 2017

Pessimistress deleted the query-features branch May 17, 2017 21:27

Firenze11 pushed a commit to Firenze11/deck.gl that referenced this pull request May 31, 2017

Add pickingRadius prop (visgl#641)

370fc9a

howtimeflies0 mentioned this pull request Jun 5, 2017

Picking - mobile tap #580

Closed

Add pickingRadius prop #641

Add pickingRadius prop #641

Conversation

Pessimistress commented May 12, 2017 • edited Loading

howtimeflies0 commented May 13, 2017

Pessimistress commented May 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericsoco May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howtimeflies0 May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howtimeflies0 May 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howtimeflies0 May 13, 2017 • edited Loading

Choose a reason for hiding this comment

howtimeflies0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnavvy commented May 15, 2017

Add `pickingRadius` prop #641

Add `pickingRadius` prop #641

Pessimistress commented May 12, 2017 •

edited

Loading

Pessimistress commented May 13, 2017 •

edited

Loading

ibgreen left a comment •

edited

Loading

ericsoco May 15, 2017 •

edited

Loading

howtimeflies0 May 15, 2017 •

edited

Loading

howtimeflies0 May 13, 2017 •

edited

Loading

howtimeflies0 May 13, 2017 •

edited

Loading