Overhaul clipping #154
Overhaul clipping #154
Conversation
| "processing OSM ways split into pieces", | ||
| doc.clipped_copied_ways.len(), | ||
| ); | ||
| for (id, way) in &doc.clipped_copied_ways { |
There was a problem hiding this comment.
What's up with these? The problem is this:
We've got a way that crosses the boundary twice. In an earlier version of this PR, we got this incorrect result. Now we split this one way into 3 pieces, discard the middle one (out of bounds). And thankfully multiple ways per OSM ID don't cause any headache elsewhere, since we're already using things like opaque IDs.
| use super::Document; | ||
|
|
||
| impl Document { | ||
| // TODO This destroys the guarantee that the Document represents raw OSM. Do we need to be |
There was a problem hiding this comment.
Depending what else we do with Document, we have to be careful here. way.pts and way.nodes get out of sync. Nothing downstream uses way.nodes right now, but if that ever changes...
| self.ways.remove(id); | ||
| } | ||
|
|
||
| // TODO Handle ways that're areas |
There was a problem hiding this comment.
Longer-term, osm_reader could be split into its own crate. It's independent of the rest of osm2streets logically. But I don't want to go through all the headache of figuring out how to clip areas and relations yet, since we don't have test cases here set up to look at those
There was a problem hiding this comment.
Exactly the idea I was having. Think about it in those terms for good separation of concerns, but only implement it as needed here.
| @@ -7,22 +7,73 @@ use osm2streets::osm::{OsmID, RelationID, WayID}; | |||
|
|
|||
| use super::{Document, Relation}; | |||
|
|
|||
| pub fn get_multipolygon_members( | |||
There was a problem hiding this comment.
No real changes here, just organizing the methods onto Document
| let untrimmed_center_line = simplify_linestring(std::mem::take(&mut pts)); | ||
| match PolyLine::new(untrimmed_center_line) { | ||
| Ok(pl) => { | ||
| // TODO If either endpoint is on the boundary, we won't record any OSM ID. |
There was a problem hiding this comment.
I want to look into this as a followup. OK with getting rid of OriginalRoad entirely? A caller can look at a Road's 0 or more WayIDs and for each endpoint, 0 or more NodeIDs. There's lots of possibilities there to link to original OSM thing, and callers need to deal with it
|
Exciting results! I like the roundabout change and the reasoning. |
Also has the side effect of getting rid of handling for tiny roundabouts. 3 tests break, with clipped roads appearing on the wrong side of the boundary.
…f the PolyLine are in-bounds. #127
dabreegster
force-pushed
the
clip_earlier
branch
from
e4e811d to
8838fe0
Compare
|
Cool! I rebased and regenerated tests. Also got rid of one of the roundabout tests; |
As discussed in #127, clipping should be implemented at the OSM document level, not for
StreetNetwork. This PR makes this change. The result definitely feels simpler.3 changes as a result. The first are spurious, slight jitters of existing geometry due to using RDP simplification at a different point, or doing some geometry operations with floating point sensitivity in a slightly different order.
Second, I got rid of the tiny roundabout collapsing logic entirely. We could consider restoring it later, but it currently just papers over a real problem we should deal with in another way. The original motivation was to help the A/B Street traffic sim logic, which doesn't like vehicles on tiny roads, but oh well. Let's actually figure out how to deal with these:
Finally, we're now much more precise with clipping roads that dip in and out of a boundary. An example:
