gravitystorm / openstreetmap-carto

A general-purpose OpenStreetMap mapnik style, in CartoCSS
Other
1.54k stars 824 forks source link

Scalable maps: update default styles to use 'px' explicitly #2044

Closed OnkelTem closed 8 years ago

OnkelTem commented 8 years ago

Problem

Current styles are implemented in the way so that all properties with linear sizes (text-size, shield-size and etc) has no units specifications. According to how Carto works a missed unit is supposed to be a px-unit and that is good for screen rendering.

But if you try to cook another osm.xml with explicit --ppi specification (default carto's ppi = 90.714) you'll find out that neither of sizes are scaled by Carto. This happens because Carto parser doesn't scale things w/o units specification:

lib/carto/tree/dimension.js:

        // normalize units which are not px or %
        if (this.unit && _.contains(this.physical_units, this.unit)) {
            // ...
            // convert all units to inch
            // convert inch to px using ppi
            this.value = (this.value / this.densities[this.unit]) * env.ppi;
            this.unit = 'px';
        }

— note this.unit which is NULL if untis are not set.

Honestly, it won't scale things even with 'px' specification but that is a part of another problem (see below).

Proposition

I propose to add explicit 'px' units everywhere where values of this type are expected. Will this make map scalable? Not right away, but it would allow to start scaling maps at Carto side.

For example, this quick patch makes Carto to scale px-es:

diff --git a/lib/carto/tree/dimension.js b/lib/carto/tree/dimension.js
index 07232f2..7d762ee 100644
--- a/lib/carto/tree/dimension.js
+++ b/lib/carto/tree/dimension.js
@@ -11,7 +11,7 @@ tree.Dimension = function Dimension(value, unit, index) {

 tree.Dimension.prototype = {
     is: 'float',
-    physical_units: ['m', 'cm', 'in', 'mm', 'pt', 'pc'],
+    physical_units: ['m', 'cm', 'in', 'mm', 'pt', 'pc', 'px'],
     screen_units: ['px', '%'],
     all_units: ['m', 'cm', 'in', 'mm', 'pt', 'pc', 'px', '%'],
     densities: {
@@ -19,7 +19,8 @@ tree.Dimension.prototype = {
         mm: 25.4,
         cm: 2.54,
         pt: 72,
-        pc: 6
+        pc: 6,
+        px: 90.714 // the same as the default ppi
     },
     ev: function (env) {
         if (this.unit && !_.contains(this.all_units, this.unit)) {

I've created a follow-up ticket in mapbox/carto queue: https://github.com/mapbox/carto/issues/427

Implementation If you agree with this initiative, I'm ready to create a PR.

Afterthoughts Actually, I have a feeling that all this can be just out of line if the styles are generated by a software like TileMill or similar. If this is the case and unit-less units is what the software generates, then my initiative is useless. Tell me the truth please :)

nebulon42 commented 8 years ago

I'm not sure what you are trying to achieve with setting the ppi parameter of carto. As far as I understand it it is intended to convert other units to pixels. Pixels is the base unit of Mapnik. It would not make sense to convert pixel values to a "wrong" pixel value depending on a ppi setting.

Instead you should set the scale factor for Mapnik (https://github.com/gravitystorm/openstreetmap-carto/blob/master/project.yaml#L1). If I set this to 2 I have no problem in generating higher resolution tiles. Here is an example (bluriness comes from Github downscaling the image, click on it to view the full size):

scale_test

OnkelTem commented 8 years ago

@nebulon42 Thanks for the feedback. Honestly I didn't know about this feature - or rather I read about it before but it never worked until now - I've finally found a bug (maybe it's actually a feature - see the next comment) in a script I used for rendering.

So I've got a chance to test this scale_factor and it really works but in the way I'd call: incoherent — for an unknown reason many features escape from the map.

Scale_factor=1, my custom style (note the amount of text objects):

Scale_factor=3, default styles (and here the great deal of the objects are gone):

OnkelTem commented 8 years ago

Just for reference, maybe here hides the reason of features "disappearing": https://github.com/Zverik/Nik4/issues/20

pnorman commented 8 years ago

I propose to add explicit 'px' units everywhere where values of this type are expected.

In CartoCSS this would be invalid

In CartoCSS, you specify just a number - unlike CSS, there are no units, but everything is specified in pixels.

If CartoCSS were to change, we could consider this, but we'd have to wait for it to change, a new carto release, and that release to be taken up by tilemill, kosmtik, and other places where carto is used.

There are also no issues that prevent this style from being used at a higher resolution, except for a few cases where PNGs are still used. I've done it with Tilemill and 600dpi, and I know people have done 2x resolution tiles with it. If your software has trouble with this, it's either a configuration error or a problem with the software.

OnkelTem commented 8 years ago

P.S. CartoCSS doesn't seem to be a standard or something. What we have is the implementation (exactly that repository which you've referenced), which does support lots of units and makes their conversion — see the code from above.

OnkelTem commented 8 years ago

And this is carto help (check out the last line):

$ ./carto --help
Usage: node ./carto <source MML file>

Options:
  -h, --help       Display this help message                                      
  -v, --version    Display version information                                    
  -b, --benchmark  Outputs total compile time                                     
  -l, --localize   Use millstone to localize resources when loading an MML          [default: false]
  -n, --nosymlink  Use absolute paths instead of symlinking files                 
  --ppi            Pixels per inch used to convert m, mm, cm, in, pt, pc to pixels  [default: 90.714]

So I wonder what new releases we should be waiting for actually. It seems like the documentation is basically outdated...

nebulon42 commented 8 years ago

To be honest I'm no expert on scaling, so I might have missed something else. But still using ppi in carto to change pixel value to pixel value seems wrong to me. On a side node development on Mapbox' carto implementation has pretty much stopped IMO and I see no way how this could become better in the near future. This is why I was "forced" to create my carto fork (https://github.com/gmgeo/cartocss). If you are looking for a more actively developed carto library I recommend checking out https://github.com/omniscale/magnacarto.

OnkelTem commented 8 years ago

Thanks @nebulon42, I'll check it out.

P.S. I'd dream to see CartoCSS to be more llike CSS, with solid inheritances and overrides.