Add support for drag events. #584

ericsoco · 2017-04-19T20:48:11Z

Note that many mouse events are already supported by luma.gl; this PR extends the support for a subset of these upwards into deck.gl.

I know that we have grander plans for encapsulating interaction models for different scenarios. I don't believe this runs afoul of those, but it is possible the direction taken in this PR is setting us up for a bigger refactor when we tackle new interaction models.

/cc @gnavvy @Pessimistress as potential consumers of these additions.

ericsoco · 2017-04-19T20:49:37Z

Currently absent from this PR: tests and docs. Will tackle after we discuss appropriateness of the feature + implementation.

ibgreen

Code looks good, it extends the existing functionality in the expected way.

My concern here is that we are currently building up about 4 or 5 different almost identical event handling systems (luma.gl, react-map-gl, deck.gl controllers, now the deck.gl component).

All of them will need similar extensions and bug fixes (touch support, support for screen offset relative coordinates, etc).

And how do all these event handlers interact? If I embed this in a controller that supports drag for panning or rotating, will (should?) this still get called?

I believe there is an opportunity to come up with a clean architecture that covers all cases and shares code and principles.

The alternative is that we keep making incremental improvements in all of these places and end up with a patch work of slightly different APIs with different capabilities and bugs.

It may well be that this is the right design, still, I think this merits a design design discussion before we merge.

ericsoco · 2017-04-21T17:36:53Z

@heshan0131 is my last commit going to cause problems with e.g. brushing?

The synopsis: "motion" events are those that are initiated on a given object and are intended to manipulate just that object. Therefore, after the initial event (e.g. dragstart), subsequent events (dragmove and dragend) don't run the picking process at all, they just return the originally-picked object.

I think this is ok for brushing, as the logic for what's within the brushed area is something run in the context of the parent/container (which would be the originally-picked object, and returned by all drag events). But might be missing something.

heshan0131 · 2017-04-21T18:17:41Z

No it will not, currently the brushing example listen to mousemove outside of MapGL component

ibgreen

So we basically just want to keep feeding coordinates with the same layer info to the app, and then a dragend when the mouse is released?

Not sure if it is a good idea, I think the event handlers could be installed dynamically on a click so it may be possible to handle this using a helper class rather than always. The EventManager class does this, I am curious if we could use it here as well.

I'd like to understand the implications of how these events interact with other events handlers, and what happens when things are redrawn during dragging.

ibgreen · 2017-04-21T18:23:30Z

src/lib/draw-and-pick.js

