Adding boundary tag to nodes on ways that span boundaries #8
Conversation
There was a problem hiding this comment.
Nice and clean, thanks @matthieun! One question and minor nit.
| @@ -694,11 +699,39 @@ private void keepOutsideWaysThatAreConnected() | |||
| { | |||
| if (!this.store.containsWay(way.getId())) | |||
| { | |||
| final List<WayNode> wayNodes = way.getWayNodes(); | |||
| for (final WayNode wayNode : way.getWayNodes()) | |||
There was a problem hiding this comment.
Nit: you use wayNodes in the for:each loop, since you extracted the variable out.
There was a problem hiding this comment.
Good catch, thanks!
| for (final WayNode wayNode : way.getWayNodes()) | ||
| { | ||
| final long identifier = wayNode.getNodeId(); | ||
| if (this.store.containsNodeAtEndOfEdges(identifier)) | ||
| { | ||
| final Long startNodeIdentifier = wayNodes.get(0).getNodeId(); |
There was a problem hiding this comment.
Any crazy chance that wayNodes are empty?
There was a problem hiding this comment.
This is within the for loop that loops through all the wayNodes, so if it is empty, it would not reach the body of the for loop.
| @@ -99,4 +112,22 @@ public void testNoFilter() | |||
| logger.info("{}", atlas); | |||
| Assert.assertEquals(3, atlas.numberOfLines()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testOutsideWayBoundaryNodes() | |||
| || containsNodeInRelations(node.getId()); | ||
| if (containsNodeInRelations(node.getId())) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
So if an OSM node is part of a relation and it is at the end of an edge, then it would be have one node counterpart in Atlas and one point counterpart. Is that intentional?
There was a problem hiding this comment.
Plus, what is the reason we make an OSM node an Atlas point because it is part of a relation?
There was a problem hiding this comment.
I am not sure why this was designed this way. I figured for this PR I would not touch that part of the logic, but you are correct this can be questionable... Created #10 to track it.
There was a problem hiding this comment.
Cool. It can definitely be addressed later.
There was a problem hiding this comment.
I don't remember why we need this check, it doesn't seem correct. Agree to try to remove this later. Thanks!
| return true; | ||
| } | ||
| // Tags from OSM are the tags that all the nodes will have | ||
| if (node.getTags().size() > AtlasTag.TAGS_FROM_OSM.size()) |
There was a problem hiding this comment.
This logic behind needs some explanation and seems easy to break. I'd suggest adding some comments and explaining why it makes a node an Atlas point.
There was a problem hiding this comment.
Created #11 as well. Again for this PR I did not want to change the logic here, but that deserves a refactor.
I will add more comments as well in the mean time.
There was a problem hiding this comment.
The design was any OSM node with artificial tag will be promoted as an Atlas Point.
So here it is checking if the OSM node has artificial tag other than those default tags from OSM (like LastEditTimeTag etc.).
When a way is not assigned any country code (it is not within a country boundary) but it is still connected to ways that are inside the boundary (long bridges, ferry routes for example), the end nodes of those ways were not assigned any
synthetic_boundary_nodetag. This PR adds the tag to those nodes.It also refactors the function that decides when an OSM node becomes an Atlas
Point. That allows to take the new tags into account, and to avoid building morePoints than necessary.