Fix way-section identifier bug for closed loops #187
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
The original issue was reported by @jklamer - lots of benign
MultiAtlasBorderFixerlogs failing to fix conflicts (see below for sample output) when stitching together raw atlas shards. I dug into this and found a corner case in identifier assignment for way-sectioned edges that had a loop as part of the underlying PolyLine. Specifically, we would assign 000 as one of the identifiers for the Edge, even though the underlying way was being way-sectioned. This would trigger theMultiAtlasBorderFixerto attempt an edge fix, specifically triggering thehasInconsistentIdentifier()method to return inconsistencies and begin the fix process.Sample Error in Logs:
2018-07-25 13:47:13 WARN [Executor task launch worker-0] MultiAtlasBorderFixer:288 - OSM way 201288149 fix for [[TemporaryRoad: subAtlas = CUB_7-35-55.atlas, osm = 201288149000, members = [Edge: id=201288149000000, startNode=2112447504000000, endNode=2112447504000000, polyLine=LINESTRING (-81.4222758 22.9527279, -81.4223432 22.952432, -81.4223722 22.9522485, -81.4223175 22.9521124, -81.4222147 22.951926, -81.4222147 22.9517869, -81.422385 22.9517248, -81.4225521 22.9517366, -81.4226999 22.9518402, -81.4229859 22.9519378, -81.4231434 22.9518964, -81.4231466 22.9517603, -81.4231016 22.951568, -81.4230116 22.9513608, -81.4230181 22.9512306, -81.423108 22.9511892, -81.4232526 22.9512158, -81.4233715 22.9513076, -81.4234615 22.951562, -81.4234936 22.951852, -81.4235451 22.9521686, -81.4235129 22.9523196, -81.4232334 22.9524616, -81.4230598 22.9525089, -81.4229217 22.9525888, -81.4229666 22.9528019, -81.4230759 22.9529972, -81.4231241 22.9531777, -81.4230116 22.9532664, -81.4228895 22.9532428, -81.4227578 22.953154, -81.422581 22.9530149, -81.4224075 22.9528463, -81.4222758 22.9527279), [Tags: [last_edit_user_name => Bilbo-denmark], [last_edit_changeset => 14662455], [last_edit_time => 1358265285000], [last_edit_user_id => 295005], [iso_country_code => CUB], [highway => service], [last_edit_version => 1]]] [Edge: id=201288149000001, startNode=2112447568000000, endNode=2112447504000000, polyLine=LINESTRING (-81.4215302 22.9537931, -81.4215302 22.9535564, -81.4216973 22.9534292, -81.421903 22.9534706, -81.4221183 22.9535623, -81.4222468 22.953586, -81.4223047 22.9534943, -81.4222276 22.9532339, -81.422144 22.9530475, -81.4222758 22.9527279), [Tags: [last_edit_user_name => Bilbo-denmark], [last_edit_changeset => 14662455], [last_edit_time => 1358265285000], [last_edit_user_id => 295005], [iso_country_code => CUB], [highway => service], [last_edit_version => 1]]] ]] is failed. org.openstreetmap.atlas.exception.CoreException: Found end first! POINT (-81.4215302 22.9537931)(occurrence 0) and POINT (-81.4222758 22.9527279)(occurrence 0) are not in order with respect to LINESTRING (-81.4222758 22.9527279, -81.4223432 22.952432, -81.4223722 22.9522485, -81.4223175 22.9521124, -81.4222147 22.951926, -81.4222147 22.9517869, -81.422385 22.9517248, -81.4225521 22.9517366, -81.4226999 22.9518402, -81.4229859 22.9519378, -81.4231434 22.9518964, -81.4231466 22.9517603, -81.4231016 22.951568, -81.4230116 22.9513608, -81.4230181 22.9512306, -81.423108 22.9511892, -81.4232526 22.9512158, -81.4233715 22.9513076, -81.4234615 22.951562, -81.4234936 22.951852, -81.4235451 22.9521686, -81.4235129 22.9523196, -81.4232334 22.9524616, -81.4230598 22.9525089, -81.4229217 22.9525888, -81.4229666 22.9528019, -81.4230759 22.9529972, -81.4231241 22.9531777, -81.4230116 22.9532664, -81.4228895 22.9532428, -81.4227578 22.953154, -81.422581 22.9530149, -81.4224075 22.9528463, -81.4222758 22.9527279, -81.4215302 22.9535564, -81.4216973 22.9534292, -81.421903 22.9534706, -81.4221183 22.9535623, -81.4222468 22.953586, -81.4223047 22.9534943, -81.4222276 22.9532339, -81.422144 22.9530475, -81.4222758 22.9527279) at org.openstreetmap.atlas.geography.PolyLine.between(PolyLine.java:302) at org.openstreetmap.atlas.geography.atlas.multi.MultiAtlasBorderFixer.fixBorderIssues(MultiAtlasBorderFixer.java:212) at org.openstreetmap.atlas.geography.atlas.multi.MultiAtlas.<init>(MultiAtlas.java:418) at org.openstreetmap.atlas.geography.atlas.multi.MultiAtlas.<init>(MultiAtlas.java:267) at org.openstreetmap.atlas.geography.atlas.multi.MultiAtlas.loadFromPackedAtlas(MultiAtlas.java:165) at org.openstreetmap.atlas.geography.atlas.multi.MultiAtlas.loadFromPackedAtlas(MultiAtlas.java:138) at org.openstreetmap.atlas.geography.atlas.AtlasResourceLoader.load(AtlasResourceLoader.java:90) at ...Potential Impact:
This was a benign error, as no actual fixes took place or were necessary (because of the consistent identifiers). However, the resulting output clogged up the logs and potentially increased runtime, as an attempt to fix the underlying PolyLines was made for multiple Edges within an Atlas.
Unit Test Approach:
I've updated the existing unit tests that handle loop scenarios to explicitly check identifier correctness. Without the fix, these would fail.
Test Results:
I verified the fix in several ways:
I set a conditional break-point in the
MultiAtlasBorderFixerto trigger when attempting to see if a fix was required for the first Edge in the output logs. Before the fix, the two edges for this OSM Identifier were:This caused an attempted fix, since an identifier ending with 000 implies no way-sectioning took place, but the presence of multiple edges for a single OSM Identifier implies way-sectioning did take place, prompting an attempted fix. Here is the output after the fix - which correctly resulted in no necessary fix:
Below are the two edges created in the pre-fix atlas and the same two-edge in the post-fix atlas. You'll notice that the geometry and tags are identical, but the identifier is now correct. Because of way-sectioning, the identifiers changes from 000 and 001 to 001 and 002.
Original Atlas - first Edge:
Original Atlas - second Edge:
New Atlas - first Edge:
New Atlas - second Edge:
MultiAtlas- I constructed two Cuba shards with the fix and combined them into aMultiAtlas, theMultiAtlasBorderFixeroutput was no longer present, but the data was identical across the MultiAtlases. I leveragedAtlasDeltato compare old and new multi-atlas and the only difference was the removal of the 000 edges.In doubt: Contributing Guidelines