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

Render “surface” tag on roads with a pattern #2640

Merged
merged 2 commits into from
May 11, 2018

Conversation

sommerluk
Copy link
Collaborator

@sommerluk sommerluk commented May 23, 2017

Render “surface” tag on roads with a pattern

Closes #2621 (mutually exclusive with this PR)
Closes #3083 (mutually exclusive with this PR)

Resolves #110

@sommerluk
Copy link
Collaborator Author

sommerluk commented May 23, 2017

This code is still far from being complete. And there are many lines of code to write. And there are yet some questions. So I think it’s good to talk about these questions yet now.

Disclaimer: The pattern itself and its color are currently only an example. And, yes, highway=motorway is not the best example, but it was the first entry in roads.mss…

1.) I’ve attached the pattern to

#roads-low-zoom[zoom < 10],
.roads-fill[zoom >= 10],
.bridges-fill[zoom >= 10],
.tunnels-fill[zoom >= 10] {

and the consequence is that, where lines starts and stops, the round line caps of one line cover the pattern of the other line:

https://cloud.githubusercontent.com/assets/6830724/26347777/1e1403f8-3f9a-11e7-8fb8-d8beed89c977.png

So there are some parts that stay without pattern. I’m not sure if we can do something about. There seems to be not round line caps for patterns in Mapnik. And putting the surface in its own layer would reduce the problem, but is probably complicate code, and still leaves some artefacts if the two lines are not directly strait. Is attaching the pattern to the roads fill okay? Any ideas? (In general, I think this is not nice, but we have also other rendering artefacts in other situations, so this might not be a show-stopper.)

2.) Is the coding style okay? (Naming of files with leading 0 to allow automatic file order; type of selectors…)

3.) The code seems not to work for motorway_link

@imagico
Copy link
Collaborator

imagico commented May 23, 2017

There seems to be not round line caps for patterns in Mapnik

If that is the case this could indeed be a deal breaker (and my mental list of things Mapnik can't do keeps growing...).

Theoretically there is of course still the option to give up and use ST_Buffer() but i don't think we will want that due to code complexity and having all the line widths etc. in SQL. Alternatively there is also ST_StartPoint()/ST_EndPoint() which could be used to separately draw the round line end caps using marker symbolizer SVGs. But overall this is likely not any less complex.

Drawing all the line patterns above all the based colors could be worth trying - like you said this will lead to artefacts (both gaps and overlaps).

I have not looked at the code yet and i am probably also not the best one to evaluate that because i am not really that familiar with the roads code.

@imagico imagico changed the title Render “surface” tag with a pattern Render “surface” tag on roads with a pattern May 23, 2017
@imagico imagico added the roads label May 23, 2017
@sommerluk
Copy link
Collaborator Author

Introducing after ::fill another ::surface layer works. I’ve tested it. Gaps are visible where roads stop (“noexit”). Gaps within the normal road framework seem to be rather seldom. Overlay on each and every intersetion.

@sommerluk
Copy link
Collaborator Author

I think having a separate layer for the “unpaved” pattern has this behaviour on junctions:

  • Imagine a junction between a paved secondary road and an unpaved unclassified road.

  • In the rendering there will be the “unpaved” pattern above the junction.

So: What is the lesser evil?
1.) Rendering artefacts where highways are split?
2.) Unpaved pattern above paved junctions (with less rendering artefacts that at 1., but still with rendering artefacts)

@imagico
Copy link
Collaborator

imagico commented May 23, 2017

You could render the base fill color of paved roads above both the base color and the pattern of unpaved roads. That would of course mean yet another level and more complexity.

In total you would have (from bottom to top, not considering bridges, tunnels etc.):

  • casing for all roads
  • base color lines for unpaved
  • line pattern for unpaved
  • base color lines for paved

There is of course the question if that is correct if you have a unpaved primary connecting to a paved secondary but that is probably fairly theoretical.

@dieterdreist
Copy link

dieterdreist commented May 24, 2017 via email

@sommerluk
Copy link
Collaborator Author

sommerluk commented May 28, 2017

