visgl / react-map-gl

React friendly API wrapper around MapboxGL JS
http://visgl.github.io/react-map-gl/
Other
7.82k stars 1.35k forks source link

> 200 markers cause map slow when dragging #750

Closed thanhphuong612 closed 4 years ago

thanhphuong612 commented 5 years ago

Hi,

When I render more than 200 markers, I notice the map start moving very slow when dragging. I have no issue with mapbox-gl-js, even with more than 400 markers written in React.

I decided to switch to react-map-gl because I wrote all markers and popup in React. It becomes cumbersome when integrating mapbox-gl-js with these components.

I can't change those markers to layer as I need to render 'complex' content (with html, css, etc.) I have tried to improve the performance by adding conditions to shouldComponentUpdate to only update when zoom, lat long, width, height changes. Also using memo or PureComponent for markers, popup. The performance gets slightly better but still, dragging around the map is slow.

Can anyone advice if is there a solution for my issue please? Thank you.

Pessimistress commented 5 years ago

Can you share a sample of your component that renders the marker content?

thanhphuong612 commented 5 years ago

Hi @Pessimistress , here you go. It's pretty simple:

const Marker = styled('div')`
    background: ${({isPinned}) => isPinned ? markerColor.pin : markerColor.unpin}; 
    border: 1px solid #ffffff;
    border-radius: 3px;
    font-size: 12px;
    color: #ffffff;
    width: 42px;
    height: 24px;
    font-weight: 400;
    display: flex;
    justify-content: center;
    align-items: center;

    &:before {
        content: "";
        position: absolute;
        top: 22px;
        left: 15px;
        height: 0;
        border-style: solid;
        border-width: 5px 5px 0 5px;
        border-color: ${({isPinned}) => isPinned ? markerColor.pin : markerColor.unpin}  transparent transparent transparent;
        z-index: 9999;
    }

    &:after {
        content: "";
        position: absolute;
        top: 23px;
        left: 15px;
        width: 0;
        height: 0;
        border-style: solid;
        border-width: 5px 5px 0px 5px;
        border-color: #ffffff transparent transparent transparent;
        z-index: 9998;
    }

    &:hover {
        background: rgb(50,139,210);
        width: 50px;
        height: 26px;
        font-size: 13px;
        font-weight: 600;

        &:before {
            top: 25px;
            left: 19px;
            border-color: rgb(50,139,210) transparent transparent transparent;
        }

        &:after {
            top: 26px;
            left: 19px;
        }
    }
`;

const PriceMarker = memo(({ amount, hotel, onMouseOver, onMouseOut, onClick, isPinned }) => {
    return (
        <Marker
            onMouseOver={() => onMouseOver(hotel)} 
            onMouseOut={onMouseOut}
            onClick={() => onClick(hotel.hotelId)}
            isPinned={isPinned}
        >{amount}
        </Marker>

    );
});

Thank you

rufushonour commented 5 years ago

This is also happening for me. I'm seeing the markers get rerendered every time the viewport changes. Which causes performance issues with a large amount of markers. I'm using the <Marker /> component directly in the children of <ReactMapGL />

jancalve commented 5 years ago

Same issue here - performance drops drastically as the number of markers increase as they're being re-rendered when panning. I'm using component directly in the children of as well.

Pessimistress commented 5 years ago

@thanhphuong612 how are you using this with MapGL and Marker?

The child of a react-map-gl <Marker> is passed through as is. You need to make sure 1) it's a pure component and 2) no prop changes during a rerender.

thanhphuong612 commented 5 years ago

@Pessimistress I should have made myself clearer in the previous post. The child of a react-map-gl <Marker> is the PriceMarker functional component above. I confirm: 1) Yes it's a pure component, the Marker/PriceMarker is not re-rendering when I am dragging the map around. I am using React.memo with functional component, which is similar to Pure Component with class.

2) Yes: no prop changes during a rerender. This is my Map class:

export default class Map extends Component {
    shouldComponentUpdate(nextProps, nextState) {
        return nextState.viewport.latitude !== this.state.viewport.latitude || 
            nextState.viewport.longitude !== this.state.viewport.longitude ||
            nextState.viewport.zoom !== this.state.viewport.zoom ||
            nextState.viewport.width !== this.state.viewport.width ||
            nextState.viewport.height !== this.state.viewport.height ||
// and some other conditions to control popups etc.
    }

