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

Maplibre compatibility / transition #14

Open
tmcw opened this issue Jan 19, 2024 · 5 comments
Open

Maplibre compatibility / transition #14

tmcw opened this issue Jan 19, 2024 · 5 comments

Comments

@tmcw
Copy link
Contributor

tmcw commented Jan 19, 2024

Right now, Placemark uses a combination of Mapbox GL JS and Deck.gl. I think this could use some freshening-up:

  • Maplibre and Protomaps are probably the best combination for an open source project, since Maplibre is fully OSS.
  • Deck.gl has been causing bugs in the app for a while and never got transitioned-to for anything more than rendering vertices. Moving away from Deck.gl would probably make sense.

The dream scenario would be implementing a diffing update pattern in Maplibre that was super efficient.

@thadk
Copy link

thadk commented Jan 23, 2024

Most of the code I've encountered has been pretty intuitive but this part has been less so:

const CIRCLE_LAYOUT: mapboxgl.CircleLayout = {};
export const FEATURES_SOURCE_NAME = "features";
export const EPHEMERAL_SOURCE_NAME = "ephemeral";
export const EPHEMERAL_LINE_LAYER_NAME = "ephemeral-line";
export const EPHEMERAL_FILL_LAYER_NAME = "ephemeral-fill";
export const FEATURES_POINT_HALO_LAYER_NAME = "features-symbol-halo";
export const FEATURES_POINT_LAYER_NAME = "features-symbol";
export const FEATURES_POINT_LABEL_LAYER_NAME = "features-point-label";
export const FEATURES_FILL_LABEL_LAYER_NAME = "features-fill-label";
export const FEATURES_LINE_LABEL_LAYER_NAME = "features-line-label";
export const FEATURES_LINE = "features-label";
export const CURSORS_POINT_LAYER_NAME = "cursors-symbol";
export const FEATURES_LINE_LAYER_NAME = "features-line";
export const FEATURES_FILL_LAYER_NAME = "features-fill";

This is a bit off topic, but for one, this whole block is erroring a bit when I tried to upgrade to Mapbox v3.1 (see devtools screenshot). But more broadly, just curious about it. And I wonder if it'd be affected by a Maplibre switchover.
Is this the mechanism that you referred to in your video as a "framework on top of Mapbox GL that supports selecting and editing multiple shapes at a time"? Basically, I couldn't find either end of the constants referenced in Mapbox or elsewhere in the codebase.

I also tried briefly to excise DeckGL as you suggested above. It isn't a huge amount of code. But it seems like it might be helpful to time travel in commits back to your prior Mapbox GL only implementation if someone actually wanted to do this. I'm not sure I want to remove it yet, myself.

@tordans
Copy link

tordans commented Jan 23, 2024

Just a quick note and maybe off topic, since I did not look at the code, yet: I am using https://github.com/visgl/react-map-gl for all my React+Maplibre Projects and I am very happy with the way one can structure sources, layers and styles. This might be a good librate to transition to. I find it a lot easier to work with than plain Maplibre in React.

@tmcw
Copy link
Contributor Author

tmcw commented Jan 23, 2024

Here are the two diffs from the commits that introduced Deck.gl: https://gist.github.com/tmcw/ab220612e3789ab9e776e0815654a170 - not sure if they'll reverse cleanly, but they should at least give an idea of the code before/after.

So this code, which is creating the style, some notes off the top of my head:

load_and_augment_style is there because Placemark supports multiple "base styles" but it needs to add additional style layers to represent both the "real" data layer as well as "ephemeral" layers, on top of the map.

Doing these style modifications entirely through the Mapbox GL JS API turned out to be pretty complicated and error-prone - so instead this does the modifications to the style JSON, and sets the style in one pass, so we don't need to add/create layers via Mapbox GL.

And, yeah, the layers themselves are fairly complicated - this is trying to optimize for performance as well as get good rendering so:

There are two sources

  1. "features", which contains the geojson feature representations
  2. "ephemeral", which contains, currently, features that are being edited. This is basically a 'hot'/'cold' system that's trying to get Mapbox GL to re-render as little as possible, because it doesn't/didn't support smart diffing. I think Maplibre does support diffing now, so maybe that's the future!

ephemeral used to, as you can see in the diffs above, contain the vertexes and the lasso data, but those were both replaced by Deck.gl representations.

Notes:

  • CURSORS_POINT_LAYER_NAME is unused - I replaced this with DOM cursors, I think, so it can be removed.
  • FEATURES_LINE is unused
  • The features source contains feature representations, but not the raw features: if you look at splitFeatureGroups, there's a bunch of code that basically strips properties, adds ids, and more. Mapbox GL JS will send all the feature data across postMessage, so features with lots of properties would be slow, even if those properties aren't used for any data-driven styles. So there's some code to strip properties, omit hidden features, and, in that function, also split features between the "hot" features to be rendered in "ephemeral" versus the "cold" features to be rendered in "features", and to generate the vertexes that are in "synthetic".

The issue that you're seeing… sometimes that crops in Placemark itself as a race condition between map handlers and the style updating - hovering over the map looks for a feature in a layer that doesn't exist yet - but maybe there's a change to GL 3.x that also changes how getting layers works?

@stevage
Copy link
Contributor

stevage commented Feb 11, 2024

Not sure if this is helpful, but there is a whole different way of solving this problem: two Map-*-gl instances sharing the same screen real estate. The bottom one contains the main map data, the top one is just used for drawing. They are synchronised, obviously, when you drag and pan etc.

@tmcw
Copy link
Contributor Author

tmcw commented Feb 18, 2024

Yeah, that's certainly an option - I can't give current estimates because I haven't tried it in a bit, but I think the perf was a bit slower with that approach. But open to whatever options, especially if folks are contributing PRs for them!

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

No branches or pull requests

4 participants