-
Notifications
You must be signed in to change notification settings - Fork 1.3k
add validator about power lines crossing other lines #11200
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
base: develop
Are you sure you want to change the base?
Conversation
|
I completely second this contribution. We highly need this. Thank you @nlehuby! |
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.
Thanks for taking the time to implement this feature, and for your patience.
modules/validations/crossing_ways.js
Outdated
| if (hasTag(tags, 'railway') && osmRailwayTrackTagValues[tags.railway]) return 'railway'; | ||
| if (hasTag(tags, 'waterway') && osmFlowingWaterwayTagValues[tags.waterway]) return 'waterway'; | ||
|
|
||
| if (hasTag(tags, 'power') && tags.power === 'line') return 'power'; |
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.
Like power=line, power=minor_line and power=cable shouldn’t be connected to roadways and land use areas either. However, if we add power=minor_line, we’ll need to add an exception for connections to buildings and possibly other features.
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.
We should consider adding power=terminal on the connecting node with the building.
Other validators like Osmose will warn for an unfinished power line here
By te way that's really cool to map service supplies
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.
Good idea, I didn’t know about that tag. Looks like openstreetmap/id-tagging-schema#653 is tracking adding a preset for power=terminal. Let’s wait until that happens before extending this validation rule to power=minor_line. Then we’ll be able to offer a second option to fix the warning by adding power=terminal to the connecting node. That would get us a little closer to being able to warn about islands in the power network as well.
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.
I've also created the PR to add the power=terminal preset : openstreetmap/id-tagging-schema#1637
modules/validations/crossing_ways.js
Outdated
| title: t.append('issues.fix.delete_connecting_node.title'), | ||
| entityIds: [this.data.crossingNode], | ||
| onClick: function(context) { | ||
| var operation = operationDelete(context, [this.issue.data.crossingNode]); |
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 option should disconnect the power line, not delete the connecting node. The connecting node could be an intersection between two roadways or a node that defines the shape of a landuse area. Deleting the node could damage the other connected features’ geometry or topology.
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.
Indeed, you are right.
I've switched to operationDisconnect (instead of operationDelete).
It's a bit annoying because I do think in most case, we want to keep the node in the other feature but we do want to delete the node on the power line... but I didn't manage to do that.
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.
Yes, that is unfortunate. Maybe we could ticket out a followup improvement that warns about an extraneous vertex along a power line, especially if it causes a kink in the line. It could offer to straighten the line (by removing the node) or add a tower or pole in case it’s missing. This warning might need some extra care, because sometimes the line can branch without a pole, as I see frequently when mapping residential service drops.
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.
For the last point about mid span branches, here are two facts:
- It doesn't make a kink the line, the branch occurs in straight line between towers
- Mappers must use power=connection + line_management=branch on the node, so there is always
poweron every nodes of 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.
It doesn't make a kink the line, the branch occurs in straight line between towers
What I mean is that power lines should be straight between towers, but some are very crooked, including most untouched TIGER-imported power lines but also potentially some lines where users have inadvertently added an errant node along the way. We could have a separate validation rule about such issues.
Mappers must use power=connection + line_management=branch on the node, so there is always
poweron every nodes of lines.
This isn’t guaranteed to be the case before the user clicks on the option to delete the node. We’d need to add another option for adding a power=connection line_management=branch node to prevent the user from damaging the existing features (quite possibly deleting one of the ways altogether). Moreover, id-tagging-schema has no preset for power=connection yet, so we can’t expect the user to know about that prerequisite.
|
When seeking for non-power features a power line could be connected to, we should take care of life cycle prefixes. That requires to consider :power= and power=* as power features (disused:power=line shouldn't connect to roads or landuse as well) |
- handle minor_line & cable - improve wording - allow building-power connexion, with power=terminal
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 looks good now, but we should wait until openstreetmap/id-tagging-schema#1637 lands so that the fix suggestions use supported tags.
modules/validations/crossing_ways.js
Outdated
| title: t.append('issues.fix.delete_connecting_node.title'), | ||
| entityIds: [this.data.crossingNode], | ||
| onClick: function(context) { | ||
| var operation = operationDelete(context, [this.issue.data.crossingNode]); |
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.
Yes, that is unfortunate. Maybe we could ticket out a followup improvement that warns about an extraneous vertex along a power line, especially if it causes a kink in the line. It could offer to straighten the line (by removing the node) or add a tower or pole in case it’s missing. This warning might need some extra care, because sometimes the line can branch without a pole, as I see frequently when mapping residential service drops.
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.
By the way, the warning’s title says the power line “crosses” the building, whereas “connects to” would be more accurate. It isn’t a big deal, but maybe the code already allows us to customize the language straightforwardly.
Co-authored-by: Minh Nguyễn <[email protected]>
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, sorry for the long wait in getting this reviewed. I noticed some things while trying out the branch and have a few general questions/remarks:
🐛 bug when terminal node is already is tagged
When the connection to a building already has tags (e.g. a simple note), the fix adds a second vertex at the same coordinates, see below:
Screencast.From.2025-09-02.16-32-19.mp4
In such cases this specific fix should either be not offered. Or, if it can be done safely, the fix should merge the new tags onto the existing node
compatibility with (proposed) preset
openstreetmap/id-tagging-schema#1637 seeks to add power=terminal as a preset, which (currently) has line_attachment=anchor as addTags. The fix in this PR does not add line_attachment, so the user would get a second validation warning about the incompleteness of the node's tags right after applying the fix. This could be addressed by adding the line_attachment tag here as well, or by removing line_attachment from the preset's addTags. (This is probably best to be discussed in the id-tagging-schema PR.)
interaction with layer tag on building
When a building is tagged with e.g. layer=1, the validation warning is not displayed. I assume this is not intentional, but rather a consequence of adding the logic in to crossing_ways which already handles layer tags for the detection of other data quality issues, and this resulted in an unwanted side effect?
In fact, it might be cleaner to extract this validation into a separate module instead of making the crossing_ways validation even more complex than it already is. Any reusable logic could be refactored out into a util library or simply be shared between the two modules directly.
power=terminal on non-end nodes
Is it reasonable to assume power=terminal can also be used on nodes that are not a start or end point of a way? The validation fix will happily offer to add the tag also to middle vertices. I have only ever seen it used on the extreme points of a way, never as a middle vertex, but maybe that's a false extrapolation of mine.
order of offered fixes
For example, when a power line connects to a building, but continues straight on, it was probably most likely the case that the mapper accidentally snapped the building to a power line (or vice versa). In this case, the first offered fix to add a power terminal is probably not the best to use by an inexperienced user to fix the issue.
Maybe the order of the offered fixes could be optimized to always show the most appropriate fix first. See #11329 for a general discussion around this topic.
other tags that can allow power line – building connection
From looking at the wiki, I had the impression that power=insulator could technically also be used to correctly tag a connection of a building with a power line. Especially in the case where the way itself does not end at that point, but it e.g. is rather a part of the power line's support structures, that might be even the only correct way to tag such situations.
Are there maybe even more tags that would allow connecting a power line to a non-power feature?
other features power lines should not connect to
The validation will only warns when power lines are connected to roads or buildings. But a similarly invalid case would be when e.g. a power line is connected to other structures (e.g. man_made=bridge, natural=water, landuse=meadow, etc.). Would it not make sense to extend this validator to also take those into account? Actually, as power features apparently should only interact with other map features at very specific features with well-documented tags, would it not make sense to warn on all occasions where the connection is between a power line and a non-power feature? Or would that lead in too many false positives?
"disconnect" fix results in two nodes at the exact same coordinate
When using the fix to disconnect the features, the result is two nodes with exact same coordinates. That is not ideal for multiple reasons (e.g. it is easy to accidentally snap the nodes together again, it will be flagged by many QA tools, data consumers that only look at the coordinates of the features might still end up with a false connection at that point, etc.). For the normal disconnect operation this is not a problem, as the mapper is already interacting with the particular intersection node and will likely follow up the disconnect operation by a move operation to reorganize the nodes appropriately. But that is not the case when the disconnect is the result of a validation fix.
Maybe an approach like ome ideas to approach this better:
- If power line is almost straight at the to disconnect point: instead offer to remove the node on the power line (the node should remain only on the highway or building).
- Otherwise this means that the power line has a kink at that location, which is strange, as power lines typically can only bend at a power pole or tower (or insulator/portal/terminal). In that case, the best fix might be to offer to create a new power pole or tower next to the intersecting feature. Maybe that's a little overkill, so instead the following could be an alternative:
- Move the disconnected node of the power line a little bit to the side of the road or building.
extendability to non-power features
Some other OSM features can also benefit from a validation that checks for suspicious connections, e.g. path and address interpolation (#9589), highway and landuse (#6631), highway and administrative boundary, etc.
It would be awesome if the implementation in this PR could be flexible enough to support for easy extension to other feature combinations like the ones mentioned in the future.
|
Thanks for the extensive review. I started reworking the code to create a dedicated module for validating incorrectly connected ways. I fixed the following remarks along the way
Now comes the difficult part:
Knowing that I haven't managed to have the fix to delete the node on the power line while keeping it on the other feature 😞 |
2891a91 to
157c865
Compare
reuse the node instead of creating another one in the same position
only if last node of way
to remove the node from the power feature
13d2906 to
c8a0141
Compare
|
I've updated the fixes:
Apart from this suggestion, I believe I have addressed all the comments made in the various reviews :) |
|
Hello, Would it be possible to have another code review on this proposal? I would very much like to see this in iD. |
This PR add a new check in the validators about power lines connected to non power feature.
Fix #8446 & #9488
I don't have much experience with javascript development, so please let me know what can be improved.