Allow combine_below on each layer

[etienneJr]

Context

The fact that combine_below is defined in the settings part of config.json is not very flexible, and not consistent with combine_points and combine_polygons_below properties, defined in each layer.

Solution

Following #837 I tried to copy the logic of combinePoints and combinePolygonsBelow layer properties to create a new property combineLinesBelow that will store the value read in each layer definition in config.json.

But it is not working 😥​

tile_worker.cpp is able to read the old ld.combinePolygonsBelow property but is not able to read the new ld.combineLinesBelow property. shared_data.cpp and shared_data.h seems fine to me, I just copied the logic of combinePolygonsBelow, I really can't see what I've missed. ProcessObjects() works fine if I force a value for its new boolean property combineLines, but in ProcessLayer() the new Layer property ld.combineLinesBelow value is always 0.

Could you help me correct this? Thanks!

[systemed]

I've had a play around with this and it seems to work as expected. Adding some extra logging at the start of ProcessObjects, and with "combine_below": 15 in the transportation layer, I get

Layer waterway at 14/ combine lines below 14
combineLines n
combineLines n
Layer waterway at 14/ combine lines below 14
combineLines n
combineLines n
Layer transportation at 14/ combine lines below 15
combineLines y
combineLines y
Layer transportation_name at 14/ combine lines below 15
combineLines y
combineLines y

which looks right to me - or am I missing something?

(Silly question - have you tried doing a make clean before building? The build doesn't always pick up changes to .h files otherwise.)

Otherwise this is great - only suggestion I'd make is renaming combine_below to combine_lines_below in the layer JSON. I think we can then deprecate combine_below as a global option.

[etienneJr]

(Silly question - have you tried doing a make clean before building? The build doesn't always pick up changes to .h files otherwise.)

Oh no, it's not a silly question at all!! It's my first C++ project, I didn't know I had to start with a make clean! It resolved all the errors I had. I feel rather foolish, I wasted so much time trying to understand my mistake in the code 😵​

only suggestion I'd make is renaming combine_below to combine_lines_below in the layer JSON. I think we can then deprecate combine_below as a global option.

I wanted to keep combine_below in the global settings for retrocompatibility. In the layers settings, I used combine_lines_below. If combine_lines_below is absent in the layer settings, we use the global setting combine_below. Is it fine for you ?

I will finish the corrections, tests, and documentation tomorrow.

[etienneJr]

Example of the resulting tiles using:

• in global settings : combine_below: 14
• in transportation layer : combine_lines_below: 15

layer	zoom 13	zoom 14
transportation	combined	combined
transportation_name	combined	not combined

the log is (is it too much?)

Layer place (z0-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer boundary (z0-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer poi (z10-14) - combine points: 0 - combine lines below 14 - combine polygons below 0
Layer poi_detail (z14-14) - combine points: 0 - combine lines below 14 - combine polygons below 0 -> poi
Layer housenumber (z14-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer waterway (z8-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer waterway_detail (z12-14) - combine points: 1 - combine lines below 14 - combine polygons below 0 -> waterway
Layer waterway_big (z3-7) - combine points: 1 - combine lines below 14 - combine polygons below 0 -> waterway
Layer transportation (z4-14) - combine points: 1 - combine lines below 15 - combine polygons below 0
Layer transportation_name (z8-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer building (z13-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer water (z6-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer ocean (z0-14) - combine points: 1 - combine lines below 14 - combine polygons below 0 -> water
Layer water_name (z6-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer water_name_detail (z6-14) - combine points: 1 - combine lines below 14 - combine polygons below 0 -> water_name
Layer aeroway (z11-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer aerodrome_label (z10-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer park (z11-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer landuse (z4-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer urban_areas (z4-8) - combine points: 1 - combine lines below 14 - combine polygons below 0 -> landuse
Layer landcover (z0-14) - combine points: 1 - combine lines below 14 - combine polygons below 0
Layer ice_shelf (z0-9) - combine points: 1 - combine lines below 14 - combine polygons below 0 -> landcover
Layer glacier (z2-9) - combine points: 1 - combine lines below 14 - combine polygons below 0 -> landcover
Layer mountain_peak (z11-14) - combine points: 1 - combine lines below 14 - combine polygons below 0

[etienneJr]

Hi Richard,
I pushed (force) a single clean commit with the feature and its documentation. You can see the expected result in the comment above. It's fine on my side, I remove the WIP.

[systemed]

Oh no, it's not a silly question at all!! It's my first C++ project, I didn't know I had to start with a make clean! It resolved all the errors I had. I feel rather foolish, I wasted so much time trying to understand my mistake in the code 😵​

It's not a global thing, it's just that tilemaker's Makefile doesn't know how to spot changes in header files. It annoys me too but I don't know enough about Makefiles to fix it unfortunately.

Changes look great - thanks. I'll have a proper test and then merge this.

[systemed]

Merged as #850 - thanks!