-
Notifications
You must be signed in to change notification settings - Fork 819
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
[WIP] Add render for power portals and insulators #3464
Conversation
Declare 4 new features : power insulators and portals as both ways and nodes
@gustavecha Can you try with thinner line? I think it's unnecessary too prominent. |
@kocio-pl will go on testing it |
I would suggest to not use green colour for the connection dots. Green should be rather reserved to natural features like trees and so an. Black dots seem more appropriate. |
I am also unhappy about green dots, for me it looks very weird. I would also keep green for natural features and leisure. |
Both insulators and portals are render with 5B5B5B grey instead of black and green. Reducing line width to 1.5 for way portals.
Node insulators move to size 5 and node portals are rendered like power tower instead of poles.
Indeed the green wasn't a great choice and I updated the mss to change that. |
Sorry for the time it took me to comment this code. The biggest question I have here is - do we really need 4 new data layers? As far as I understand (which might be not true) adding new layers can make performance problems. |
I think it all should be coloured by "@power-line-color" to match the rest of power system rendering. I would use 1.5 width for power insulators ways. |
I guess it's enough that this is just wider, because it's clear from the context, no need to change the color. Could you test the rendering? I'm not even sure why we need both Usage: |
I'm not too familiar with electricity distribution in general and in OSM, could you show an example of such a busbar, so I could understand the problem? |
Busbars are power lines in a substation allowing to switch the power from one incoming power line to another outgoing from the substation. Power portals are metallic but grounded (not powered) supports. Power lines are anchored with insulators on them but not connected (unless a big lightning could appear and damage stuff around). Tavel substation has recently been updated with actual bays between incoming lines and busbars : https://www.openstreetmap.org/#map=18/44.01483/4.64341 Then, despite all power stuff shouldn't clutter render, I think that it should not confuse between powered elements and grounded ones. May we use two different greys for that ? |
I see now and I think you're right.
Make a test, please. |
It doesn't stand up too much, I prefer original look. Power stations are quite boring and unified, some elements should look different enough to help understand its structure, not only to see a lot of similar lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gustavecha
First of all, thanks for this pull request. I think it is a good idea to have a more complete rendering of power infrastructure, and your code is a good starting point for this.
Here I try to provide a more detailed review of your PR to allow more fine-tuning.
About the colour choice for the portals: It’s important to stick to our current colour scheme. The rendering might seem “boring”, but that’s nothing negative at all. Maps have to be “boring” to a certain extend to remain legible, especially if they are so feature-rich like this map style is. Having too many different colours is confusing. Therefore the colour for the portals should be a standard colour. This could be either the standard “power” colour of maybe also another appropriate colour, maybe the “man-made” colour.
I’m thinking about it might be better not to render insulators at all.
-
One reason is consistency: All power towers for high voltage lines do not have the lines directly fixed on them, but they are fixed with insulators. Though most of the power=tower nodes do not have a power=insulator explicitly tagged, treating insulators on portals somewhat special seems inconsistent.
-
Another reasons is the proposed rendering style: In circuit diagrams, the dot means “connection”. Using the same shape here for the opposite (=insulator) seems missleading.
-
The rendering is not strictly necessary, because in most cases, lines crossing portals will indeed be connected by insulators with the portal. So it is perfectly possible to read the map also when the insulators are not explicitly rendered.
The other, more technical comments of this review you can find inline in the code.
Thanks again for the PR!
We try to give feedback to the mappers, so yes, there is clear downside of rendering all the elements the same, it's not about entertaining them. 😄 We show different colors of nodes for shops/gastronomy/offices because of that, we show roads with different colors and size to help determine more about them other than just being road, we also show power poles different exactly because of this. Power station should also be more than just a bunch of identical lines, if we know they differ and play different role there. More solid/important elements of the structure should use stronger color. |
I second this point of view. |
@kocio-pl Shops and offices are big categories. It makes sense that they have their own colour. But if we would introduce individual colours for each individual rather seldom feature, than we would not only run out of available colours soon, but we would also have a horrible mess in the map soon. So: No, we do not need at all individual colours for each feature. Groups are more important, so the map stays legible. @gustavecha Please stick with the currently existing colours. You could try
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for these improvements. Sorry for the delay in review.
The new rendering works well on z16, but it could still be better at higher zoom levels. There the points seems a little too small, especially at z18 and above, and they could still be more similar to the ways. (The best option would be to generate a way in SQL, perpendicular to the power line and use this for rendering, but that's a bit complex)
z16 comparison (left current, right after)
Also the lines in project.mml about power=insulator
can be removed now.
@flacombe are you able to update this PR with the changes requested above? To test the rendering, I'd recommend trying our setup with Docker. It's easier to work with in many cases. |
Hi all, Be sure I'm after that, a bit overloaded this week. |
I've made the changes I find necessary to this PR. Thanks for the review and useful hints to improve it. Generating ways with SQL as to make them orthogonal to the power line is a good idea, unfortunately it would take time I haven't this week :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the best rendering so far, just a small change needs to be made about the initial zoom level.
However, I'm still not certain about using the different rendering for ways and nodes. Perhaps there are other contributors who would be able to give their thoughts about this.
Example rendering:
z15 - icon shown on nodes but no rendering for ways; this needs to be fixed.
@sommerluk - what do you think about this idea?
That would allow as to use the same, linear rendering style for both ways and nodes, rather than using a different rendering for the two ways of mapping, but it would increase the complexity of the SQL. |
The good thing about doing this in SQL is that it results in nice rendering (the orientation of the portal rendering depends on the power line). However, I have no experience with such SQL queries and do not know how to do this. |
@imagico - what are your thoughts about the current test renderings?
|
The most important thing about rendering is that it needs to be intuitively understandable and that it does not create counterproductive incentives. You can do join queries to determine the intersecting power lines but you need to make sure you cover all the cases that might occur - including more than one power line intersecting the feature. I am not sure if that amount of complexity makes sense for a feature like this of relatively low overall significance. From the most recent sample rendering it seems there might be a risk of the line rendering being confused with other linear features, in particular minor service roads |
Should we just render the nodes then?
Perhaps we can consider this a situation like barrier=gate where
mapping as a linear way is not really helpful or necessary?
But the examples of linear ways for power=portal seem a little more
consistent than the situation with barrier=gate
|
Restore initial portal zoom level and make portal ways a little stronger to differ from service roads
Here I come with some SQL to generate portal ways for node ones. We've got approx 500 portal nodes and 440 of them are connected to a power line DROP TABLE IF EXISTS powerlines_bounds;
DROP TABLE IF EXISTS powerlines_dumps;
DROP TABLE IF EXISTS powerlines_portals;
CREATE TABLE IF NOT EXISTS powerlines AS select osm_id, area, z_order, way_area, tags, ST_Transform(way,3857) AS way from planet_osm_line where tags->'power'='line';
CREATE TABLE IF NOT EXISTS powerportals AS select osm_id, area, z_order, tags, ST_Transform(way,3857) AS way from planet_osm_point where tags->'power'='portal';
CREATE INDEX ON powerportals USING gist(way);
-- Create negative/positive offset geoms from line
-- Intersect and cut powerlines with portal nodes: we keep only 10% of lines that actually connect to a portal node
-- LineMerge is used to normalise continuous multilinestrings and no issue if a single line touches several portal, inner join will duplicate it.
CREATE TABLE powerlines_bounds AS WITH powerlines_geom AS (
SELECT
pl.osm_id,
ST_LineSubstring(ST_LineMerge(pl.way), GREATEST(0, ST_LineLocatePoint(ST_LineMerge(pl.way), pp.way)-0.1), LEAST(1, ST_LineLocatePoint(ST_LineMerge(pl.way), pp.way)+0.1)) AS geom,
COALESCE(pl.tags->'voltage','100000') AS voltage
FROM powerlines pl
INNER JOIN powerportals pp
ON ST_Touches(pp.way, pl.way))
SELECT
osm_id,
geom,
ST_OffsetCurve(geom, 7, 'quad_segs=4 join=mitre') AS pos_bound,
ST_Reverse(ST_OffsetCurve(geom,-7, 'quad_segs=1 join=mitre')) AS neg_bound
FROM powerlines_geom;
-- Dump points from both actual lines and boundaries
CREATE TABLE powerlines_dumps AS SELECT
osm_id,
0::bigint as portal,
(ST_DumpPoints(pos_bound)).geom AS pos_bound_dump,
(ST_DumpPoints(neg_bound)).geom AS neg_bound_dump,
(ST_DumpPoints(geom)).geom AS geom_dump
FROM powerlines_bounds;
CREATE INDEX ON powerlines_dumps using gist(geom_dump);
-- Qualify geom_dump with portal nodes. Drop anything else.
UPDATE powerlines_dumps SET portal=pp.osm_id FROM powerportals pp WHERE ST_Equals(pp.way, powerlines_dumps.geom_dump);
DELETE FROM powerlines_dumps WHERE portal < 1;
-- Draw virtual portals
-- Distinct on portal_id as to draw only one portal when several lines share the same portal
CREATE TABLE powerlines_portals AS SELECT DISTINCT ON (pld.portal)
pld.osm_id AS line_osm_id,
pld.portal AS portal_osm_id,
ST_MakeLine(pld.pos_bound_dump, pld.neg_bound_dump) AS portal_line,
ST_Azimuth(pld.pos_bound_dump, pld.neg_bound_dump) AS portal_azimuth
FROM powerlines_dumps pld; All the best |
To avoid you putting too much work into an approach that we can't use here:
|
@imagico and @sommerluk - do you see any way that this PR can move forward with rendering just the power=portal nodes (or just the ways)? |
Rendering nodes only could work, rendering ways only not - due to the numbers. In principle i would not mind a unified rendering of both nodes and ways if it conforms with the mentioned criteria. |
If we render a certain tag on ways but not on nodes it communicates to mappers that this tag is supposed to be used on ways but not on nodes. Since 80 percent of the uses of |
According to https://wiki.openstreetmap.org/wiki/Tag:power%3Dportal portals can be mapped on both nodes and ways. I spent weeks to move from nodes power=portal to power=portal ways + power=insulator on anchor points. It's a long term work. On the other hand it's not desirable to make mappers think portals are only available on nodes by render them without ways, are you? |
I'm looking at this, I see two reviews requesting changes and a number comments. Is there a route forward for this PR? |
It does not appear that we can get a good rendering for both ways and nodes. I would be willing to render the symbol only on nodes, but I don't know if @flacombe would support this idea - it appears he is in favor of mapping these features as ways in many cases. |
Hi Rendering nodes only may encourage to convert existing ways as nodes and it's not a really good thing to do. All the best |
Do you have any new cartographic ideas for rendering these features when mapped as ways? |
I'm closing this PR since it has merge conflicts and will not work in the current format. However, it's possible that a new PR would be accepted which renders the same symbol for That would be a way to render both nodes and ways (at least when the ways intersect a power=insulator node) which might be acceptable. |
Fixes #2107
Proposed rendering has been discussed there and contributors all agree. Don't forget to ask them prior to edit this PR.
Changes proposed in this pull request:
Test rendering with links to the example places:
Let's take this massive power substation : https://www.openstreetmap.org/way/110086345
Before
After