mapbox / mapbox-gl-js

Interactive, thoroughly customizable maps in the browser, powered by vector tiles and WebGL
https://docs.mapbox.com/mapbox-gl-js/
Other
11.23k stars 2.23k forks source link

Highlight polygons becomes extremely slow when large number of features present #2225

Closed rantao closed 8 years ago

rantao commented 8 years ago

mapbox-gl-js version: v0.15.0

Steps to Trigger Behavior

  1. Be at a zoom level that displays a lot of features
  2. Move mouse over the map

    Expected Behavior

The hover highlight should be immediate with movement of the mouse, even if you move the mouse fast.

Actual Behavior

There is an increasing lag as the # of features in the canvas increases and as you increase the speed of mouse movement. You can see a trail of polygons being painted if you move your mouse across the screen before it finally 'catches up.' It looks like the hover requests are queued up instead and not rendered async.

Video of the delayed highlight with ~5000 features

I understand this was improved A LOT already in a recent update (#2174). But it is still not usable for my scenario when there can be > >10,000 features easily. And there is already a visible performance issue at even > 1,000 features. I've tried to only show hover when there are less features on the canvas but it makes for a confusing/broken ux so i will turn it off until it can be more performant.

Code for adding highlight layer:

map.addLayer({
    'id': 'highlight-hover',
    'type': 'fill',
    'source': 'terrain-data',
    'source-layer': 'mylayer',
    'paint': {
        'fill-outline-color': '#FFFFFF',
        'fill-color': '#ffffff',
        'fill-opacity': 0.4
    },
    'filter': ['==', 'geoid10', '']
  });

Code for triggering highlight of polygon on mousemove:

  map.on('mousemove', function(e) {
    map.featuresIn(map.getBounds(), function(err, features) {
      if (err) console.warn(err);
      map_feature_size = features.length;
    });
    if (map_feature_size <= 1000)  {
      map.featuresAt(e.point, {
        radius : 1,
        includeGeometry: true
      }, 
      function (err, features) {
        if (!err && features.length) {
          map.setFilter("highlight-hover", ["==", "geoid10",features[0].properties.geoid10]);
        } else {
          map.setFilter("highlight-hover", ["==", "geoid10", ""]);
        }
      });
    }
  });
averas commented 8 years ago

My personal experience is that when you only want to deal with a proportionally small set of features out of a set of many features, such in your use case, it's much more efficient to maintain a separate GeoJSON source that you transfer your "filtered" features to.

In your example the filtering has to be carried out over the entire data set without any efficient indexing every time you hover over a new feature. If you instead copy the feature geometry of the feature you hover over to a separate source, the only lookup carried out frequently will be the featuresAt, which is an indexed lookup.

I've used this approach successfully for sets with hundreds of thousands of features, and a variant of it (including source) can be found at the Mapbox Blog: https://www.mapbox.com/blog/properties-philly/

So, in your case, if you want to try it out, add a new GeoJSON source to your map, such as terrain-data-hover, change your highlight-hover layer to use this source and finally change your featuresAt call to set the data of your new source to whatever is found (or clear it in case nothing is found).

mourner commented 8 years ago

@rantao why do you do featuresIn for the whole map bounds on mousemove? This is very expensive and also seems unnecessary. Also, could you set up a JSFiddle with your test case to demonstrate it live?

averas commented 8 years ago

@mourner

why do you do featuresIn for the whole map bounds on mousemove?

Based on...

I've tried to only show hover when there are less features on the canvas

... I believe this was an effort to try to limit when the featuresAt-call is carried out.

But I agree, this will only hamper performance even more.

mourner commented 8 years ago

@averas avoid featuresIn calls with a big bounding box at all costs. Small targeted queries (small radius/bbox) are thousands of times faster.

averas commented 8 years ago

@mourner Yep. Again, this was probably a trail-and-error measure by @rantao to try to increase performance without any deeper analysis of the behind-the-scenes workings.

I have several implementations of both featuresIn and featuresAt on very large datasets (millions of features), and as you say, as long as you keep the query as narrow as possible (for example with the smallest bbox possible) things are quite performant even with these data set sizes. That's why I have a hunch this is rather related to the filtering which I believe (correct me if I'm wrong) is not indexed at all, so if you change a filter on a 10k GeoJSON data source you're looking at a full scan of that set, i.e. O(N), which may be too much considering the number of events triggered by a mousemove.

mourner commented 8 years ago

@averas yes, in filter change is O(N log M), where M is the number of items in the filter.

One good strategy of improving hover performance on the app side is throttling the mousemove handler. Try 16ms (60fps), then 32ms (30fps) etc. until performance is acceptable.

mourner commented 8 years ago

Another part that's quite expensive is includeGeometry: true. In the example it is not necessary too, as the geometry is not used, so setting this to false should improve performance significantly.

rantao commented 8 years ago

@averas is correct. I was only using the featuresIn call as a temporary limitation. Sorry for the confusion but my actual code calls featuresIn only on pan/zoom changes not in mousemove. I just copied into mousemove for the purposes of showing where my variable map_feature_size is set.

As for a live demo, unfortunately my data is on my local, so it's not easy to create a jsfiddle.

I am currently trying some of the suggestions from both @mourner and @averas (that philly example is pretty cool). Will update whether anything solves this issue :)

Thanks so much!

rantao commented 8 years ago

So removing includeGeometry, and the featuresIn call helped but there is still a lag. I played around with throttling of the mouse movement and decided to go with debounce for a temporary solution.

In the end, there should be an update to how mousemove is handled. If mousemove is triggered and there something that is still being rendered from the last mousemove, then the current call should override it. This way we don't have a massive queue and trail of highlighted polygons that no longer need to be rendered. Please let me know when this behavior can be added. Thanks!

averas commented 8 years ago

Your stream of events should be pushed through a throttle rather than a debounce (if that is in fact what you do). You can essentially reuse the code from mapbox-gl-js-draw: https://github.com/mapbox/mapbox-gl-draw/blob/master/src/util.js#L180

I believe your suggestions on mousemove handling may be too use case specific to make a general case out of. These kind of things are probably best handled by client code.

Did you try my other suggestion? Copying the features to a separate source/layer.

mourner commented 8 years ago

If mousemove is triggered and there something that is still being rendered from the last mousemove, then the current call should override it

The problem is we don't know when GPU finishes rendering, this operation is async and has no callbacks. The best we can do is throttle calls like setFilter so that they're not called more often than, say, 30fps.

lucaswoj commented 8 years ago

Lots of great advice in this issue! I'm going to close this as I feel that actionable follow-ups are captured by #1002 #2224 and #1504