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

draggable point example is slow #2334

Closed
ansis opened this issue Mar 25, 2016 · 19 comments
Closed

draggable point example is slow #2334

ansis opened this issue Mar 25, 2016 · 19 comments
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@ansis
Copy link
Contributor

ansis commented Mar 25, 2016

mapbox-gl-js version: v0.16.0

Steps to Trigger Behavior

  1. Drag point in draggable point example http://localhost:4000/mapbox-gl-js/example/drag-a-point/

Expected Behavior

It should be fast and smooth.

Actual Behavior

It's slow and choppy.

@bsudekum
Copy link

I actually can't even drag the marker...

@ansis
Copy link
Contributor Author

ansis commented Mar 25, 2016

I actually can't even drag the marker...

It's broken but fixed by #2333

@mourner
Copy link
Member

mourner commented Mar 25, 2016

As @mcwhittemore points out in #2327, its lagginess depends on how much you're zoomed in. Investigating.

@mourner
Copy link
Member

mourner commented Mar 25, 2016

OK, spent some time investigating this and I found at least two issues that cause most of the lagginess:

  1. The mousedown handler needs e.originalEvent.preventDefault(). I assume this prevents Chrome (and possibly other browsers) from triggering some native drag-handling code that conflicts with point dragging.
  2. As of 1f9029c by @jfirebaugh, Map setPaintProperty (which we use to change point color on hover) calls batch, which in turn always calls style._cascade, which is very expensive to do on every mousemove, especially when the map is full-screen. We should update the code to only cascade on paint property change if we change pseudo-paint properties like text-size (see changing text-size with style api broken #1451).

The dragging becomes much smoother if you fix both (not one or the other).

I'm also convinced that it will get much smoother if mousemove is throttled — should we do this internally for the Map mousemove event? It's a very common gotcha.

@mourner
Copy link
Member

mourner commented Mar 25, 2016

Fixing the second point can also improve Studio responsiveness considerably (cc @samanpwbb), so we should prioritize this. It may be pretty tricky though, considering upcoming DDS changes, after which cascading may be required by paint properties other than text-size and icon-size. How can we tell if a paint property needs cascade or not, without hardcoding? cc @lucaswoj

@mourner mourner added bug 🐞 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage and removed bug 🐞 labels Mar 25, 2016
mourner added a commit that referenced this issue Mar 25, 2016
mourner added a commit that referenced this issue Mar 25, 2016
This was referenced Mar 25, 2016
mourner added a commit that referenced this issue Mar 25, 2016
@jfirebaugh
Copy link
Contributor

Even before 1f9029c Map#setPaintProperty called _cascade, and I don't think "call _cascade only for text-size" is the right condition. Many (most) other cases need _cascade as well -- basically, any time you change a property that should have a visual effect, you need to cascade. The only exception would be changing the property of a class that's not currently active.

@lucaswoj
Copy link
Contributor

We can improve cascade performance by

  • getting rid of paint classes
  • only "cascading" properties that have changed

@jfirebaugh
Copy link
Contributor

Isn't the main issue in this example that there are two mousemove handlers, one of which is:

  • Always active, even when dragging, when it doesn't need to be
  • Always calling setPaintProperty, even when it's a no-op

Before going deeper on library-level optimizations, let's rewrite the example to have a single mousemove handler that calls setPaintProperty only when actually necessary.

@jfirebaugh
Copy link
Contributor

After that, I'd prioritize things like this:

@jfirebaugh
Copy link
Contributor

Another thing that would be useful for this example is mouseenter/mouseleave type delegated events.

@mourner
Copy link
Member

mourner commented Mar 28, 2016

@jfirebaugh yeah, my bad about misunderstanding cascade. The main issue in the example is the preventDefault thing — it's still laggy even if you remove the laggy mousemove completely.

I also think that we should optimize noop setPaintProperty/setLayoutProperty internally — this problem came up in other examples recently too.

@mourner
Copy link
Member

mourner commented Mar 28, 2016

DOM.disableDrag doesn't help for this example, only preventDefault in mousedown does. I have no idea why. What else does it do besides disabling user selection? Could this be a Chrome bug of some sort?

@emiltin
Copy link

emiltin commented May 7, 2016

looking at mapbox gl i was first puzzled by the lack of an easy way to work with markers. i then found the draggable point example which comes close to a marker, but i also get extreme lagging when zoomed into street level on chrome. with firefox zoom level doesn't seem to matter, it's sluggish no matter what, but not extreme.

in my opinion, a good user experience requires that placing and dragging markers is very responsive. it should be possible considering how smooth and responsive e.g. map zooming is.

@lucaswoj
Copy link
Contributor

lucaswoj commented Jul 29, 2016

This is much better now due to improvements in the query*Features and setData APIs.

@bobmoff
Copy link

bobmoff commented Feb 19, 2017

The example is still extremely laggy in Chrome. :(

@livemixlove
Copy link

Is there work being done to improve dragging responsiveness? I'm working on an application that would benefit from very snappy drag-n-drop interactivity, and my code using mapbox is not quite able to do what I want yet. When I look at the mapbox example page, it's not very fast either, even with the minimal example. The circle still delays behind the mouse more than I'd like. I'm assuming that the problem is due to a pretty fundamental design choice where data layers need to be updated via .setData(...) which triggers quite a bit of computation/rerendering.

It's almost like there should be a setting to temporarily give a layer priority rerendering so you can do more tightly interactive animations. Or maybe others have figured out some solutions here?

I know Leaflet is completely different on the backend, but here's an example of acceptable dragging performance:
https://codepen.io/PyroStrex/pen/JKpGKv

@emiltin
Copy link

emiltin commented Dec 19, 2017

I haven't looked into the performance recently, but the comment by @livemixlove seems to indicate the problem is still there.

There must be a way to make dragging fast. It's basic map interaction and poor performance impacts very negatively on user experience.

@livemixlove
Copy link

@emiltin , To be clear, I'm guessing that performance has improved since this issue was first created. I would consider the current drag performance to be "OK", but not great. My metric is "slower than leaflet".

Leaflet:
leaflet-drag-no-lag

example page:
mapboxgl-drag-lag

@m-abboud
Copy link

m-abboud commented Mar 20, 2018

Ended up working around this by overlaying an absolutely positioned seperate canvas on top of the mapbox container with pointer-events off to be used for actively being edited features. So mapbox still handles mouse events and when you edit a shape it moves it to the active canvas and when you stop it saves/moves it back to the mapbox layer data source.

Turned out to be less painful than I thought it would be to add some of this, just use map.project(coords) to get screen x,y coordinates and your in business. Though since my issue was with mapbox gl draw I ended up having to rewrite the different draw modes for doing it this way.

The problem seemed to be much more noticeable when you have additional large vector or raster layers on top of the base map.

Also it's very related to screen resolution. You don't see the lag at all in the tiny little div on the mapbox draw demo but if you copy the code and put it in a full width/height container the lag is noticeable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

No branches or pull requests

9 participants