Here an example with the unpaved pattern in its own layer:

https://cloud.githubusercontent.com/assets/6830724/26527696/f24e5442-4388-11e7-9bdb-ffd6df5cb952.png

When the way is continued, it looks good when the angle is not too extrem (violet). When the angle is very extreme, there are some artefacts (blue). When the way is split and there is some 90° angle, than there is a small part that will not be covered, and when it’s more than 90°, than this uncovered part might be bigger (green). Also at start and end points of lines there is a small part that is not covered (black). When ways are crossing, there is an overlay (orange), but it’s less extreme than expected.

Overall, I think this is not ideal, but at least better than attaching the pattern to the very same street layer.

@sommerluk
Copy link
Collaborator Author

The idea with comp-op sounds nice. I’ll try that…

@sommerluk
Copy link
Collaborator Author

@sommerluk
Copy link
Collaborator Author

@dieterdreist Could you give some details about your proposal? What (alpha) geometry has to be substracted?

@pnorman
Copy link
Collaborator

pnorman commented May 28, 2017

Another layer won't work because it will fail with group-by and layering, e.g. a paved bridge over an unpaved road. You might be able to do something with attachments.

Besides the technical feasibility of pattern fills, we also have to look at if we can get good cartography from them. So far I don't like the examples I've seen, but how would they look on highway=secondary and below? Most unpaved roads will be some type of road which we render as white.

@imagico
Copy link
Collaborator

imagico commented May 29, 2017

I agree with Paul that now that we know the principal technical limitations of using line patterns it would probably be a good idea to test the visual suitability before putting too much work into finalizing the technical details. Your current examples are good for showing the principal problems but practically we will need to see how problematic these are with the colors we would actually use.

Since you will probably end up using a differently colored pattern for every road base color you probably can also use an SVG with a solid color background which would avoid the overlaps although the gaps and missing line ends would still create problems.

@sommerluk sommerluk changed the title Render “surface” tag on roads with a pattern [Work in Progress] Render “surface” tag on roads with a pattern Jul 2, 2017
@sommerluk
Copy link
Collaborator Author

After some time being busy with other stuff, finally I could do some work on this issue. It’s less straightforward than I thought. The above screenshots show different patterns.

Lessons learned:

  • It’s a good idea to start with white roads for developing the pattern, as suggested by @pnorman because that’s the most usual case.
  • It’s not a good idea to use a “colourful colour” for the pattern. Using something like brown, that could give an association to dirt, does not work well; on white residential roads, the rendering of brown patterns makes the road more similar to the yellow secondary roads, what is in my opinion not useful. It might therefor be better to use a grayscale colour. As residential roads are white, this could be grey. For other road types, this could also be grey, but always darker than the road colour, not lighter (this is to have the same paradigm on each road type).
  • On very thin roads, the pattern approach does not work well. I suppose this affects basically roads without casing, and maybe also roads with casing if they are still thin because the zoom level is still low. So probably this means that the “unpaved” pattern can be rendered starting from a given road width and would therefore have a different start zoom level for each road type. (Otherwise, we would have to make the zoom level depend on the smallest roads – service roads – which would limit the rendering to really high zoom levels).

Comments? Which of the above patterns might be the best to continue the work?

@imagico
Copy link
Collaborator

imagico commented Jul 2, 2017

I think these are mostly too strong, the pattern should be strong enough to be recognizable under most circumstances, at least for the wider roads, but not any stronger. Remember this is a secondary characterization of roads behind the main classes. It is better if in some cases the pattern is not recognizable than if it hides or obscures other information. The 07 example seems fine in that regard but as you mentioned it should be color neutral for the white roads of course.

For the colored roads i would probably try making the pattern background slightly brighter than the paved color and the grain slightly darker. This is not possible for the white roads of course.

Something like

http://www.imagico.de/map/jsdotpattern.php#x,64,jdp59980;g,2.25,16,16;rx,250,2,8,8;rx,250,2,8,8;s,jdp58799;s,jdp49103;rx,250,2,8,8;rx,250,2,8,8;s,jdp94534;rx,250,2,8,8;rd,0,0,1,scree,0.07,5,10,0,jdp40148,3e3e3e,ffffff;

might work for the white roads, maybe even with an even lighter color.

What a grain pattern will inevitably do is making color differences between the different road classes more difficult to distinguish. In case of the white roads this mainly concerns highway=living_street.

Yes, the very thin roads will be a problem - though primarily at z<16 of course. Also need to see if the access restrictions are still properly visible with the pattern.

@dieterdreist
Copy link

dieterdreist commented Jul 2, 2017 via email

@sommerluk
Copy link
Collaborator Author

did you try subtracting with dst-out rather than adding the pattern?

Yes, a little bit. At the first try, I didn’t understand how this works. I tried to create a new layer filled with the pattern and cut off what’s out of the road surface. It didn’t work, also because Mapnik cannot group layers.

After reading the documentation, I guess it could work this way:

  • Render the unpaved roads layer with dst-out cutting a hole where the roads fill should be.
  • Next, render a layer with the pattern (opaque) and use a layer rendering mode that pulls it behind the existing content.
  • To make this work for various road types, we need an own layer for each road type (or at least each road colour).

Is this what you had in mind? If so, I can try to make a working prototype, but this might take some time…

@dieterdreist
Copy link

dieterdreist commented Jul 2, 2017 via email

@sommerluk
Copy link
Collaborator Author

Pattern 11 proposed by @imagico looks good. Personally I would not go to a lighter tone, because yet right now it depends on the angle at which I’m looking to my screen if I can remark it or not.

A positive side-effect of pattern 11 (compared to the previous patterns) is that, as it’s lighter, the rendering artefacts of line-pattern are less visible.

@sommerluk
Copy link
Collaborator Author

sommerluk commented Jul 8, 2017

@sommerluk
Copy link
Collaborator Author

The basic idea is to work with the holes, not with another color that is added.

Okay, now I understand. This would make the street colour look different depending on the ground where they are rendered above. I’m not sure if that’s desirable (especially for wood).

Technically, dst-out cuts a “hole” (transparent part) in the layers below. I’m not sure if it’s possible to cut just the roads fill and the casing (which is below), without cutting the background (residential, wood…) also.

@sommerluk
Copy link
Collaborator Author

dst-out (mentioned by @dieterdreist) could maybe help with the line-pattern rendering artefacts.

dst-out ignores colours, it takes just the shape information (where is something rendered), and instead of rendering, it cuts a hole in all underlying layers (=it makes them transparent). In the final rendering, these parts show up with the colour #dddddd; which must be some sort of default.

We could

  • render the road fill of unpaved roads with the dst-out option (leaving these parts transparent)
  • add a new layer above the road layer, which uses as data some fake data (polygons that cover the hole planet), render these polygons with the roads fill colour as an opaque colour and with the unpaved patters above, and then tell Mapnik to render it behind the yet existing layers (which makes it appear at all transparent parts = unpaved roads).

I’ve done a quick-and-(very-)dirty hack, and it works.

  • The advantage is that this is a solution without rendering artefacts.
  • Maybe there’s a more elegant way to do this instead of using fake polygons.
  • The downside is that (as far as I understand) we would need to have separate layers (!) for each road colour, so we would need separate SQL querries for each road colour. If so, the code will become much more complex.
  • The complex code changes might have undesired side effects.
  • It will raise the maintaince burden.
  • An intermediate solution might be to do this only for one road colour (for example the white roads, which are most interesting regarding paved/unpaved), but use the normal ligne-pattern for the other colours. This might work without splitting the the roads SQL querry.

(Anyway, I’m new to the comp-op feature and so I’m not totally sure if everything above is correct…)

@kocio-pl
Copy link
Collaborator

kocio-pl commented May 7, 2018

Do you feel that this code is mostly complete and sane? If yes, we could merge it as it is soon, so it would be included in the next release, which is planned on Friday.

I think 1 year of work is long enough and we can tune the pattern visibility later on.

