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

Reduce number of overlapping admin borders #1107

Merged

Conversation

matthijsmelissen
Copy link
Collaborator

This prevents overlapping admin borders by rendering them in order from high to low admin_level, and by giving all admin borders a white background while using comp-op: darken.

Admin borders are currently grouped into three groups: level 0-4, level 5-8, and level 9+. Overlap between these groups is avoided, without compromising performance, by creating one attachment for admin_levels 0-4 for low zoom levels, one for admin_levels 0-8 for mid zoom levels, and one for all admin_levels for all zoom levels.

This solves #344.

This reduces the number of overlapping admin borders by rendering them in order
from high to low admin_level, and by giving all admin borders a white background
while using comp-op: darken.

Admin borders are currently grouped into three groups: level 1-4, level 5-8,
and level 9+. This changes only prevents overlap of admin borders within a
group (including overlap of borders of the same level). To prevent overlap of
borders within groups, it would be necessary to merge the groups, which would
lead to performance loss, and is therefore not included in this PR.

This partially solves gravitystorm#344.
This prevents overlapping admin borders of different layers.
For performance reasons, the admin border layers are split into three groups
for low, middle and high zoom levels.
For each zoomlevel, all borders come from a single attachment, to handle
overlapping borders correctly.
@matthijsmelissen
Copy link
Collaborator Author

Updated PR - it now also prevents overlap between groups.

@matthijsmelissen
Copy link
Collaborator Author

Note that this solution is quite different from the proposed solution in #344, which proposed solving it by a carefully crafted SQL statement. That appeared not so simple (see here), while the current solution, which is basically rendering the spaces between the dashes in white so that underlying borders get painted over, seems to work fine. The comp-op: darken guarantees that the white does not have any effect on other layers.

@yohanboniface
Copy link

Do you have any before/after screenshots by hand? I'm really curious about the result. I've made some tests in the past (for HOT rendering), but I never get to the point I really controlled the magic of comp-op.

@yohanboniface
Copy link

For example, this kind of fuzzy rendering (the mix between boundaries make the drawn path irregular):
screenshot from 2014-11-02 12 56 21

@matthijsmelissen
Copy link
Collaborator Author

Before:
before_812729

After:
after_a271e6

@yohanboniface
Copy link

Basically, in my case at least, I think there are two drawbacks:

  • the output will be different according to the number of actual boundaries drawn one over the other (the more the darker)
  • if two boundaries are drawn one on top of the other but both paths don't start at the same point, so it seems the dash array is out of sync, which create a fuzzy rendering

Plus, but this one is acceptable to my eyes given that we have other benefits, we can't mix dash-array patterns.

@yohanboniface
Copy link

Thanks! It's really great on this example.

Let me try this anyway locally with the data I'm used to work with :)

@matthijsmelissen
Copy link
Collaborator Author

Basically, in my case at least, I think there are two drawbacks:

My solution avoids these drawbacks, as every new boundary is painted on top of (and erasing) the previous boundaries.

Make sure that all admin boundaries are in the same attachment. For it to work, it does require that high admin_level boundaries are not wider than low admin_level boundaries, but most styles would require that anyway.

@yohanboniface
Copy link

every new boundary is painted on top of (and erasing) the previous boundaries.

So I think I'm missing a point in the logic here. Here is what I understand you are doing:

  • ORDER BY admin_level DESC on the SQL side
  • add a white path attachment below normal path (of the same width)
  • add a comp-op: darker rule so that the white never get paint

So what I don't get is how do you prevent one line segment drawn below a white background to appear? I mean if you have two boundaries drawn one of top of the other, but the paths don't start at the same point, so the dash-array is out of sync, so some "white background" (i.e. space between path segments) is drawn on top of the segment of the path underneath? What I understand of the comp-op algorithm is that if that when a path (or any other shape) is being to be paint, only the elements that are darker that the underneath already paints elements will be actually paint.

(Not sure my question is clear though ;) )

@matthijsmelissen
Copy link
Collaborator Author

The comp-op property applies to attachments, not to individual objects. So first all boundaries get drawn to the admin border attachment one by one without taking comp-op into account (resulting in a purple-white dashed line of the last drawn admin border), and only then the admin border attachment gets merged (with comp-op) with the earlier drawn attachments.

So your interpretation is darken(admin-border-2, darken(admin-border-1, background)), while what happens is darken(renderontop(admin-border-2, admin-border-1), background).

Not sure if my answer is clear enough either :).

@yohanboniface
Copy link

Thanks for those details!

But still something that doesn't connect in my slow brain ;)

Here my SQL (made simpler than the real final need in order to make simpler to debug):

SELECT way, name, admin_level 
    FROM planet_osm_roads
    WHERE boundary='administrative' AND admin_level IN ('0', '2', '3', '4')
    ORDER BY admin_level DESC

Here is my style (made simpler than the final too for same reason)

#admin-1-4[admin_level='2'][zoom>=5],
#admin-1-4[admin_level='3'][zoom>=5],
#admin-1-4[admin_level='4'][zoom>=5] {

  eraser/line-color: white;
  eraser/line-width: 1;
  line-color: @admin_2;
  line-width: 1;
  [admin_level!='2'] {
    line-dasharray: 5,5;
    line-color: @admin_3;
  }
  comp-op: darken;
}

But I still get this kind of fuzzy rendering:
screenshot from 2014-11-02 14 16 11

Same view with your OSM-Carto branch goes well:
screenshot from 2014-11-02 14 18 12

Any idea what I'm doing wrong?

(Oh, and, humm, sorry for hijacking your PR :s)

@matthijsmelissen
Copy link
Collaborator Author

Maybe interaction between #admin-1-4 and #admin-5-whatever? Does the fuzzy rendering remain if you remove the attachments for admin levels 5 and higher from the code?

@yohanboniface
Copy link

Yes, I've already filtered out those levels. The rendering goes as expected only when I filter out admin_level=4.

@matthijsmelissen
Copy link
Collaborator Author

As the line gets rendered (partially) dashed, it seems that the border for level 4 gets rendered on top (so after) the border for level 2? Seems that the 'ORDER BY admin_level DESC' is not taken into account?

@yohanboniface
Copy link

Mamma mia, I was editing admin-level-1-4 instead of admin-1-4! Thanks for having played the rubber duck role :)

@matthijsmelissen
Copy link
Collaborator Author

Good to have an independent verification of my code/method at least :).

@matkoniecz
Copy link
Contributor

It works but I am not sure why - how background/line-color blocks previously rendered borders but is not displayed?

@matkoniecz
Copy link
Contributor

It seems that labels for borders are displaying now only the most important one (what may be a good or bad change) and are now really rare (what I think is a change for worse).

@matthijsmelissen
Copy link
Collaborator Author

It works but I am not sure why - how background/line-color blocks previously rendered borders but is not displayed?

As I said, comp-op works per attachment, not per object. So we generate an attachment containing a set of purple-white dashed lines (of which only the top ones are visible), and with comp-op: darken the white part is ignored, while the purple part is rendered.

It seems that labels for borders are displaying now only the most important one (what may be a good or bad change) and are now really rare (what I think is a change for worse).

Border labels come from admin-text, which I did not touch. Are you sure about this?

@matkoniecz
Copy link
Contributor

Are you sure about this?

It turns out that differences in this case are caused by my TileMill setup (most likely - related to my dataset).

@vholten
Copy link
Contributor

vholten commented Nov 2, 2014

I think that this method does not work if the borders are rendered on a dark area. In that case, comp-op: darken will not allow anything to be drawn because the border color is lighter than the landuse color.
Nevertheless, the method seems to work in this style because all landuse colors are lighter than the border color.

@matthijsmelissen
Copy link
Collaborator Author

Good catch. Do you think there is a different comp-op that works better?

@matthijsmelissen
Copy link
Collaborator Author

I experimented a bit with this, and I don't think there's much we can do about this. The admin borders are not visible on landuse darker than #606060 or so. For the current style it's not a problem, whether the possibility to extend the style to other colours is a problem is for @gravitystorm to decide. I would suggest to merge this anyway, as without this PR, the borders are essentially unreadable (it's impossible to distinguish the different admin_levels), and such dark landuse is rare, also in other styles.

@matkoniecz
Copy link
Contributor

I also would suggest merging - it is vastly better than the current situation.

@math1985
Maybe add comment somewhere about this potential problem? But I am not sure about proper place for it.

@Circeus
Copy link

Circeus commented Nov 6, 2014

Just want to say that (as far as I understand it, not being cognizant in cartocss), this sounds like a really elegant solution to one of my biggest pet peeves.

gravitystorm added a commit that referenced this pull request Nov 14, 2014
Reduce number of overlapping admin borders
@gravitystorm gravitystorm merged commit 37a928e into gravitystorm:master Nov 14, 2014
@gravitystorm
Copy link
Owner

This fails the "too much magic" test! I'd definitely like some explanation to go in the code, otherwise it's very easy to overlook the comp-op lines or understand how the whole thing works.

@matthijsmelissen
Copy link
Collaborator Author

@gravitystorm Thanks for merging!

I will add some documentation.

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