+    // Note that glContextCallback is called synchronously;
+    // code in this function is executed in the order it's written.
+    // TODO - just return glContextWithState once luma updates
+    glContextWithState(gl, {


While I agree that this function need to be split into smaller functions, I think this is split in the wrong place. Move the glContextWithState into the sub function and rename it something meaningful.

It will make the diff a little smaller also since indentation will be unaffected. It would help my review if this was the case.

Just in case: what glContextWithState does is that it locally sets some values on the gl context and guarantees that they are reset even if we exit with an exception.

Makes sense, code will be cleaner to do so.

For future reference, ?w=1 is your friend for muting whitespace changes.

ibgreen · 2017-04-21T18:24:12Z

src/lib/draw-and-pick.js

+    // e.g. to enable dragging behaviors.
+    // Therefore, the picking process does not run for these events.
+    const {layerId} = lastPickedInfo;
+    pickedLayer = layers.find(l => l.props.id === layerId);


Probably you inherited this code but please type out variables: l => layer.

ibgreen · 2017-04-21T18:28:38Z

src/lib/draw-and-pick.js

@@ -23,6 +23,12 @@ import {GL, glContextWithState} from 'luma.gl';
 import {getUniformsFromViewport} from './viewport-uniforms';
 import {log, getBlendMode, setBlendMode} from './utils';

+const MOTION_EVENTS = [
+  'dragmove',


Why do we need to separate between drag and touch? These are abstract modes right?

Sorry, I don't understand -- I'm not making a distinction between them, they're both included here...?

ibgreen · 2017-04-21T18:29:25Z

src/react/deckgl.js

@@ -100,12 +108,18 @@ export default class DeckGL extends React.Component {
    this._updateLayers(this.props);

    // Check if a mouse event has been specified and that at least one of the layers is pickable
-    const hasEvent = this.props.onLayerClick !== noop || this.props.onLayerHover !== noop;
+    const hasEvent =


As discussed, we need to move this code out to a non-react dependent helper. There should just be a few calls in here that delegate to that helper.

That will be part of the future event handling / controller refactor.

ibgreen · 2017-04-21T18:30:17Z

src/react/deckgl.js

+    let mode;
+    let layerEventHandler;
+    switch (type) {
+    case 'mousedown':


You defined touch modes but I don't see touch events handled here.

This PR doesn't as far as supporting touch events. Since we're planning to refactor event handling out anyway, I'm deferring touch event handling for that refactor and focusing only on drag events here (for the graph-layer needs). I'll remove them from MOTION_EVENTS for consistency.

ibgreen

@Pessimistress should probably take a quick look before we merge, since she has done the most recent overhaul of picking.

ibgreen · 2017-04-21T23:10:46Z

src/react/deckgl.js

@@ -52,7 +56,11 @@ const defaultProps = {
  onWebGLInitialized: noop,
  onLayerClick: noop,
  onLayerHover: noop,
-  onAfterRender: noop
+  onAfterRender: noop,


Keep the event handlers together, separated from the render callback.

ibgreen · 2017-04-21T23:12:12Z

src/react/deckgl.js

+      this.props.onLayerDragStart !== noop ||
+      this.props.onLayerDragMove !== noop ||
+      this.props.onLayerDragEnd !== noop ||
+      this.props.onLayerDragCancel !== noop;
    const hasPickableLayer = this.layerManager.layers.map(l => l.props.pickable).includes(true);
    if (this.layerManager.layers.length && hasEvent && !hasPickableLayer) {
      log.once(
        0,


I think we should change the log level to 1.

Note that many mouse events are already supported by luma.gl; this PR extends the support for a subset of these upwards into deck.gl. Handle dragmove/dragend/touchmove/touchend differently: retain the picked object/layer from gesture initiation. Don't process motion events if nothing was picked on the initial mousedown. Skip picking process for "motion events" that only track motion of initially-picked objects. Address some comments Break picking process out into its own function.

coveralls · 2017-04-26T22:38:05Z

Coverage decreased (-0.6%) to 37.359% when pulling 4c07b05 on drag-events into 7171f7b on master.

Pessimistress · 2017-04-26T22:47:43Z

src/react/deckgl.js

+      onDragStart: this._onDragEvent,
+      onDragMove: this._onDragEvent,
+      onDragEnd: this._onDragEvent,
+      onDragCancel: this._onDragCancel


We're invoking picking on all of these events as long as one of them is specified. Can we add handlers only when they are not noop?

Actually, digging deeper into this, we're always running picking, even if no layers have pickable set. There's no check for existence of handlers or pickable truthiness before calling addEvents, we're just running picking on every mouse move and calling noops every frame.

Additionally, we currently have no facility for adding/removing event handlers after initialization; deck.gl calls luma's addEvents only once, on init; and addEvents itself does not support adding/removing handlers. We need to address this in the event handling refactor, @ibgreen.

All this indicates to me that:

we should definitely fix this

we should fix it in the event handling refactor, not in this PR.

If @ibgreen and @Pessimistress agree, I'd like to merge as-is.

We definitely should not register events based on pickable states at initialization. In most of our use cases the layers are not passed in till data is loaded. Since we're only logging a warning it's not broken at the moment.

I agree this should be fixed in a separate PR.

Pessimistress · 2017-04-26T22:48:58Z

src/react/deckgl.js

+      this.props.onLayerDragStart !== noop ||
+      this.props.onLayerDragMove !== noop ||
+      this.props.onLayerDragEnd !== noop ||
+      this.props.onLayerDragCancel !== noop;


This is an anti-React-pattern: these props are only checked once on itialization.

You're right @Pessimistress, this is an anti-pattern. However, it's only checking to offer a warning, in the case that an onLayer* handler is passed when no layers have set pickable; I imagine the overwhelmingly most common use case will be to set onLayer* handlers on init, not dynamically at some point later in the program. Checking these props on every update just to conditionally issue a warning doesn't seem worth the (probably minor) cost.

My vote is we leave as-is unless it proves to be a problem.

* Add support for drag events. Note that many mouse events are already supported by luma.gl; this PR extends the support for a subset of these upwards into deck.gl. Handle dragmove/dragend/touchmove/touchend differently: retain the picked object/layer from gesture initiation. Don't process motion events if nothing was picked on the initial mousedown. Skip picking process for "motion events" that only track motion of initially-picked objects. Address some comments Break picking process out into its own function. * Address more comments.

ericsoco requested review from ibgreen and howtimeflies0 April 19, 2017 20:48

ibgreen reviewed Apr 19, 2017

View reviewed changes

ibgreen reviewed Apr 21, 2017

View reviewed changes

ibgreen approved these changes Apr 21, 2017

View reviewed changes

ericsoco force-pushed the drag-events branch from 67d0d18 to 8da6c06 Compare April 22, 2017 02:14

ericsoco added 2 commits April 26, 2017 15:28

Address more comments.

4c07b05

ericsoco force-pushed the drag-events branch from f1599a7 to 4c07b05 Compare April 26, 2017 22:28

ericsoco requested a review from Pessimistress April 26, 2017 22:31

Pessimistress reviewed Apr 26, 2017

View reviewed changes

ericsoco merged commit 12e7f49 into master Apr 27, 2017

ericsoco deleted the drag-events branch April 27, 2017 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for drag events. #584

Add support for drag events. #584

ericsoco commented Apr 19, 2017

ericsoco commented Apr 19, 2017

ibgreen left a comment

ericsoco commented Apr 21, 2017

heshan0131 commented Apr 21, 2017

ibgreen left a comment

ibgreen Apr 21, 2017

ericsoco Apr 21, 2017

ericsoco Apr 21, 2017

ibgreen Apr 21, 2017

ibgreen Apr 21, 2017

ericsoco Apr 21, 2017

ibgreen Apr 21, 2017

ericsoco Apr 21, 2017

ibgreen Apr 21, 2017

ericsoco Apr 21, 2017

ibgreen left a comment

ibgreen Apr 21, 2017

ibgreen Apr 21, 2017

coveralls commented Apr 26, 2017 •

edited

Pessimistress Apr 26, 2017

ericsoco Apr 26, 2017

Pessimistress Apr 27, 2017

Pessimistress Apr 26, 2017

ericsoco Apr 26, 2017

Add support for drag events. #584

Add support for drag events. #584

Conversation

ericsoco commented Apr 19, 2017

ericsoco commented Apr 19, 2017

ibgreen left a comment

Choose a reason for hiding this comment

ericsoco commented Apr 21, 2017

heshan0131 commented Apr 21, 2017

ibgreen 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 26, 2017 • edited

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

coveralls commented Apr 26, 2017 •

edited