Skip to content

feat(mapbox): Replace shadow transform with proxied approach #2514

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

Merged
merged 6 commits into from
Mar 28, 2025

Conversation

Pessimistress
Copy link
Collaborator

@Pessimistress Pessimistress commented Mar 28, 2025

This is a pretty big change to how we handle controlled Mapbox instance.

Background

The basis of our approach is that we keep two Transform instances: one that Map can mutate freely, and one that is controlled by React props.

In 7.x and 8.0, we created a renderTransform as the controlled instance. The mutable instance is the permanent source of truth of the map state. Its changes are merged with React props and synchronized back to renderTransform. During 1. render, 2. query and 3. event callbacks, renderTransform is swapped in so the map reads the controlled matrices.

We have made a lot of hacks and patches around this approach, and there still are a handful of long-standing issues, mostly around terrain and non-mercator projection. The source of the grievance is that

  1. Mapbox can mutate map.transform on various lifecycle stages and some of them do not trigger an event.
  2. Transform cannot be deterministically reproduced by its public methods and from a ViewState object. Terrain mutates unexposed private fields that affect matrix calculation ([Bug] map.flyTo centers on the incorrect spot when terrain is enabled. #2194, [Bug] When 3D Terrain is enabled, updating state in React causes Markers to render in their original locations, instead of on top of the terrain. #2088, Pointer event lat lng position doesn't factor in terrain #2248). New features can also fail to be synchronized ([Bug] v8.x does not display Mapbox road labels when using Standard Styles #2506).

Changes

This PR proposes a (hopefully) cleaner approach. We replace map.transform with a ProxyTransform, which uses Proxy to trap get and set operations. Under the hood, it manages the two Transform instances (freely mutable proposedTransform and React-controlled controlledTransform). Of the two, controlledTransform is the permanent source of truth, and proposedTransform is only temporarily populated IF the map is controlled AND during a move session. This results in much better consistency in map behavior:

  1. In the uncontrolled use case (using initialViewState) there is only one transform instance, which should minimize the perf/bugs impact from our wrapper
  2. No more instance swapping, which should eliminate the discrepancy in behavior between GL renderer, DOM controls and query.

I am also trying to document inline extensively to improve code readability.

Release plan

Targeting a v8.1 release.
I am leaning towards NOT back porting this to mapbox-legacy, which is mainly used for mapbox-gl@1 and will be eventually deprecated. The old approach works fairly well for the basic (no terrain or alt projection) use case. The new approach relies heavily on the internals of the Transform class and the official types. I did a quick audit and most of the private methods have existed since v2.0. However we do not test with non-latest mapbox versions and it could become a maintenance hazard.

Copy link
Contributor

@chrisgervang chrisgervang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this! I think mapbox users will be happy to have terrain supported. The proxy approach looks a lot more maintainable as well.. much better than needing to reserve engineer all of the ways the camera gets transformed!

At another time, would it be possible to we populate elevation in maplibre's viewState as well?

Also, do you want to add elevation to the docs?

@Pessimistress Pessimistress merged commit 93f0ba7 into master Mar 28, 2025
1 check passed
@Pessimistress Pessimistress deleted the x/proxy branch March 28, 2025 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants