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

Local Test Fixes #1505

Merged
merged 2 commits into from
Mar 21, 2023
Merged

Local Test Fixes #1505

merged 2 commits into from
Mar 21, 2023

Conversation

benedikt-brandtner-bikemap
Copy link
Contributor

@benedikt-brandtner-bikemap benedikt-brandtner-bikemap commented Mar 19, 2023

This PR fixes local tests affected by the changes introduced in #1440 #1444 #1361 #1501

@@ -483,7 +483,7 @@ BEGIN
WHERE
(highway = 'motorway'
OR construction = 'motorway'
-- Allow trunk roads that are part of a nation's most important route network to show at z4
-- Allow trunk roads that are part of a nation's most important route network to show at z5
OR (highway = 'trunk' AND osm_national_network(network))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this highway = 'trunk' necessary given the change at line 525?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct if we add all important networks independent of their highway/construction tags to z4 anyways we should do it here as well.

@ZeLonewolf
Copy link
Contributor

This PR reverses #1444 which corrected a bug introduced in #1440. It was never anyone's intention to show all motorways at z4, only the most important ones. So far "most important" has only been defined for the US and Canada, but the intent is that similar top-level networks in other countries would be similarly defined for visibility at z4. Unless there is a specific client demand for z4 motorways in general, I don't think it makes sense to render them to the tiles as a universal rule. Of course the client can always filter them out at style time.

@benedikt-brandtner-bikemap
Copy link
Contributor Author

benedikt-brandtner-bikemap commented Mar 19, 2023

Hey, oh sorry so the intention of #1444 was to remove all motorways from osm_transportation_merge_linestring_gen_z4?

Because in the currently released version 3.14 this table contains all motorways independent of their network tags.
Then #1440 added trunks with the mentioned network tags and #1444 removed those trunks again but also removed all motorways and i thought that was not intentional.
But if it was then of course i'll revert 83589c2

@ZeLonewolf
Copy link
Contributor

ZeLonewolf commented Mar 19, 2023

The intent of the transportation layer at zoom 4 is to render an unlabeled transportation network that shows the overall shape of the road network without excessive detail. Including "any motorway" at that zoom would include many motorway spurs and islands that aren't desirable to see at that zoom.

Some of the discussion in ZeLonewolf/openstreetmap-americana#565 might be helpful to understand what this lowest-zoom rendering is trying to achieve. Also #1444 has some of the metrics, which include a 15% tile size difference at z5 (!!).

Adding test cases of course is a good idea (and would have prevented me from making the mistake in the first case!)

@benedikt-brandtner-bikemap
Copy link
Contributor Author

Alright, have removed the commit and ammended the tests

@benedikt-brandtner-bikemap
Copy link
Contributor Author

Have also removed this branch as a base for all the other PRs and rebased them ontop of master.

@ZeLonewolf
Copy link
Contributor

Good work. I dropped a note in Slack asking for a maintainer to run workflows.

@benedikt-brandtner-bikemap
Copy link
Contributor Author

thanks a lot :)

@TomPohys
Copy link
Member

Thanks a lot, @benedikt-brandtner-bikemap! Could you please also edit the "explanation text" for this PR? Thanks!

@benedikt-brandtner-bikemap benedikt-brandtner-bikemap changed the title Restore missing Motorways to Zoom-Level 4 Local Test Fixes Mar 21, 2023
@benedikt-brandtner-bikemap
Copy link
Contributor Author

Hey, have updated the title and description

@TomPohys
Copy link
Member

Thanks a lot!

@TomPohys TomPohys merged commit f918f4d into openmaptiles:master Mar 21, 2023
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.

None yet

3 participants