hootenanny icon indicating copy to clipboard operation
hootenanny copied to clipboard

Ref conflate doesn't handle situations where secondary road data has footway=crossing sharing nodes with roads

Open bwitham opened this issue 4 years ago • 12 comments

Found while working on #3895. Due to the crossing sharing a node with the road. I have no idea yet how to fix this. The crossings get snapped to the new intersection of the merge roads. See test-files/cases/reference/unifying/highway/sidewalk-3895

bwitham avatar Mar 11 '20 18:03 bwitham

Have found other work in the meantime before starting this that I think is more important.

Did some cleanup of the railway schema, similar to what was done last week with the waterway schema, which should help runtime performance out.

Seeing some painfully slow processing with this data and collection relation matching, primarily for very large route and waterway relations...EdgeDistanceExtractor is just too slow for them. Going to try to find a better performing extractor to use on large relations of those types.

bwitham avatar Apr 14 '20 20:04 bwitham

Back on this.. Fortunately, there's already a test from #3895 with an example of this. I'm also noticing that secondary highway=crossing nodes are unsnapped in the output, despite the fact that #3922 should have fixed that.

bwitham avatar Apr 16 '20 15:04 bwitham

The orphaning of the crossing node actually happens before WayNodeCopier from #3922 kicks in during the snapping of the ends. Seeing if I can preserve it in that function.

bwitham avatar Apr 16 '20 16:04 bwitham

Fixed the crossing node issue with a HighwaySnapMerger node copying during end snapping and an update to the SuperfluousNodeRemover unallowed orphans list. lots of test failures, though...

bwitham avatar Apr 16 '20 17:04 bwitham

Many of the test failures have been due to improvements so far and the output has been updated. A couple of the tests now have some strangely bad output that I'm looking at.

bwitham avatar Apr 17 '20 17:04 bwitham

Actually, the bad output was pre-existing and looks to be caused by the collection relation merging. Very strangely, railway crossing nodes are added to some buildings and other roads. Some ids must be getting mixed up somewhere.

bwitham avatar Apr 17 '20 18:04 bwitham

I think the bad output is caused by railway conflation. Still trying to verify. Even though I fixed the original problem with footways that I set out to fix, I'm going to go ahead and merge the snapped node tag fixes and then come back to these other two problems.

bwitham avatar Apr 20 '20 13:04 bwitham

The bad output ended up being caused by WayNodeCopier allowing copying of nodes from relations, which it shouldn't. Correcting that fixes the problem. Will merge changes so far and circle back around to the footway problem laster.

bwitham avatar Apr 20 '20 18:04 bwitham

Actually have one POI regression test failing as a result of this...should have waited a little longer to merge. Number one suspect would be the changes to SuperflousNodeRemover but haven't verified that yet. Since some of that regression testing input data gets artificially filtered down to POIs by custom scripts (orphaned POI nodes created that wouldn't ever exist in normal OSM), may have to add an option for this test only to not remove such nodes in the output.

bwitham avatar Apr 21 '20 14:04 bwitham

Was able to fix the test by disabling the orphaned node removal by kvp for it only.

bwitham avatar Apr 21 '20 15:04 bwitham

Worth noting that the Network alg doesn't do the bad footway snapping but it also fails to connect a portion of sidewalk at the intersection...so still worth fixing this in Unifying I think. I may be able to add in some custom handling to disconnect the node being snapped on the residential road to prevent the bad footway snapping. The problem is that the residential intersection will get the crossing node based on recent secondary info node retention changes when it really shouldn't. Need to think more about how to prevent that from happening.

bwitham avatar Apr 21 '20 20:04 bwitham

Going to have to bail on this one for awhile. I came up with a partially working solution pushed to the 3901 branch but just can't come up with anything completely solid. Will leave the issue open for awhile in case I get any ideas in the future. While it would have been nice to finish this, for this particular data, the secondary is wholly superior to the reference, so probably a replacement would be done in this situation anyway.

bwitham avatar Apr 23 '20 17:04 bwitham