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

Emit plotly_relayouting event during drag interactions #2606

Closed

Conversation

zhihua-chen
Copy link

Currently a gl3d plot does not emit 'plotly_relayout' while dragging is happening. The event is not emitted until 'mouseup'. This PR will let a gl3d emit the 'plotly_relayout' event while the mouse is moving.

@etpinard
Copy link
Contributor

etpinard commented May 3, 2018

I'd vote 👎 here. This would make 3D drag inconsistent with its cartesian counterpart. You may argue that 3D drag is already inconsistent, so what gives? But, going forward we should try to make interactions on all our subplot types more consistent, not the other way around.

Now, I'm not against adding another event (e.g. plotly_dragging). But first, we need to:

  • Make sure triggering an event on mousemove doesn't affect performance.
  • We might need to throttle it?
  • We'll need to add something similar to cartesian, ternary, polar and geo subplots

@zhihua-chen
Copy link
Author

@etpinard Thanks for the feedback! When you said 3D drag is "already inconsistent", were you referring to 3D zooming which does emit 'plotly_relayout' events as the zooming is happening?

Haven't really studied the performance issue. FYI, in my situation, I'm trying to create a synchronized set of 3d scatter plots. Trying to make all of them rotate in real time as you drag on one of them... So far things are pretty smooth with a set of 3 synchronized plots with ~4500 points in each.

@alexcjohnson
Copy link
Collaborator

As you point out @zhihua-chen wheel is already emitting a relayout event for every DOM event... which is different from 2D where we wait a somewhat arbitrary delay time after the last wheel event we see before firing the relayout. We really should make 3D work like 2D here, at least once we have these intermediate events implemented.

I can't imagine that simply emitting the event would impact performance noticeably, right? It's how the developer responds to that event that matters, but I think we should let them decide how much performance hit is acceptable in their own application. Likewise I don't think we should throttle these events, let developers do that themselves if it suits their application - though at some point perhaps it would be neat to build that into our events API, like gd.onThrottled(event, callback, minDelay)?

Can we do this in a way that explicitly mirrors relayout so that a) it's clear what the event data structure will be, and b) it will apply not just to drags but wheel events as well? That could be plotly_relayouting, or plotly_relayoutpending, something like that?

@etpinard
Copy link
Contributor

That could be plotly_relayouting

I'm a fan of this. This mirrors plotly_selecting for selection.

@zhihua-chen would you be interested in adding plotly_relayouting to our other subplot types?

@zhihua-chen
Copy link
Author

@etpinard Sure. Do you have a list of the plot types so I don’t leave anything out?

@etpinard
Copy link
Contributor

@zhihua-chen sure!

  • 3D (just make sure plotly_relayouting is emitted not plotly_relayout on mousemove)
  • Cartesian (plotly_relayouting should be emitted from zoomMove and plotDrag)
  • Polar (in zoomMove)
  • Ternary (in zoomMove and plotDrag)
  • Geo (in the various handleZoom handlers of geo/zoom.js)
  • Mapbox (in the mousemove handler)

@zhihua-chen
Copy link
Author

Writing down some notes and questions as I go through the code.

What payload should be emitted with these events?

With gl3d, this was easy, just emit the updated camera location.

What about other chart types? Should the event payloads for “relayouting” be standardized somehow?

People must have thought about this when they decided to emit the “relayout” event. Are payloads for this event already consistent across chart types?

Is there a place to document these events?

@etpinard
Copy link
Contributor

What about other chart types? Should the event payloads for “relayouting” be standardized somehow?

Easy answer. The payload should be the same as the associated plotly_relayout event.

@etpinard
Copy link
Contributor

Is there a place to document these events?

We have https://plot.ly/javascript/plotlyjs-events/, but you don't need to worry about it.

@zhihua-chen
Copy link
Author

When should the event be plotly_relayouting and when should it simply be plotly_relayout?

For zooming in cartesian plots, it makes sense to have a relayouting event in zoomMove for intermediate drag action and a relayout for when the drag action ends and plot gets resized.

For zoomwheel and panning in cartesian plots, there are no intermediate steps. Once the you move the mouse or scroll the wheel, the plot is redrawn immediately (a small delay is set in the case of zoomwheel.)

Currently, zoomwheel emits a plotly_relayout in dragTail for each wheel scroll. This is great. Should we make panning behave the same way: emitting a 'plotly_relayout` every time the mouse moves?

