visgl / react-map-gl

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

[Bug] fork MapLibre terrain 3D jitter up & down as we pan #2211

Open jawensi opened 1 year ago

jawensi commented 1 year ago

Description

When I install MapLibre fork and load a 3D terrain, there is an effect in the navigation. When you make pan with the mouse there is a change in the zoom level. This doesn't happen when you rotate the view.

https://github.com/visgl/react-map-gl/assets/6216293/f5bc2d1d-f74b-4046-8eae-22abf1666251

Expected Behavior

The expected behaviour is to keep the zoom level constant, avoiding this effect. When is used a MapBox fork, the jitter effect doesn't happen.

Steps to Reproduce

import maplibregl from "maplibre-gl";
import "maplibre-gl/dist/maplibre-gl.css";
import Map, { Source } from "react-map-gl";

export const MyMap = () => {

  return (
    <div style={{ height: "100%", width: "100%" }}>
      <Map
        id="map"
        initialViewState={{
          longitude: 11,
          latitude: 49,
          zoom: 4,
        }}
        mapLib={maplibregl}
        minZoom={0}
        maxZoom={22}
        mapStyle={
          "https://api.maptiler.com/maps/streets/style.json?key=get_your_own_OpIi9ZULNHzrESv6T2vL"
        }
        terrain={{ exaggeration: 1.3, source: "terrainLayer" }}
      >
        <Source
          id="terrainLayer"
          type="raster-dem"
          tiles={[
            "https://wms.wheregroup.com/dem_tileserver/raster_dem/{z}/{x}/{y}.webp",
          ]}
          tileSize={256}
        />
      </Map>
    </div>
  );
};

Environment

Logs

No response

Pessimistress commented 1 year ago

The same behaviour when is used a MapBox fork

Not sure what "fork" you are talking about. If you mean Mapbox, this example is built with the latest mapbox-gl and works fine: https://visgl.github.io/react-map-gl/examples/terrain

jawensi commented 1 year ago

The bug only appears when you use MapLibre, adding the property 'mapLib' to the Map component and import MapLibre

mapLib={maplibregl}

mszekiel commented 11 months ago

It updates the camera elevation based on the terrain mesh. For version 3.1.0 I can see it's following the terrain during panning, but for latest 3.3.1 it updates the camera position only after panning is finished (idle event I guess). Problem doesn't exists when using mapbox, only when using maptiler service. I guess it is maplibre bug? But problem doesn't exist in maplibre JS examples with terrain.

pviejo commented 8 months ago

Any update on this?

gauthierelcapitan commented 7 months ago

Same behavior here, you guys have any workaround ? The bug is on MapLibre side any issue already exists ?

lymperis-e commented 4 months ago

Same behavior here, you guys have any workaround ? The bug is on MapLibre side any issue already exists ?

I believe the issue is with react-map-gl rather than maplibre. I 've been looking into it (I admit that I am not very familiar with the internals of either project, although I use maplibre a lot), and I believe the issue has to do with the way react-map-gl applies the ViewState changes.

The implementation is based on mapbox's Transform class, which is significantly different from maplibre's . My understanding is that react-map-gl watches for both user and map changes to the view state, and tries to reconcile the two and apply them smoothly, similar to map.jumpTo, as per the author. This is method that implements that:

src/mapbox/mapbox.ts

  // Adapted from map.jumpTo
  /* Update camera to match props
     @param {object} nextProps
     @param {bool} triggerEvents - should fire camera events
     @returns {bool} true if anything is changed
   */
  _updateViewState(nextProps: MapboxProps<StyleT>, triggerEvents: boolean): boolean {
    if (this._internalUpdate) {
      return false;
    }
    const map = this._map;

    const tr = this._renderTransform;
    // Take a snapshot of the transform before mutation
    const {zoom, pitch, bearing} = tr;
    const isMoving = map.isMoving();

    if (isMoving) {
      // All movement of the camera is done relative to the sea level
      tr.cameraElevationReference = 'sea';
    }
    const changed = applyViewStateToTransform(tr, {
      ...transformToViewState(map.transform),
      ...nextProps
    });
    if (isMoving) {
      // Reset camera reference
      tr.cameraElevationReference = 'ground';
    }

    if (changed && triggerEvents) {
      const deferredEvents = this._deferredEvents;
      // Delay DOM control updates to the next render cycle
      deferredEvents.move = true;
      deferredEvents.zoom ||= zoom !== tr.zoom;
      deferredEvents.rotate ||= bearing !== tr.bearing;
      deferredEvents.pitch ||= pitch !== tr.pitch;
    }

    // Avoid manipulating the real transform when interaction/animation is ongoing
    // as it would interfere with Mapbox's handlers
    if (!isMoving) {
      applyViewStateToTransform(map.transform, nextProps);
    }

    return changed;
  }

So when panning, the wrapper effectively instructs the underlying library to set a new view state (including a new lon/lat). Mapbox's Transform has the concept of transform.cameraElevationReference = 'sea' | 'ground. Maplibre on the other hand does not. It has the attribute transform.minElevationForCurrentTile, which queries the underlying terrain tile for its elevation. This attribute is updated with the _updateElevation method of maplibre's Camera class, as seen below:

maplibre-gl-js/src/ui/camera.ts

    _updateElevation(k: number) {
        this.transform.minElevationForCurrentTile = this.terrain.getMinTileElevationForLngLatZoom(this._elevationCenter, this.transform.tileZoom);
        const elevation = this.terrain.getElevationForLngLatZoom(this._elevationCenter, this.transform.tileZoom);
        // target terrain updated during flight, slowly move camera to new height
        if (k < 1 && elevation !== this._elevationTarget) {
            const pitch1 = this._elevationTarget - this._elevationStart;
            const pitch2 = (elevation - (pitch1 * k + this._elevationStart)) / (1 - k);
            this._elevationStart += k * (pitch1 - pitch2);
            this._elevationTarget = elevation;
        }
        this.transform.elevation = interpolates.number(this._elevationStart, this._elevationTarget, k);
    }

Now, what I assumed based on the above, is that when maplibre is imperatively instructed to update the ViewState, it recalculates the camera height, based on the new central lat/lng tile's elevation (this._elevationCenter)


Adding a console log, and destructuring elevation from tr, I observed that indeed when panning, the transform returns different elevations, whereas when rotating it returns the same elevation. The map.transform.elevation property, stays the same.

src/mapbox/mapbox.ts

    // Take a snapshot of the transform before mutation
    const {zoom, pitch, bearing, elevation} = tr;
    const isMoving = map.isMoving();

    console.log(
      `map camera elevation: ${map.transform.elevation}, elevation: ${elevation}`
    );

image

Workaround (possibly)

A possible workaround would thus be to capture the map.transform.elevation, and enforce it on the updated view state, instead of using the tr.elevation value. I have not quite figured out how the transform is applied to produce the next view state, although I tried several things, like the example below. Any maintainer help would be more than welcome!

const changed = applyViewStateToTransform(tr, {
      ...transformToViewState(map.transform),
      ...nextProps,
      elevation: map.transform.elevation
    });
lymperis-e commented 2 months ago

Any update on this?

ggaans commented 2 months ago

Any progress or likely hood of a fix?

Fabioni commented 2 months ago

I just started using this library an hour ago and already ran into this problem. What is the status?

Fabioni commented 1 month ago

@Pessimistress whats the status on this? This is a major flaw in the that library. 🙈