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

Change way_pixels calculation to use scale_denominator #3657

Merged
merged 1 commit into from
Feb 2, 2019

Conversation

jeisenbe
Copy link
Collaborator

Changes proposed in this pull request:

  • Way_pixels will now be calculated from way_area divided by the square of scale_denominator and the standard pixel size

Explanation:

  • Currently, the value of the column way_pixels is calculated from the way_area (which represents the approximate area of a polygon in square meters, but by Mercator projection) by dividing by pixel_width * pixel_height.
  • However, this causes the calculation to change when the resolution is changed, for example, when exporting at 2x resolution in Kosmtik, or when rendering "retina" resolution tiles.
  • Using the scale_denominator instead of the actual pixel height/width results in a consistent rendering at different rendering resolutions.

Test rendering with links to the example places:

Southern Australia
9/-32.7988/137.0764
Screenshot at standard resolution - unchanged before and after PR
z8
z8-master-x1-lake-gilles
z9
z9-master-lake-gilles-1x

Export at 2x resolution
z8 2x Before
z8-lake-gilles-master-2x

z8 2x After
z8-after-2x-gilles

Previously way_pixels was calculated from the way_area in square meters by dividing by pixel_width and pixel_height,
however, this causes the calculation to change when the resolution is changed. Using the scale_denominator results in a consistent
rendering at different rendering resolutions, and should be more compatible with vector tiles
@jeisenbe
Copy link
Collaborator Author

In #3648 we discussed the best way to show this code. @imagico had suggested basing it on the ppi (pixels per inch) of the standard resolution, which is approximately 90.71, hence:

way_area/NULLIF(POW(!scale_denominator!*0.001*25.4/90.71, 2),0)

I think the current code, way_area/NULLIF(POW(!scale_denominator!*0.001*0.28*, 2),0) is somewhat simpler. This is based on the 0.28mm standard pixel size that Mapnik assumes.

However, I'm happy to change this.

@imagico
Copy link
Collaborator

imagico commented Jan 27, 2019

Looks good but i have not tested it so far.

@hholzgra and @woodpeck might be able to comment if this works well practically.

@imagico imagico merged commit 3a0244b into gravitystorm:master Feb 2, 2019
@imagico
Copy link
Collaborator

imagico commented Feb 2, 2019

Tested and works fine, thanks.

Would still be good to get some feedback if this is useful in practical for-print rendering

@matthijsmelissen
Copy link
Collaborator

Good to have this finally fixed!

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Feb 2, 2019 via email

@Adamant36
Copy link
Contributor

Have you looked into MyOSMatic? It creates printable maps and I think it supports PDF. If so, maybe you could email the person that created for ideas.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Feb 3, 2019 via email

@jeisenbe jeisenbe deleted the way_pixels branch February 7, 2019 02:15
@sommerluk
Copy link
Collaborator

As it seems it's difficult to use way_pixels on vector tiles, I've played a bit with the MSS code. It is possible to define in MSS this variable @way_pixels: "([way_area]*pow(4,@zoom)/24505740000)"; and then use @way_pixels (instead of the SQL based way_pixels) later on. But it does not work well: While it works in Kosmtik for normal rendering, but yet for export is fails (completly, even when exporting at 100% scale). It seems that @zoom is not reliably available. scale_denominator isn't available in MSS, is it? I've also asked here.

@pnorman
Copy link
Collaborator

pnorman commented Oct 2, 2019

Way pixels has two uses - MSS filtering and SQL filtering. The SQL filtering is important for performance.

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.

6 participants