Move event management from deckgl.js React component into LayerManager. #708

ericsoco · 2017-06-15T06:12:20Z

*** * * DO NOT MERGE, WIP * * ***

This PR tackles the EventManager followup items in #656. Among its goals:

move event management into LayerManager
fix to the picking-on-every-event bug (Picking always runs #634)
begin to remove drag handling (Drag event handling is implemented incorrectly #648)

More to come.

ibgreen

I really like the direction this is taking.

Some comments mostly to make sure we have considered various design options.

Once this is done, I think it would be very easy to create a non-react based example using deck.gl, just using basic JavaScript setting up the LayerManager and EventManager and a luma.gl AnimationLoop

ibgreen · 2017-06-15T13:54:18Z

src/lib/layer-manager.js

    return this;
  }

+  addEventListeners(eventNames) {


I am wondering if we should keep the event registration outside of the LayerManager, so that the user could use some other event generation.

Hm...so then EventManager or an equivalent would be passed into LayerManager? This feels a bit heavy, though I can see how it may improve extensibility for event handling. I wonder though how useful that flexibility is, given it would slightly increase complexity of the API.

We could make it an optional param and assume most people will use the default, which we would set to EventManager. Is that what you're thinking?

Hm...so then EventManager or an equivalent would be passed into LayerManager?

I had a different model in mind. The external event handling would be independent and just call layerManager.onClick and layerManager.onPointerMove when events occurred to get help with picking and delivering the resulting infos to the deck.gl callback system in the right way. Admittedly, I haven't thought it through completely.

ibgreen · 2017-06-15T14:00:27Z

src/lib/layer-manager.js

+   * with any picking info generated by `pickLayer`.
+   */
+  _onClick(event) {
+    const pos = getPos(event);


So the main thing the user needs to specify is how to extract the position from the event. The rest of the logic here is independent of event handling system, right?

So exploring the idea of keeping event registration separate, we could potentially move out the event registration, keep these functions in here (maybe removing the _ underscore prefixes) so that the can be called by the external event handlers, and add a getPositition argument to the LayerManager constructor ?

ibgreen · 2017-06-15T14:02:22Z

src/lib/layer-manager.js

+    const radius = this.pickingRadius;
+    const selectedInfos = this.pickLayer({x: pos.x, y: pos.y, radius, mode: 'click'});
+    if (selectedInfos.length) {
+      const firstInfo = selectedInfos.find(info => info.index >= 0);


Should we just have a function pickTopLayer so we can drop the find here? @Pessimistress

ibgreen · 2017-06-15T14:04:17Z

src/lib/layer-manager.js

+/**
+ * Get mouse position {x, y} relative to the container
+ */
+function getPos(event) {


Abbreviated names are typically avoided in this code base: getPos => getPosition

ibgreen · 2017-06-15T14:07:38Z

src/lib/layer-manager.js

+    return null;
+  }
+
+  const rect = rootElement.getBoundingClientRect();


I am not an expert on these things, but folks who seem to be have advised me in the past to avoid calling getBoundingClientRect on any regular basis, as it can trigger an expensive reflow in the browser.

My understanding was that events have offsetX and offsetY or similar parameters that contain relative coordinates so that this can be handled in a more lightweight fashion.

I wrote a comment here earlier today and GH seems to have swallowed it.

IIRC I suggested we use rootElement.offsetLeft/Top instead of getBoundingClientRect() to avoid reflows. @Pessimistress what's the use case here, was this logic added to handle the fact that touch events don't have clientX/Y or to handle situations in which the canvas doesn't fill the viewport?

ibgreen · 2017-06-15T14:10:03Z

src/react/deckgl.js

  }

  componentWillReceiveProps(nextProps) {
+    this._setLayerManagerProps(nextProps);


Feels a little misleading since LayerManager is a traditional JS class and doesn't really have props. Just call it _updateLayerManager?

ibgreen · 2017-06-15T14:13:58Z

src/lib/layer-manager.js

+    // TODO
+  }
+
+  validateEventHandling() {


While I understand why you put this here, I'm a little torn about it. Can this be moved "deeper" into the layer manager life cycle?

Yes -- after I wrote this I realized I needed to store onLayerClick and onLayerHover somewhere that LayerManager could get at them. That means now that all this logic can live within LayerManager instead of straddling across deckgl.js as well.

ibgreen · 2017-06-15T14:17:18Z

src/react/deckgl.js


    // Note: avoid React setState due GL animation loop / setState timing issue
-    this.layerManager = new LayerManager({gl});
+    this.layerManager = new LayerManager({gl, canvas});


FWIW: WebGLRenderingContext.canvas

There's no change here in the way we're using canvas, just passing it along to LayerManager for EventManager event registration. Is there some caveat we're not covering here (hence the link)?

The link was just to show that canvas is already available as a member on the gl context that we are already passing as a parameter, so it might be unnecessary to add the canvas parameter.

But perhaps we want to handle the case where the event canvas is different from the canvas that the context was created on.

I was thinking that the DOM element handling events should not be coupled with the gl context. If event handling exists in its own module, alongside LayerManager (instead of within it), this will be moot anyway (it will accept an arbitrary DOM element just like EventManager does).

Begin to remove drag event handling. Remove unused event handler props from webgl-renderer.js.

…ct deck.gl component and LayerManager. This enables a non-react app to opt into event handling. This commit is a quick pass for exposition and needs cleanup + tests.

ericsoco · 2017-06-17T22:44:07Z

The diff is a little weird now due to @Pessimistress' PR and my rebasing onto it.
Here is a more legible version (that just diffs between the last commit and master).

ibgreen · 2017-06-18T17:43:16Z

src/lib/layer-event-manager.js

+    // but do so in a way that doesn't cause reflow (e.g. `offsetWidth/Height`).
+    // maybe the event object offers offsetCenter as a 0<>1 value as well?
+    // since it's already doing size calculations...
+    const {width, height} = this.layerManager.context.viewport;


Right or wrong, my expectation was that this file would contain code that was unique to doing event integration with the EventManager.

The code in _onPointerMode and _onClick (with the exception of the refernece to oevent.offsetCenter) line like it would be identical for any event system integration, and so might be better to keep inside LayerManager.

Thus

_onPointerMove(event) { this.layerManager.onPointerMode({event, pos: event.offsetCenter}); }

Doing it like this woud also mean that this class wouldn't need to rake for the LayerManagers internal state like the context.viewport.

Makes sense, will do.

Pessimistress · 2017-06-18T23:06:02Z

src/lib/layer-event-manager.js

+
+  set(options = {}) {
+    Object.keys(DEFAULT_OPTIONS).forEach(k => {
+      this.options[k] = options[k] !== 'undefined' ? options[k] : DEFAULT_OPTIONS[k];


undefined?

crap, good catch.

ericsoco · 2017-06-19T19:56:00Z

@ibgreen and I discussed where to draw the lines between modules here.

Currently (before this PR):

deckgl.js listens for <canvas> events from event-manager.js and handles them by calling:
the per-layer handlers (e.g. onClick), which are set on the layers themselves and called by draw-and-pick.js, via 'layer-manager.js';
the top-level event handlers (e.g. onLayerClick), which belong to deckgl.js.

A cleaner implementation of all of this would involve:

lifting per-layer handler calls out of draw-and-pick.js and instead calling them from layer-manager.js based on the picking info objects returned from draw-and-pick.js::pickLayers;
allowing layer-manager to manage top-level event handlers as well;
calling both per-layer and top-level handlers from a single function per event type, that lives on layer-manager.js;
keeping the connection to event-manager.js for DOM event handling broken out separately from either deckgl.js or layer-manager.js so that applications can use their own DOM event handling if so desired.

In this scenario, layer-event-manager.js acts as a handler pass-through from deckgl.js to layer-manager.js for React apps, and can either be used or bypassed by non-React apps to connect DOM events to layer-manager.js's handler functions that call per-layer and top-level event handlers.

I'll rework this PR a bit further toward this goal, but will refrain from lifting per-layer callbacks out of draw-and-pick.js until another PR.
/cc @ibgreen

Pessimistress · 2017-06-21T17:41:40Z

Is the latest luma.gl still registering its own event handlers? If so, why are we not using those?

ibgreen · 2017-06-21T17:51:02Z

Is the latest luma.gl still registering its own event handlers? If so, why are we not using those?

luma.gl has a legacy system for registering event handlers, an optional addEvents function. It does not get automatically called, it is a utility that deck.gl can choose to use.

Due to the complexity of the event handling topic and the fact that we already have two repos in play I have intentionally kept luma.gl event handling out of the discussions.

A problem with the addEvents that I just ran into is that there is no removeEvents, which I would need to handle the new example website in a good way. So it could make sense to ask whether to invest more time in the old system, or whether to bring the new system into luma.gl as well.

ericsoco · 2017-06-21T19:43:54Z

@Pessimistress we replaced a luma addEvents() call in deckgl.js with EventManager. AFAIK (and per @ibgreen above) luma's event handling isn't used anywhere else by deck.gl.

Adopt to new luma state management api.

Begin to remove drag event handling. Remove unused event handler props from webgl-renderer.js.

…ct deck.gl component and LayerManager. This enables a non-react app to opt into event handling. This commit is a quick pass for exposition and needs cleanup + tests.

…o events-in-layer-manager

Move all input event handling logic into LayerManager.

ericsoco · 2017-06-22T06:37:11Z

@ibgreen here's a diff for the state of this PR as of the latest push, rebased onto master.

This commit does away with LayerEventManager and instead leaves layer event handling in control of LayerManager.

In the interest of closing this PR, I stopped short of fixing the picking-on-every-event bug (#634), since it will require adding/removing handlers to/from EventManager every time new top-level handlers are passed to setEventParams() or layers are updated with new per-layer handlers.

For the same reason, I'm also leaving for another PR the work to pull the per-layer handler calls out of draw-and-pick.js.

howtimeflies0

@ericsoco Great work for the PR.

Since this PR actually introduced some complicated merge actions between various branches, could you close this PR and open a new one? I have cherry-picked your commits to the new branch events-in-layer-manager2 based on latest master.

ericsoco · 2017-06-22T16:13:23Z

Closing in favor of #738.

ericsoco requested review from Pessimistress and ibgreen June 15, 2017 06:12

ibgreen reviewed Jun 15, 2017

View reviewed changes

ericsoco added 2 commits June 17, 2017 13:55

Move event management from deckgl.js React component into LayerManager.

59087aa

Begin to remove drag event handling. Remove unused event handler props from webgl-renderer.js.

Break event handling out into its own module, decoupled from both rea…

b177245

…ct deck.gl component and LayerManager. This enables a non-react app to opt into event handling. This commit is a quick pass for exposition and needs cleanup + tests.

ericsoco force-pushed the events-in-layer-manager branch from 5bc59c4 to b177245 Compare June 17, 2017 22:38

ibgreen approved these changes Jun 18, 2017

View reviewed changes

Pessimistress approved these changes Jun 18, 2017

View reviewed changes

ericsoco mentioned this pull request Jun 19, 2017

Events refactor followups #656

Closed

11 tasks

1chandu and others added 6 commits June 21, 2017 15:53

Merge pull request #726 from uber/stateImprovements-withParameters

b4a5b7d

Adopt to new luma state management api.

Change queryObjects to queryVisibleObjects (#736)

955d457

Move event management from deckgl.js React component into LayerManager.

ad1bd0f

Begin to remove drag event handling. Remove unused event handler props from webgl-renderer.js.

Break event handling out into its own module, decoupled from both rea…

4753260

…ct deck.gl component and LayerManager. This enables a non-react app to opt into event handling. This commit is a quick pass for exposition and needs cleanup + tests.

Merge branch 'events-in-layer-manager' of github.com:uber/deck.gl int…

0fb47e4

…o events-in-layer-manager

Eliminate intermediate LayerEventManager.

e6895c2

Move all input event handling logic into LayerManager.

howtimeflies0 suggested changes Jun 22, 2017

View reviewed changes

ericsoco mentioned this pull request Jun 22, 2017

Move event management from deckgl.js React component into LayerManager. #738

Merged

ericsoco closed this Jun 22, 2017

balthazar deleted the events-in-layer-manager branch July 31, 2017 18:29

Move event management from deckgl.js React component into LayerManager. #708

Move event management from deckgl.js React component into LayerManager. #708

Uh oh!

Conversation

ericsoco commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibgreen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibgreen Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsoco commented Jun 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsoco commented Jun 19, 2017

Uh oh!

Pessimistress commented Jun 21, 2017

Uh oh!

ibgreen commented Jun 21, 2017

Uh oh!

ericsoco commented Jun 21, 2017

Uh oh!

ericsoco commented Jun 22, 2017

Uh oh!

howtimeflies0 left a comment

Choose a reason for hiding this comment

Uh oh!

ericsoco commented Jun 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ericsoco commented Jun 15, 2017 •

edited

Loading

ibgreen Jun 15, 2017 •

edited

Loading