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

Add oneway arrows to remaining highway classes #512

Merged
merged 30 commits into from
Jun 23, 2023
Merged

Conversation

claysmalley
Copy link
Member

Adds oneway arrows to non-motorway highway classes, with half the layers.

Screenshot from 2022-07-24 19-39-28

@ZeLonewolf
Copy link
Member

I honestly didn't even realize this was missing.

@ZeLonewolf
Copy link
Member

It appears that this change also unifies the link and regular versions of each road class, is that also a deliberate consolidation?

@ZeLonewolf
Copy link
Member

#513 would be helpful in assessing changes in layer count.

@claysmalley
Copy link
Member Author

It appears that this change also unifies the link and regular versions of each road class, is that also a deliberate consolidation?

Yes - were they separate layers intentionally?

@ZeLonewolf
Copy link
Member

Yes - were they separate layers intentionally?

Only in the sense that I (a) didn't really know what I was doing and (b) didn't realize layer count had a performance implication.

@ZeLonewolf
Copy link
Member

The changes appear to be working from visual inspection.

The arrows on motorway_link are hard to see and we may want to open a separate issue to track it, as reducing layer count is a clear improvement by itself.

image

@claysmalley
Copy link
Member Author

Adding oneway arrows seems to cause a significant slowdown at very high zoom, so it's hidden at ≥z20.

@ZeLonewolf
Copy link
Member

Adding oneway arrows seems to cause a significant slowdown at very high zoom

This sounds dodgy.

@jleedev
Copy link
Member

jleedev commented Jul 25, 2022

Something is extremely slow at high zooms already.

@claysmalley
Copy link
Member Author

How about white arrows for motorways and trunks?

Screenshot from 2022-07-24 22-41-24

@claysmalley
Copy link
Member Author

On second thought, white is even less legible against the red on trunk_link. Changing trunk oneway arrows back to black.

Screenshot from 2022-07-24 22-45-36

@ZeLonewolf
Copy link
Member

There is an artifact in cases of a link road:

image
Location (localhost link)

Is this resolvable with a change in ordering or unavoidable? Is there a way to prevent the arrows from being drawn too close to the ends of a road?

@claysmalley
Copy link
Member Author

claysmalley commented Jul 31, 2022

We would have to sandwich oneway arrows in between two road fill layers, which would hinder our ability to refactor roads to use more expressions and fewer layers. I don't think there's an easy solution to this, other than maybe lightening the motorway fill to accommodate a dark oneway arrow.

@1ec5
Copy link
Member

1ec5 commented Jul 31, 2022

Is there a way to prevent the arrows from being drawn too close to the ends of a road?

It is possible to use invisible spacer symbols to collide out any arrows that would be drawn at the endpoints, similar to how we’re pushing waterway labels away from bridges. In this case, the symbol’s symbol-placement property would be set to point so that it gets placed at one of the endpoints instead of repeated along the line. We’d need to pay attention to the spacing settings so that shields and road names don’t become less dense because of collisions suddenly occurring at junctions – or junctions on nearby roadways. Also, openmaptiles/planetiler-openmaptiles#10 proposes to fix wrong-way one-way arrows by disabling line merging of one-way lines; with that stopgap fix in place, the spacers would frequently prevent any one-way arrows or shields from showing up at all on one-way streets.

/ref mapbox/mapbox-gl-js#4003

@1ec5
Copy link
Member

1ec5 commented Jul 31, 2022

One-way arrows only come in at z15 for freeways. At this zoom level, we’re already starting to show surface streets in the “cased” style. I think this gives us some flexibility to deemphasize color distinctions based on road classification. It’s also at a high enough zoom level that there’s a less ambiguity as to which ramp goes in which direction, so maybe the ramp arrows aren’t essential.

/ref #486

@adamfranco
Copy link
Collaborator

Hmm... Without arrows on link roads, figuring out whether a ramp is an on or off requires the eye tracing it up to the lanes it diverges from. I imagine that there are a few cases of stacked interchanges where that becomes really difficult to tease out visually. (for example, the GW bridge)

@github-actions
Copy link

PR Preview:

Sprite Sheets:

Sprites 1x
Sprites 2x

@claysmalley claysmalley marked this pull request as ready for review June 18, 2023 22:08
@ZeLonewolf
Copy link
Member

ZeLonewolf commented Jun 19, 2023

These looks pretty good for all but the lowest highway classes.

For the tiniest roads (alleys and so forth), I'm not sure if I'm seeing arrows or if there's a speck on my laptop screen that needs to be cleaned. Not sure if there's a good solution to this. Perhaps a hard minimum size to the arrow and let it overflow a bit?

Location:
image

@claysmalley
Copy link
Member Author

This better?

image

@ZeLonewolf
Copy link
Member

This looks good!

I note that bridges are not included (example), however, bridges appear to be not supported presently. Is that intended to be part of this PR or left as future work?

@claysmalley
Copy link
Member Author

I note that bridges are not included (example), however, bridges appear to be not supported presently. Is that intended to be part of this PR or left as future work?

That's a mistake. They're in the list of layers, so I don't know why they aren't showing up.

@ZeLonewolf
Copy link
Member

At a glance, it appears that one-way arrows are below bridges in the layer stack.

@claysmalley
Copy link
Member Author

claysmalley commented Jun 20, 2023

Figured it out. The culprit was the invisible spacer label that prevents waterway labels from colliding with bridges. This spacer is now disabled if the bridge is tagged oneway=yes.

Example location

image

Copy link
Member

@ZeLonewolf ZeLonewolf left a comment

Choose a reason for hiding this comment

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

Good work. Thanks for sticking it out!
This would be a great use case for #881 if we're not in a rush to push the change.

@jleedev
Copy link
Member

jleedev commented Jun 21, 2023

The arrows seem to serve the spacers' purpose except at really high zooms.

Screenshot_20230621-123530

@claysmalley
Copy link
Member Author

The arrows seem to serve the spacers' purpose except at really high zooms.

Before removing the spacer, oneway arrows would only show up at very high zooms. Something weird is going on and it's cooking my CPU.

@claysmalley claysmalley merged commit 4a84f51 into main Jun 23, 2023
@claysmalley claysmalley deleted the clay-oneway branch June 23, 2023 16:07
@jleedev
Copy link
Member

jleedev commented Jun 23, 2023

Ah, it might be that some combination of spacers + oneway arrows + lettering causes this.

I still also hypothesize that it's overzooming the z14 tile to z22 and then stuffing it full of symbols, i.e. if higher zoom tiles existed that were simply slices of the full ones then it would attempt to do less work.

Might also be that the waterway lettering is attempting to overlap itself severely, could see if the slowdown is localized depending on the presence of waterways or oneways.

https://osmus.slack.com/archives/C01V02K52UX/p1658777024337899

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants