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

Centralize layer z-ordering. #166

Merged
merged 1 commit into from
May 16, 2023
Merged

Centralize layer z-ordering. #166

merged 1 commit into from
May 16, 2023

Conversation

dabreegster
Copy link
Contributor

@dabreegster dabreegster commented May 15, 2023

Before, when we had points, lines, and polygons overlapping, the z-ordering was not great -- you couldn't really select the smaller object:
Screenshot from 2023-05-15 13-35-34

This PR defines an ordering for all of the layers in ATIP, so we can explicitly control everything from one place. The behavior is nicer now:

screencast.mp4

As background for why getting z-ordering between different layers in MapLibre is tough, see mapbox/mapbox-gl-js#7016.

Next steps

I originally wanted to also make sure that we don't cover up road names: #102. We now draw beneath the road labels, but because they're faint and black in the default style, it's... still pretty useless against the blue routes:
Screenshot from 2023-05-15 13-38-52
If anyone has color or symbolizing suggestions either for the colors we're using, or to change the road labels, it's easy to try things out!

beforeId: string | undefined = undefined
) {
// The layer.id here MUST be present in layerZorder.
export function overwriteLayer(map: Map, layer) {
if (map.getLayer(layer.id)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Starting a slightly orthogonal conversation here: using these two overwrite methods is kind of gross; see https://github.com/acteng/atip#hmr-best-practices for why. I see people using Svelte components to specify GeoJSON sources and layers in https://svelte-maplibre.vercel.app/examples/geojson_polygon, https://www.npmjs.com/package/@mapcomponents/react-maplibre, etc. https://imfeld.dev/writing/svelte_domless_components and other articles on the same blog explain the reasoning here. I think we should consider switching to this style too.

Also, if you edit this file and let Vite auto-reload, you'll see it crash! You have to hard refresh. It's because while some component gets recreated the maplibre Map is null, and we try to addSource. We need to figure out how to robustly add stuff to the map, or defer it to when the map is loaded, and handle all the weird Svelte component orders

@dabreegster
Copy link
Contributor Author

Also CC @Pete-York. Robin, Pete's not in the GH team member list yet, so I can't add as a reviewer

@robinlovelace-ate
Copy link
Contributor

If anyone has color or symbolizing suggestions either for the colors we're using, or to change the road labels, it's easy to try things out!

I think given the current way of doing it, and without complexity of rendering our own road names (v. low priority, probably not in scope), it seems like a balance between line opacity and seeing road name. For me, given that this is aimed at transport planning professionals who know their areas well, I would favour visibility of ATIP layers, I think we have that now.

Copy link
Contributor

@robinlovelace-ate robinlovelace-ate left a comment

Choose a reason for hiding this comment

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

👍 based on the video this is clearly a major improvement in cases where there are overlapping area and line interventions, which ATE should be encouraging. Area-wide traffic management in and new routes are mutually beneficial.

@dabreegster
Copy link
Contributor Author

it seems like a balance between line opacity and seeing road name

Or we just pick a route color that contrasts better with the default black... yellow looks fine, but is too close to the basemap style for main roads:
Screenshot from 2023-05-15 22-00-37

@robinlovelace-ate
Copy link
Contributor

Lighter shade of blue?

@dabreegster
Copy link
Contributor Author

Lighter shade of blue?

Screenshot from 2023-05-15 22-05-46
Yeah actually this looks pretty nice to me. I don't remember if the 4 colors originally came from some kind of palette that's important to keep as "qualitative / discrete", so maybe we ought to adjust everything. This is #BCF5F9, set in colors.ts. I'll wait for Alex's and Pete's opinion, but otherwise I'd be happy switching to this as an incremental improvement

@robinlovelace-ate
Copy link
Contributor

I'd be happy switching to this as an incremental improvement

Same, although my one comment would be maybe a touch darker would be better, that is a super light blue.

@dabreegster
Copy link
Contributor Author

Same, although my one comment would be maybe a touch darker would be better, that is a super light blue.

Any recommendations specifically? You can edit route in colors.ts and live-reload if you npm run dev

@robinlovelace-ate
Copy link
Contributor

Same, although my one comment would be maybe a touch darker would be better, that is a super light blue.

Any recommendations specifically? You can edit route in colors.ts and live-reload if you npm run dev

Will wait for there to be a devcontainer as working on work laptop atm...

@dabreegster dabreegster requested a review from Pete-Y-CS May 16, 2023 10:36
@dabreegster dabreegster merged commit 41be5db into main May 16, 2023
@dabreegster dabreegster deleted the zorder branch May 16, 2023 14:23
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