   renderMarkers = (results) => {
       return results.map(item => {
            const amount = formatPrice(item.price.currency, item.price.amount);

            return (
                <Marker key={item.id} latitude={item.latLng.lat} longitude={item.latLng.lng}>
                    <PriceMarker
                        amount={amount}
                        isPinned={pinnedStore.isPinned(item.id)}
                        onMouseOver={this.handleMarkerMouseOver}
                        onMouseOut={this.handleMarkerMouseOut}
                        onClick={this.handleMarkerClick}
                    />
                </Marker>
            );
        });
   }

   render() {
          <MapGL
                                ref={this.mapRef}
                                {...viewport}
                                mapStyle="mapbox://styles/mapbox/streets-v8?optimize=true"
                                mapboxApiAccessToken={mapboxgl.accessToken}
                                onViewportChange={this.updateViewport}
                                onLoad={this.onLoad}
           >
                          {this.renderMarkers(results)}
           </MapGL>
}
}

Thank you

Pessimistress commented 5 years ago

shouldComponentUpdate Is not necessary since the viewport change is triggering the rerender during dragging.

I'd recommend caching the result of renderMarkers. It's essentially generating identical nodes on every frame.

thanhphuong612 commented 5 years ago

Thanks @Pessimistress I will cache the result of renderMarkers and see if the performance improves.

adamthewan commented 5 years ago

Did you guys manage to resolve this? I am also seeing drastic performance drops

adechassey commented 5 years ago

Hi all, In order to have better performance, I use geojson layers.

  1. Fetch your data
  2. Build the geojson
  3. Store the data in a source
  4. Inject the layer (which is binded to your source)

Have a look at the Mapbox docs :) and Uber team did a great example here: https://github.com/uber/react-map-gl/tree/master/examples/geojson

thanhphuong612 commented 5 years ago

I cache the result of renderMarkers method. The performance is getting better a lot.

luskin commented 5 years ago

Any more updates on this? We are allergic to frame drop at my company and unfortunately need to display hundreds of complex markers at a time on a map. We have optimized all components and memoized/callback everything we can however we are still seeing pretty bad performance. @thanhphuong612 have you found a way to get 50+ FPS with hundreds of markers?

I can confirm that on pan none of the markers are re-rendering so I'm curious where the lag is stemming from. The react profiler isn't showing much in the way of the performance drops we are noticing.

thanhphuong612 commented 5 years ago

@luskin I am using memo, PureComponent, memoized for all components. I'm displaying 200-600 markers and the performance seems fine, definitely not 50+ FPS but around 20-40+ FPS when zooming/dragging/panning. If you find better solution, let me know please.

luskin commented 5 years ago

@thanhphuong612 Ah alright that is consistent with our experience. We are often rendering upwards of 1000 markers and getting that same FPS range. It doesn't sound like there is anything more we can do to make it more performant.

eeston commented 5 years ago

Glad I stumbled upon this thread. I've having similar issues with lag.

@thanhphuong612 do you mind sharing how you cached the markers without causing a re-render every time the viewport changes?

eeston commented 5 years ago

FYI for anyone else that has a similar issue, I ended up going down the GeoJSON route that @AntoinedeChassey suggested. Everything running smoothly again... :)

raine commented 5 years ago

It does indeed seem that custom layers with GeoJSON source is the way to go for symbols, instead of using the Marker component.

The performance is a lot better, even with just 100 points on the map, and you get hiding of overlapping points for free.

Using a custom SVG as icon image with custom layers can be tricky. I had to use mapRef.current.getMap().addImage('my-icon', svgImageElement) on the onLoad event to add a svg that was used with layout: { 'icon-image': 'my-icon', ... } in the layer specification. (See https://github.com/uber/react-map-gl/issues/770#issuecomment-485772473 and https://github.com/mapbox/mapbox-gl-js/issues/5529).

RobJacobson commented 4 years ago

I'm having the same issue. I have about 60 markers and use a "fly to" animation when my map loads. It's silky-smooth when I remove the markers, but stutters on my notebook when the markers are enabled.

I would like to try the GeoJSON suggestion. However, each of these markers includes an onClick event handler to catch mouse clicks. Is there an away to attach a click handler to individual markers/objects if they're rendered using GeoJSON?

derwaldgeist commented 4 years ago

We are using a lot of markers on the map, and it gets unusable on mobile devices after about 100 of them. Will try the suggestions above. Yet I think this should really be addressed in the library somehow. I'm also missing the ability to cluster markers automatically.

Pessimistress commented 4 years ago

There are several things that will help perf:

On the application side

Apps should attempt to cache the Marker nodes whenever possible. When there are a few hundred markers, operations such as array.map inside render() can become notably expensive. Consider the following implementation:

class Map extends Component {
  state = {
    viewport: {
      latitude: 37.78,
      longitude: -122.41,
      zoom: 8
    },
    markerData: []  // a big array
  };

  _renderMarker(datum) {
    return (
      <Marker key={datum.id} longitude={datum.longitude} latitude={datum.latitude} >
        <img src="pin.png" onClick={() => this._onClick(datum)} />
      </Marker>
    );
  }

  render() {
    return (
      <ReactMapGL {...this.state.viewport} onViewportChange={viewport => this.setState({viewport})}>
        {markerData.map(this._renderMarker)}
      </ReactMapGL>
    );
  }
}

Whenever the user interacts with the map, React will need to 1) construct several hundres of new nodes, 2) compare their props with the previous ones, 3) because the markers' children (<img>) change shallowly, it triggers the rerendering of the entire React tree. All of these are unnecessary since markerData never changed in this process.

(Edited) The following code is considerably faster:

class Markers extends PureComponent {
  render() {
    const {data, onClick} = this.props;
    return data.map(datum => (
      <Marker key={datum.id} longitude={datum.longitude} latitude={datum.latitude} >
        <img src="pin.png" onClick={() => onClick(datum)} />
      </Marker>
    ));
  }
}

class Map extends Component {
  state = {
    viewport: {
      latitude: 37.78,
      longitude: -122.41,
      zoom: 8
    }
  };

  _onClick = (datum) => {
    // do something
  };

  render() {
    return (
      <ReactMapGL {...this.state.viewport} onViewportChange={viewport => this.setState({viewport})}>
        <Markers data={MARKER_DATA} onClick={this._onClick} />
      </ReactMapGL>
    );
  }
}

When the viewport is changed, Marker instances are not regenerated.

On the frameworks side

Following the similar principle, react-map-gl can attempt to cache the node rendered inside the Marker class as well. This should provide further perf improvement, but will only be effective if the first strategy is employed on the application side.

Let me know if you are interested in testing this solution - I can release a react-map-gl beta that contains this optimization.

derwaldgeist commented 4 years ago

Thanks for sharing these insights! Yes, I’d like to try out such a beta version.

RobJacobson commented 4 years ago

Ditto! Would it perhaps be possible for someone to write a page for the docs along the lines of "best practices for performance"? I was quite discouraged when I opened up my beautiful map in Firefox Android and saw it chugging away at just a few frames per second, like the Little Engine that Could.

RobJacobson commented 4 years ago

Could I also request a modified version of the "controls" examples that demonstrates these best practices? The current example has this code in the render function, which seems contrary to your suggestion:

{CITIES.map(this.renderCityMarker)}

I tried implementing your suggestion myself. However, it fails on the following line:

this.state = {
  ...
  markers: CITIES.map(this.renderCityMarker)
};

In a create-react-app implementation, the debugger states that this.renderCityMarker "is not a function." In the original webpack implementation, it just fails with a cryptic error. I'm not quite understanding why this.renderCityMarker isn't visible within the constructor.

derwaldgeist commented 4 years ago

I am trying to implement the performance optimisations based on the suggestions above, e.g. memoization, moving the markers to the state etc.

The problem I am facing with that approach: In our case, the marker set varies dynamically, based on the viewport of the map. If the user zooms out, we retrieve more markers from the database, based on the currently visible bounding box.

This invalidates the markers array, so all markers are re-rendered, even if we apply the techniques above. What would be the best practice for such a scenario?

RobJacobson commented 4 years ago

@derwaldgeist: I had a similar problem. I wanted my markers to have a size based on their zoom level, so they become bigger as the user zooms in.

I solved this by moving the caching logic into the render function. If the zoom level has changed since the last render, it discards the cached markers, creates new ones, and updates state, which triggers a second render. However, if the user is only panning (not zooming), or there's another reason for the render call, then it reuses the old markers. You could use the same logic.

The demonstration code is at the first link, and a hosted version (esp. for testing on mobile devices) is at the second link. This does seem more performant on mobile than before.

https://codesandbox.io/s/modest-allen-8vjw1

http://react-map-marker-test.surge.sh/

@Pessimistress: I noticed that on iOS devices, the markers still lag visibly behind the map when I'm panning the map. Would you have any idea about why this is? Also, would you have any solutions to the apparent problem with calling this._renderMarker inside of state?

derwaldgeist commented 4 years ago

However, if the user is only panning (not zooming), or there's another reason for the render call, then it reuses the old markers. You could use the same logic.

The problem is: I need to update the markers based on panning as well. The idea is: I only retrieve the markers that are within the visible viewport. Once the viewport changes, I want to add new markers dynamically, covering the new region (and maybe hiding markers that are not visible any more). The main reason for this is that I have a lot of these markers, spread across the whole world (they are marking "points of interest").

This was never a problem with Leaflet (which I had used previously), but I can't get this working with react-map-gl in a performant way. Since the markers are contained within a (dynamically updated) array, the whole array is always considered as being touched once I add or remove items.

I considered going the GeoJSON layer route. But if I understand it correctly, it won't allow me to attach click handlers to each of the markers, which I also need.

What I would probably need is a way to programmatically add / delete markers. Or a "clever" caching strategy inside the Map GL framework that detects if a marker stays the same. I tried setting a unique "key" attribute for each of them, but this didn't help.

RobJacobson commented 4 years ago

Ouch. I can see why that is a problem. One option might be to calculate the distance of every marker in pixels from the four sides of the viewport. This can be done using viewport.project(), as shown in this link.

https://uber.github.io/react-map-gl/#/Documentation/advanced/custom-components

Then you could dynamically add a marker when it becomes visible (comes inside the visible window), and vice versa. Alternatively, you could have a timer that resets the marker cache, e.g., every one second, and only adds markers that are within the browser window.

As food for thought, there's an outstanding demo here about using Deck.GL to display hundreds of animated markers simultaneously.

https://reactviz.holiday/webgl-airplanes/

Edit: This Deck.GL example is impressive. It shows hundreds of icons with smooth panning/zooming. It also uses a click handler on the markers.

https://deck.gl/#/examples/core-layers/icon-layer

I would love to hear from @Pessimistress about working with GeoJSON layers. My questions are (1) whether you can attach click handlers, and (2) whether you can attach/show custom HTML (like text) for individual features, instead of just using icons.

Pessimistress commented 4 years ago

@derwaldgeist @RobJacobson

Leaflet/mapbox/deck.gl draw GeoJSON/markers on a canvas, not with DOM elements. It is inherently more expensive to paint a large number of DOM elements, so it's not fair to compare their performance. I would argue that it is unnecessary/unrealistic to render all markers with DOM elements in your use case.

With that said, it's reasonable to expect react-map-gl's <Marker> to have similar perf as mapbox's own Marker control, both of which use DOM. That brings us back to the original issue. Using mapbox with React means that whenever the map is panned/zoomed, on top of mapbox redraw, we also have React updates. Therefore, we have to make the extra effort to write the React component smartly.

Pessimistress commented 4 years ago

Try 5.1.5 with memoized markers

RobJacobson commented 4 years ago

Thank you so much @Pessimistress. I will give these a try.

Pessimistress commented 4 years ago

@RobJacobson @derwaldgeist I have updated the sample code in my previous comment. Essentially you can use a React PureComponent to wrap the markers that do not need to be regenerated on every viewport change. Let me know if the new approach makes more sense.

I will put together a "best practice" article for the documentation website.

Pessimistress commented 4 years ago

Closing for inactivity. See "performance notes" on this page if you are experiencing the same problem.

derwaldgeist commented 4 years ago

I would argue that it is unnecessary/unrealistic to render all markers with DOM elements in your use case.

How is it possible to click on a GeoJSON drawn marker? Would be happy to convert to these kind of markers, if this improves performance. I actually don't care how exactly the markers are drawn, I just need them to be clickable.

I also saw the deck.gl samples. Is it possible to use deck.gl to create markers instead?

Pessimistress commented 4 years ago

Add your geojson layer id to interactiveLayerIds, it will be in the callback parameter of onClick.

