Make data preprocessing more error-tolerant, rewrite combineWays

[deevroman]

Fix #80 #83

1. I added downloading of ways for which there are no nodes in FullData (augmentedNodelist, augmentedWays)
2. I added a selector that checks that the members of the relation are ways
   3. I rewrote combineWays() to get rid of freezing. Now use recursive search similar to DFS.

All in all, we can finally render this!

https://deevroman.github.io/OSMBuilding/?id=419266377

[Beakerboy]

I love that this gets everything working! I have a question about:

I added downloading of ways for which there are no nodes in FullData (augmentedNodelist, augmentedWays)

Are you certain this extra download step is necessary? When I looked at it, I thought there was a nearby building, and one of the parts for this other building was being analyzed. Because some ways of this other building were outside the bounding box of this relation, they were not being downloaded. I thought the best solution would be to examine the initial set of downloaded ways, and if the part does not have any points which exist inside the building of interest, discard that building part. If I've misunderstood the issue, could you explain your best guess as to why the initial download fail to pull in all the requires relations, nodes, and ways?

[Beakerboy]

I've added a few tests to my original code, and it looks like a couple are failing with your code. Would you mind checking that? It might be easier to split this up into a couple pull requests. The CombineWays is an obvious improvement. Can you spin this into a separate pull request and include a test case that fails under my code but passes under yours?
I imagine something like this will work for your code:

test('Test combining 3 out of 4 ways', () => {
  var way1 = '<way id="1"><nd ref="1"/><nd ref="2"/></way>';
  var way2 = '<way id="2"><nd ref="3"/><nd ref="2"/></way>';
  var way3 = '<way id="3"><nd ref="3"/><nd ref="1"/></way>';
  var way4 = '<way id="4"><nd ref="1"/><nd ref="4"/></way>';
  let parser = new window.DOMParser();
  let xml1 = parser.parseFromString(way1, 'text/xml').getElementsByTagName('way')[0];
  let xml2 = parser.parseFromString(way2, 'text/xml').getElementsByTagName('way')[0];
  let xml3 = parser.parseFromString(way3, 'text/xml').getElementsByTagName('way')[0];
  let xml4 = parser.parseFromString(way3, 'text/xml').getElementsByTagName('way')[0];
  let result = BuildingShapeUtils.combineWays([xml1, xml2, xml3, xml4]);
  expect(result.length).toBe(1);
  expect(BuildingShapeUtils.isClosed(result[0])).toBe(true);
  expect(BuildingShapeUtils.isSelfIntersecting(result[0])).toBe(false);
  expect(result[0].getElementsByTagName('nd').length).toBe(4);
});

The current function has 100% code coverage, would you able to provide coverage for your improved function so we can be sure everything works right? I need to transition all these separate tests into a test.each() format.

Thanks for all your help!

[deevroman]

Can you spin this into a separate pull request and include a test case that fails under my code but passes under yours?

I'll try adding this example #83 (comment)

---

This was partly mentioned here #42

Add to this, the standard says that “building-parts must fill in the entire building” but parts actually CAN extend outside the “outline”.
I think what I need to do is, inspect each way and if no node falls inside the building, discard it. If a node falls on the border, a calculation would have to be done to see if the border of this way crosses through some part of the outer building.

It seems that to correctly calculate intersections, you will have to have a full multipolygon and you will still have to load the missing ways/nodes.

why the initial download fail to pull in all the requires relations, nodes, and ways

building:part can be a complex multipolygon with holes. /map loads the nodes in the bbox + all parent ways + relations, but without the relation members. So you can load only the outer lines of a building part and the current code will at best draw it, and at worst crash.

---

I'll fix the tests

[Beakerboy]

building:part can be a complex multipolygon with holes. /map loads the nodes in the bbox + all parent ways + relations, but without the relation members. So you can load only the outer lines of a building part and the current code will at best draw it, and at worst crash.

I got it. I think in the specific case in front of us though, the issue is not that parts are extending outside the building outline, it's that neighboring parts are being analyzed. I cannot think of a situation where no nodes for a building part would be within the outline. Based on your description, there still could be a rare case where some parts need to be re-queried.

[deevroman]

I'm afraid that where mappers draw in 3D, the probability of encountering this increases :)

upd: I'll ping you when the new tests are ready

[Beakerboy]

If no nodes for a building part reside within the building of interest, it certainly makes me think it’s more likely a part of some other building. Is the expectation that the bounding box be increased and that other building be identified? I’m a fan of tackling the simpler problems first. If you look at the Chrysler Building on the examples page, you see some other neighboring buildings that are rendered. Eliminating this is already a necessity, and will fix the issue in front of us as well without the extra complexity. If you can find an example of a building with a part outside of the outline, then I think it’s worth adding that. Acknowledging that such a situation could exist and logging an error would be good.

[deevroman]

There seems to be a potential example in the first post of this PR. Debugging...