@sommerluk sommerluk force-pushed the unpaved04 branch 2 times, most recently from 966cd9d to b573d60 Compare May 7, 2018 22:59
@sommerluk
Copy link
Collaborator Author

I’ve removed the pattern for track area. An additional argument for removing pattern for track areas: The pattern is about “surface” tag. But for track ways, we do not render the surface tag, but the tracktype tag. This would be confusing together the track area surface rendering.

From my side, this is ready now.

@kocio-pl
Copy link
Collaborator

kocio-pl commented May 7, 2018

@matkoniecz What's your opinion on merging current code?

@kocio-pl
Copy link
Collaborator

kocio-pl commented May 8, 2018

I think it makes sense to move all the images into new symbols/unpaved/ directory, there are too many of them.

@sommerluk
Copy link
Collaborator Author

move all the images into new symbols/unpaved/ directory

Done.

@sommerluk
Copy link
Collaborator Author

we could merge it as it is soon, so it would be included in the next release, which is planned on Friday

Than I would rather suggest – instead of merging it before Friday – merging it after Friday, so having it in master for some time before a release.

@sommerluk
Copy link
Collaborator Author

Rebased.

@kocio-pl kocio-pl merged commit e3a508a into gravitystorm:master May 11, 2018
@kocio-pl
Copy link
Collaborator

OK, I have merged it now, since v4.11.0 is released already. Please test it as much as possible now.

@matkoniecz has not approved merging, but I think this is the right moment and if anything is seriously broken, this code can be reverted, but if there are some smaller issues (like my own concerns with pattern visibility), they should be just fixed now.

@sommerluk Thanks for your work! 😄

@sommerluk
Copy link
Collaborator Author

I want to say “Thank you” to everyone who has contributed here (in order of appearance in the comments):

  • @imagico For advice with the pattern generation, an extension in jsdotpattern helping in this PR, writing the whole code for colour generation within a perceptional colour space, debugging the colour choice, SQL, and and and…
  • @dieterdreist For the initial proposal to use comp-op (without that, this code wouldn’t have been possible)
  • @pnorman For help with SQL code and alternative styling proposals
  • @matthijsmelissen For help with SQL
  • @mboeringa For taking part in the discussion
  • @kocio-pl For discussion, testing, taking this PR further and all the release-related staff
  • @HolgerJeromin For taking part in the discussion
  • @matkoniecz For testing and helpful feedback to the code

@sommerluk
Copy link
Collaborator Author

Earlier @imagico said “i think it would be nice if you could find the time to write up the underlying technical idea in a generic form”.

Well, I’m not really good in writing documentation (especially en English), but here we go:

https://github.com/sommerluk/roadpatternrendering

Comments are welcome, including language-related corrections.

I think we should have also at least a basic documentation here in openstreetmap-carto. Maybe in roads.mss (but not so verbose)?

@imagico
Copy link
Collaborator

imagico commented Jun 24, 2018

Looks very good. Reading over it now it seems the two nested queries to get a list of all distinct layer values are unnecessarily complex. In the original sketch the inner query was reused from the main geometry query but without doing this i think the following is more compact and should lead to the same result:

(SELECT
    DISTINCT ON (COALESCE(layer,0))
    COALESCE(layer,0) AS layernotnull
  FROM planet_osm_line
  WHERE bridge IN (...)
  AND highway IS NOT NULL
  AND surface IN (...)
) AS layers_list

pnorman added a commit that referenced this pull request Jun 27, 2018
This reverts commit e3a508a.

This PR introduced critical performance issues.

Fixes #3280
@kocio-pl
Copy link
Collaborator

kocio-pl commented Jul 2, 2018

@sommerluk What do you think about this code now? It's reverted to make the immediate effect on performance and v4.12.1 has been released, so we can think what to do next. Would you like to update it and re-submit for merging or maybe something else would be better for you?

@sommerluk sommerluk deleted the unpaved04 branch July 6, 2018 05:32
@sommerluk
Copy link
Collaborator Author

@kocio-pl Thanks for bringing this up. I’ve played around with this, see #3280 (comment) for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants