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 shields for Nova Scotia scenic drives #912

Merged
merged 25 commits into from
Apr 18, 2024

Conversation

quincylvania
Copy link
Contributor

Closes #911.

Creating a draft for sprite design feedback. Waiting for a tile refresh to review how these look on the map.

@@ -4201,6 +4204,33 @@ export function loadShields() {

// Ref-specific cases. Each entry should be documented in CONTRIBUTE.md

shields["CA:NS:S"].overrideByRef = {
BdOLSD: {
Copy link
Member

Choose a reason for hiding this comment

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

Do people or publications normally refer to the routes by these initialisms? If so, let’s add an explanation to CONTRIBUTING.md, in the section where we justify other unsignposted refs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, I just made up the initialisms so we'd have something to key off of. I don't think we can key off the route name because I thiiiiink the name on the tile comes only from the way itself. The Merritt Parkway is the only one we do like that so far and it happens to have all way names match the parkway relation name.

This situation is kind of like the Great Lakes Circle Tour routes, but with a more items and less documentation. I imagine the problem will come up more often as we add additional networks of scenic routes.

I can add documentation for this.

Copy link
Member

@1ec5 1ec5 Aug 4, 2023

Choose a reason for hiding this comment

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

For #451, the New Zealand community apparently decided to give each route a distinct network subvalue in order to avoid polluting ref with unsignposted initialisms. However, this fragmentation makes less sense when the shield designs all follow a common motif, as in this case.

I think inevitably we’ll need the tiles to contain something beyond the network=ref syntax in route_* properties. If we could go back and change the format, I would’ve suggested something like route:2:network or route_network_2, which would’ve also logically allowed for route_name_2 or even route_wikidata_2 for these tricky cases.

In the meantime, unsignposted initialisms are unfortunately a fact of life in refs in many parts of the world. If the local mappers are OK with it, then we can go along with it for now.

Copy link
Member

Choose a reason for hiding this comment

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

This situation is kind of like the Great Lakes Circle Tour routes, but with a more items and less documentation.

The Great Lakes Circle Tour routes are known by the initialisms in ref, at least informally, so it isn’t a big deal if a non-shield-capable renderer displays the ref verbatim. I’d still reach out to the local community to get their opinion on this use of ref, in case there are more established initialisms for the routes.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the choice of ref on bespoke tourist routes, don't we have overrideByName as an option? Then we would be tied to the name, which should be beyond dispute. In any case, the choice of ref value on the route relation of an obscure tourist route seems like something we can just make up using reasonable conventions and change later if the community changes their mind down the line. In short, I'd favor pushing this forward either as-is or with a name-based definition.

@ZeLonewolf overrideByName was introduced for the Kentucky parkways. I agree that it would be an ideal solution… except that the name is just the way name, not the route relation’s name. For tourist routes, the road name isn’t reliably the route relation’s name, although it is in the case of the Cabot Trail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, here's an illustrative example. Names like "Lighthouse Route" don't always appear in the data, so we're left to rely on a ref at the moment.

Screenshot 2023-08-14 at 1 54 06 PM

Copy link
Member

Choose a reason for hiding this comment

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

Yep, makes sense to me - thanks for refreshing how that works. I built the damn logic for those route_X attributes and I can't even keep it straight...

Copy link
Member

Choose a reason for hiding this comment

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

Let’s ticket out something about a route_#_name scheme in OpenMapTiles. Something similar would also help with Spain’s per-route colors in #654.

Copy link
Member

Choose a reason for hiding this comment

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

As of #1032, each feature has a route_#_name property based on the route relation. The name gets plumbed through to the shield selection code, so you should be able to specify overrideByName without worrying about the roadway carrying a different name, and the relations no longer need to be tagged with initialisms as refs.

route_1_network: CA:NS:R; route_1_ref: 322; route_1_name: Route 322; route_2_network: CA:NS:S; route_2_name: Lighthouse Route

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if a few of these shields would be more legible if we don’t need to offset them to the left but can instead have them take up more space to the top and right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we could make the pictures slightly bigger by centering then and moving the water down. But personally I think I prefer sticking closer to the source designs. Though maybe seeing them on the map will affect this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we’ll see how it turns out on the map. For the Ohio River Scenic Byway in #893, I made the shield 20 pixels wide but taller than 20 pixels, avoiding whitespace on either side, in order to maximize room for the shield’s details.

Copy link
Member

Choose a reason for hiding this comment

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

The current shield for network=CA:NS:H is only 20px tall because we made it before relaxing the height guidelines for narrow shields. I think the designs introduced in this PR are good for now. If we want to enlarge them later, along with the Arterial shield, let's put that in a new issue.

@ZeLonewolf
Copy link
Member

Tiles are updated!

@quincylvania
Copy link
Contributor Author

In terms of size, I think the original 16x20 looks okay, perhaps too small:

Screenshot 2023-08-07 at 2 48 17 PM
Screenshot 2023-08-07 at 2 49 28 PM

I tried 18x22 to keep even numbers and it seems a bit too big:

Screenshot 2023-08-07 at 2 53 26 PM
Screenshot 2023-08-07 at 2 53 38 PM

Ultimately I'd say 17x21 is a good compromise size as long as it doesn't cause rounding/aliasing issues.

Screenshot 2023-08-07 at 3 09 53 PM

@ZeLonewolf
Copy link
Member

Either the 17px or the 18px versions look pretty good to me in the screen shots. I agree that the 16px one seems slightly undersized.

@quincylvania quincylvania marked this pull request as ready for review August 7, 2023 21:23
@quincylvania
Copy link
Contributor Author

I standardized on 17x21 px shields. I couldn't find any signage for the Fundy Shore Scenic Drive but otherwise this is complete and ready for review. FYI there was a ref mistake for the Sunrise Trail so that shield won't show up until a new tile update goes through but you can see it in the spritesheet.

Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

These icons seem to hew to an 8-bit style, which happens to look great for these particular shields. I wonder if we should aim for a similar style in other shields, where we’re currently going for a very literal, realistic look and feel.

@@ -4201,6 +4204,33 @@ export function loadShields() {

// Ref-specific cases. Each entry should be documented in CONTRIBUTE.md

shields["CA:NS:S"].overrideByRef = {
BdOLSD: {
Copy link
Member

Choose a reason for hiding this comment

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

This situation is kind of like the Great Lakes Circle Tour routes, but with a more items and less documentation.

The Great Lakes Circle Tour routes are known by the initialisms in ref, at least informally, so it isn’t a big deal if a non-shield-capable renderer displays the ref verbatim. I’d still reach out to the local community to get their opinion on this use of ref, in case there are more established initialisms for the routes.

@ZeLonewolf
Copy link
Member

Regarding the choice of ref on bespoke tourist routes, don't we have overrideByName as an option? Then we would be tied to the name, which should be beyond dispute. In any case, the choice of ref value on the route relation of an obscure tourist route seems like something we can just make up using reasonable conventions and change later if the community changes their mind down the line. In short, I'd favor pushing this forward either as-is or with a name-based definition.

@quincylvania
Copy link
Contributor Author

This is ready for review btw. The discussion kind of got lost in the code comments but I think everything's been addressed.

src/js/shield_defs.js Outdated Show resolved Hide resolved
spriteBlank: "shield_ca_ns_s_st",
},
};

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FSSD is documented on Wikipedia but I couldn't find any signage for it.

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.

Looks good pending the fix for FDLT. We can either add FSSD here or in a separate ticket.

@zekefarwell
Copy link
Collaborator

zekefarwell commented Sep 8, 2023

So if I understand correctly ref=BdOLSD on this route https://www.openstreetmap.org/relation/11051558 isn't a real identifier, but just made up to work around the fact that no other field from a route relation is available in OpenMapTiles? I wouldn't call myself a ground truth verifiability purist, but if that's the case this feels like a real stretch. Seems to me like it would be better to work on improving OpenMapTiles with the desire for these type of scenic route shields as motivation. I doubt these will be the last scenic routes folks will want to add.

@quincylvania
Copy link
Contributor Author

So if I understand correctly ref=BdOLSD on this route https://www.openstreetmap.org/relation/11051558 isn't a real identifier, but just made up to work around the fact that no other field from a route relation is available in OpenMapTiles?

I wouldn't go so far as to say an initialism isn't a real identifier, but yes, these ref values are just unofficial workarounds (hacks).

Seems to me like it would be better to work on improving OpenMapTiles with the desire for these type of scenic route shields as motivation. I doubt these will be the last scenic routes folks will want to add.

Agreed that something cleaner upstream would be nice, that there will be more cases in the future, and that this is the kind of issue Americana could push the envelope on.

But personally I think this is a "perfect is the enemy of good" scenario and would move forward as-is. We can improve the system later. But up to y'all.

@zekefarwell
Copy link
Collaborator

personally I think this is a "perfect is the enemy of good" scenario and would move forward as-is. We can improve the system later.

I also would consider it such a scenario if this was a fully self contained project. However, we are just one part of the OSM ecosystem, and I don't think we want to be seen as a project that supports questionable tagging practices. The stakes are quite low in this case, but I think it would be best to wait for a proper solution in OpenMapTiles.

@ZeLonewolf
Copy link
Member

ZeLonewolf commented Sep 13, 2023

I think openmaptiles/openmaptiles#1576 is not that far off, if the team is on board with that approach. While testing it, I did find a pretty major bug in openmaptiles that's causing a massive reduction in shields from z11 and lower. So my plan is to fix that issue and then push 1576 along. Since it's been a very long time since the last openmaptiles release, I'm led to believe we're not that far off from a proper solution for ref-less routes.

@ZeLonewolf ZeLonewolf dismissed their stale review September 13, 2023 14:05

issue addressed

@ZeLonewolf
Copy link
Member

Okay, the prerequisite is done.

I could use some design help in figuring out what the implementation for openmaptiles/openmaptiles#1576 should actually do to support these use cases.

@ZeLonewolf
Copy link
Member

Looks like the deploy preview is hit by actions/first-interaction#10. I'll plan to do some research as to how that can be resolved.

@osm-americana osm-americana deleted a comment from github-actions bot Sep 23, 2023
@ZeLonewolf
Copy link
Member

The CI issues have been resolved. The schema update remain unresolved.

Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

From my perspective, the artwork in this PR is ready to ship, so the only thing that needs to change is migrating from overrideByRef to overrideByName.

@@ -4201,6 +4204,33 @@ export function loadShields() {

// Ref-specific cases. Each entry should be documented in CONTRIBUTE.md

shields["CA:NS:S"].overrideByRef = {
BdOLSD: {
Copy link
Member

Choose a reason for hiding this comment

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

As of #1032, each feature has a route_#_name property based on the route relation. The name gets plumbed through to the shield selection code, so you should be able to specify overrideByName without worrying about the roadway carrying a different name, and the relations no longer need to be tagged with initialisms as refs.

route_1_network: CA:NS:R; route_1_ref: 322; route_1_name: Route 322; route_2_network: CA:NS:S; route_2_name: Lighthouse Route

@quincylvania
Copy link
Contributor Author

I've changed this to use overrideByName so I think it should be ready to go. I removed the made-up ref tags in this changeset: https://www.openstreetmap.org/changeset/150189553

Copy link
Member

@claysmalley claysmalley left a comment

Choose a reason for hiding this comment

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

@ZeLonewolf ZeLonewolf merged commit 9cf4cdd into osm-americana:main Apr 18, 2024
6 checks passed
@ZeLonewolf
Copy link
Member

Thanks for sticking this one out! Looks awesome!

@quincylvania
Copy link
Contributor Author

Thanks for running a project that improves tooling and sees things through even if it takes eight months!

@quincylvania quincylvania deleted the ns-scenic-drives branch April 20, 2024 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Nova Scotia scenic drive shields
5 participants