@alexcjohnson
Copy link
Collaborator

When should the event be plotly_relayouting and when should it simply be plotly_relayout?

Every time we actually alter gd.layout (via a Plotly.relayout call, or directly if we've short-circuited that) there should be an associated plotly_relayout event. If we've only altered the appearance on-screen but haven't propagated the change to gd.layout yet, it should be a plotly_relayouting event instead. So with that in mind:

Currently, zoomwheel emits a plotly_relayout in dragTail for each wheel scroll.

dragTail is what actually updates gd.layout, so that's the correct place to emit plotly_relayout. But in fact we update the viewbox (on-screen appearance) in principle many times before we get to dragTail (notice that zoomWheel calls clearTimeout(redrawTimer);) so it would be reasonable to add a plotly_relayouting event inside zoomWheel.

Should we make panning behave the same way: emitting a plotly_relayout every time the mouse moves?

panning works the same as zoom - on mousemove we just change the viewbox but do not alter gd.layout and redraw properly until mouseup. So like zoom, mousemove should have a plotly_relayouting event, not plotly_relayout.

@zhihua-chen
Copy link
Author

zhihua-chen commented May 29, 2018

Thanks for the detailed response @alexcjohnson, that is really helpful.

Please forgive this newbie for some more questions.

Question 1: why the multiple layers of drawing action? What determines what gets redrawn during the interaction and what gets redrawn at the relayout?

Related question 2: zoomWheel employs a debounce, why doesn’t panning? gl3d interactions do not use a debounce right now, do they need one?

Wait. Answering part of the question 2 myself, panning doesn’t do relayout until dragging ends. So the debounce in zoomWheel is more about reducing number of relayout calls.

@alexcjohnson
Copy link
Collaborator

Please forgive this newbie for some more questions.

No problem at all! I'm happy to answer lots of questions for anyone contributing a PR 😎

why the multiple layers of drawing action? What determines what gets redrawn during the interaction and what gets redrawn at the relayout?

Just performance - we can translate and rescale very efficiently, but after that's done we need to actually recalculate & repositions the points to be displayed, and depending on the new ranges there may be more (or less) detail in the new view - for example scatter plots employ more aggressive decimation (and eventually clipping) the farther you get from the visible viewport.

zoomWheel employs a debounce, why doesn’t panning? gl3d interactions do not use a debounce right now, do they need one?

Wait. Answering part of the question 2 myself, panning doesn’t do relayout until dragging ends. So the debounce in zoomWheel is more about reducing number of relayout calls.

You got it! zoom/pan effectively have a built-in debounce with the multiple mousemoves before mouseup. zoomWheel only has one event type.

The performance considerations are very different for SVG than for WebGL. It might be worthwhile (cc @etpinard) to make the WebGL events look more like the SVG ones though, both to signal that the plot is mid-interaction so should perhaps not be subjected to other modifications (see eg #2644) and to limit effects on performance of the rest of the application (so we can allow users to choose whether to respond to every mousemove or just the final view)... but we may need to wait for v2 for that as it would be a somewhat breaking change.

@zhihua-chen
Copy link
Author

All right. I have added layouting events to all chart types @etpinard listed.

@etpinard
Copy link
Contributor

etpinard commented Jun 4, 2018

All right. I have added layouting events to all chart types @etpinard listed.

Nice job @zhihua-chen . Would you be interesting in adding test cases for each of these new event triggers?

@etpinard etpinard changed the title Generate events while dragging gl3d plots. Emit plotly_relayouting event during drag interactions Jun 4, 2018
@zhihua-chen
Copy link
Author

sure. busy with work though. will take a while...

@etpinard
Copy link
Contributor

etpinard commented Apr 3, 2019

@zhihua-chen are you still interested in trying to bring this PR to the finish line?

If not, I might have time to add a few commits in time for 1.47.0.

@zhihua-chen
Copy link
Author

zhihua-chen commented Apr 3, 2019 via email

@archmoj
Copy link
Contributor

archmoj commented Apr 11, 2019

Solving merge conflicts for this PR is rather risky considering the changes since last year. I suggest we may close this PR and open a feature request to possibly address relayouting for gl3d and other traces. @etpinard what do you think?

@etpinard
Copy link
Contributor

👍 for opening a new PR. I would wait until this new PR exists before closing this one here, to keep an active placeholder.

@etpinard
Copy link
Contributor

Closing. I just realised there's an issue opened for this already -> #2082

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants