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 'shield_text' property on roads #966

Merged
merged 10 commits into from
Aug 12, 2016
Merged

Conversation

zerebubuth
Copy link
Member

This adds a shield_text property to go with the network on roads. There are a few changes from previous behaviour:

  • A "most important" network is now chosen, instead of whichever was first. This will give better consistency between adjacent road segments and when routes merge.
  • Only road networks are considered for "most important". This means the network property disappears from some existing roads where it duplicates the bicycle_network property.

The "most important" calculation is currently very simple, and does not have special cases in for various special roads. For example it is recommended that Historic Route 66 is mapped without a ref tag, which will mean it doesn't appear. In general, name is ignored and ref is assumed to be numeric.

This seemed good enough for an iteration, and we can improve it if/when good examples are available. @nvkelso does that sound good?

Connects to #192.

@rmarianski could you review, please?

--
-- it does this by joining onto the relations slim table, so it
-- won't work if you dropped the slim tables, or didn't use slim
-- mode in osm2pgsql.
--
CREATE OR REPLACE FUNCTION mz_get_rel_network(
CREATE OR REPLACE FUNCTION mz_get_rel_networks(
way_id bigint)
RETURNS text AS $$
Copy link
Member

Choose a reason for hiding this comment

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

Is it easy to have this return back an array, hstore, or json with the values instead of a delimited string? This is just meant for the python transform to consume and process right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes! Good point! I had forgotten that was JSON and thought it had to be stringly to fit in an hstore. Fixed in 861ac6d.

@rmarianski
Copy link
Member

Put in some minor comments, but the implementation lgtm. 👍

@nvkelso
Copy link
Member

nvkelso commented Aug 12, 2016

Nice!

If we're iterating thru all the networks and their shield_text, can we please also export that entire list of shields (even if it's on two new properties instead of the current singular properties)?

@zerebubuth
Copy link
Member Author

Yup, sure! In 62375da added support for exporting all_networks and all_shield_texts as lists. This means that they won't be available in MVT format, as we currently can't encode lists as MVT values.

@nvkelso
Copy link
Member

nvkelso commented Aug 12, 2016

For lists in MVT values, do we drop those completely for that format now? Could we add new logic in the mapbox-vector-tile library to concatenate lists into a single string with ; separator characters? (We need to support these for Eraser Map, for instance, that uses MVT but also needs multi shields.)

@zerebubuth
Copy link
Member Author

Yes, we drop them at the moment. We could add that logic, but it might be better to encode them as JSON lists rather than ; concatenate them just in case there are escaping issues.

I've added tilezen/mapbox-vector-tile#64 to track this. But since multiple road shields aren't supported in any of the renderers using our tiles, and aren't part of the v1.0 milestone, that suggests it's okay to leave until v2 or later.

@zerebubuth zerebubuth merged commit 4c3957b into master Aug 12, 2016
@zerebubuth zerebubuth deleted the zerebubuth/192-shield-text branch August 12, 2016 18:25
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.

3 participants