RobJacobson commented 4 years ago

Thanks for your help with this, @Pessimistress. I'm going to try the GeoJSON route. I was likewise unclear about how to handle clicks.

Unless I'm mistaken, there aren't any current examples that show how to do this. Once I get this figured out in my head, I'd be happy to create a new example with GeoJSON and submit a pull request.

howdyhyber commented 4 years ago

I have read a Mapbox documentation and it sort of help me regarding this issue:

"You can use style-optimized vector tilesets in Mapbox GL JS by adding ?optimize=true to the end of your style URL:

var map = new mapboxgl.Map({ container: 'map', style: 'mapbox://styles/mapbox/outdoors-v10?optimize=true' // optimize=true });"

https://docs.mapbox.com/help/troubleshooting/mapbox-gl-js-performance/

ghost commented 4 years ago

Thanks for your help with this, @Pessimistress. I'm going to try the GeoJSON route. I was likewise unclear about how to handle clicks.

Unless I'm mistaken, there aren't any current examples that show how to do this. Once I get this figured out in my head, I'd be happy to create a new example with GeoJSON and submit a pull request.

@RobJacobson good news! I was able to follow @Pessimistress suggestion to add the GeoJSON layer ID to interactiveLayerids, then the onClick from ReactMapGL was applied to all my GeoJSON data!! All I had to do was check if the features array had any data; if not, it means the user didn't click on an area where any of my "markers" were. check it out:

let _onClick = (obj) => {
if (obj.features.length !== 0) {
//do  something, user clicked on your geojson data
}
}
<ReactMapGL 
...
interactiveLayerIds={["buildings"]}
onClick={_onClick}
>
<Source type="geojson" data={geoJsonData}> 
<Layer id="buildings" ....>
</Source>
</ReactMapGL>
Marius-Adam commented 4 years ago

After being unsuccessful in trying to converting the data to geoJSON I implemented a debouncer on viewport change and seems to work quite okay. Here is my app (still working on it) https://stoic-carson-b251bf.netlify.app/

ajmeraaxesh commented 4 years ago

@Marius-Adam can you share your code or a simpler example. on how you implemented debouncer.

ajmeraaxesh commented 4 years ago

Found another solution like suggested in their API reference docs and @Pessimistress Docs: http://visgl.github.io/react-map-gl/docs/api-reference/marker

Instead of purecomponent as shown in docs above I use React.memo

const Markers = React.memo(({markers})=> {
  return (
     <React.Fragment>
        {
            markers.map((marker)=> {
                <Your code to render marker>
            })
        }
    </React.Fragment>
  )  
})

<ReactMapGl ...>
    <Markers markers={<some markerdata>} />
</ReactMapGl>
shaythuram commented 4 years ago

Thanks for sharing these insights! Yes, I’d like to try out such a beta version.

Hi there i was scrolling through and found ehr comment very helpful, what may help further is the link below, from the official docs.

https://visgl.github.io/react-map-gl/docs/api-reference/marker

ghost commented 2 years ago

I am trying the GeoJSON route. How would I render a different marker depending on a property is has?

happycoder0011 commented 6 months ago

I found an alternate solution. Th idea is to load only the markers that are present in the viewport at any given map onMove action. Adding a condition to load markers only if less than 400 features are in the viewport. (This (400 count) worked for me as it was not lagging). Let's say I want to get features in the "blocks" layer that is present in the viewport and then add markers on zooming in.

<Map ref={mapRef} onMove={(e: any) => { const featuresInViewport = mapRef?.current?.getMap().queryRenderedFeatures({ layers: ['blocks'] }); if(featuresInViewport?.length < 400) setMarkerPropertyData(featuresInViewport) }}

{. markerPropertyData.length > 0 && <Marker. {...props} /> }

This worked like charm. Keep dragging on the map and the current 400 visible markers are loaded in the DOM. Was able to handle 40,000 markers using this.

Hope it helps!!

Nikoru14 commented 5 months ago

can anyone help me optimize my map its laggy cause it has 700+ markers use memo doesn;t eliminate lag

const markers = useMemo(() => !showHeatmap && filteredPins.map((pin, index) => (

handleMarkerClick(event, pin)} style={{ cursor: 'pointer' }}>
    )), [filteredPins, showHeatmap]);