Skip to content
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

roof:ridge and rood:edge do not work properly when crossing building:part boundries #170

Open
GoodClover opened this issue Sep 1, 2020 · 3 comments

Comments

@GoodClover
Copy link

Pre-note

When I mention r/e I mean roof:ridge/roof:edge.

I may not have explained the best here, seeing the Discord chat may help.
If you have any questions, ask away! I did have a look at the source code and can probably point to where the issue is if I have a longer look.

Images and explanation of issue

So if an r/e crosses over a building:parts edge, it gets ignored:
ignored rood:ridge

Just making a node on the r/e where it intersects the change in building:part fixes the issue.
fixed via node JOSM

Here is a helpful diagram: (this is on dANW34V3R:master iirc so it's slightly different to above, the r/e does appear in the building:part it started in, but not the others.
On tordnik:master it doesn't appear in either)
very nice diagram
Part 1 of the above at a different angle:
part 1 of diagram angle

How to fix

This appears to be an issue with the algorithm that detects whether a way is inside a building part, it doesn't always detect it if it only 1 node is inside of it/on it's edge.
If 2 nodes are inside the building part/on the edge it sees it fine, so it works if a node is on the boundary so both building parts see it.

Discussion of this issue happened on Discord

Here's a Discord link to the conversation, where there is probably better and more explained detail than here:
https://discordapp.com/channels/413070382636072960/429092644438802432/750146965597061140
(You need to be in the OSM World Discord for the link to work).

@GoodClover
Copy link
Author

for (MapOverlap<?,?> overlap : area.getOverlaps()) {

It seems to be the area.getOverlaps() function that is the culprit, but I could be wrong I do not fully understand your code.

@tordanik
Copy link
Owner

tordanik commented Sep 1, 2020

The current code is written under the assumption that all r/e segments are completely contained within the building part polygon, yes. Is it valid for a ridge to cross a building part boundary? The documentation for ProposedRoofLines doesn't really say much on building parts, but as far as I can see, all of the examples have outlines fully enclosing the r/e segments.

To change this assumption would probably involve at least:

  • changing the "inside" check. This currently tests if the center of the line segment is inside the polygon. (I believe this is why it sometimes works with the left building part in your example, but not the right one: The left one contains the center of the line.)
  • making sure the triangulation of the roof polygon in the HeightfieldRoof superclass still works if an "inner segment" extends beyond the polygon that will be triangulated. This could be done by doing a special case handling for this and splitting the segment at the intersection with the outline (that is, automatically performing the fix you achieved by inserting a shared node).

At least in my own test (which may be subtly different from yours), getOverlaps did successfully detect the overlap between the ridge and both building parts, so that part of the code could stay as it currently is. Could you send me an .osm file so I can make sure?

All in all, it could be done with a bit of effort; I'd say the priority of this fix depends on whether the community considers this valid mapping. (Even if it's not, it wouldn't hurt for OSM2World to be able to deal with it, of course.)

Thanks for the very thorough illustrations by the way! :)

@GoodClover
Copy link
Author

GoodClover commented Sep 1, 2020

Here is a file (fictional data) showing some of the issues, left building (with 1 floor) is virtually identical to what I was talking about above.
GithubGist/GoodClover/fictional_houses.osm
This is different to my original test, but it is close enough.

In my opinion having the roof lines cross between building:parts is acceptable, and should be dealt with by software,
but doesn't make sense and is incorrect mapping (I can't explain this very well, apologies).

As you can see with the last of the building, the correct way was different anyways and nothing crossed a building:part boundary. If it crosses over a boundary, that means something about the way the building works has changed, a roof line wouldn't just carry across, there should be a node where it changed.
It can always be assumed that if there isn't a node, one should be at the intersection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants