-
-
Notifications
You must be signed in to change notification settings - Fork 353
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
Make the conversion from geo::Polygon to geom::Polygon fallible #980
Changes from all commits
4dcede2
964749d
b44d1a2
f1e54c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -509,12 +509,14 @@ fn cluster_jams(map: &Map, problems: Vec<(IntersectionID, Time)>) -> Vec<(Polygo | |
} | ||
} | ||
|
||
// TODO This silently hides jams where we can't calculate the convex hull. The caller just | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's an annoying consequence of strict checks and fallible conversions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be feasible to have a convex_hull method that returns a tesselation? It seems like what would be failing is the Maybe being able to tesselate from a related? feature request: georust/geo#568 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the 3 callers of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I'm wrong, all 3 need a Polygon (well, without more work to make some operations handle either Polygon or Tessellation). Keeping this for now. I wrote a |
||
// needs a Tessellation, so should we have a less strict version of convex_hull? | ||
jams.into_iter() | ||
.map(|jam| { | ||
( | ||
map.get_i(jam.epicenter).polygon.clone(), | ||
Polygon::convex_hull(jam.all_polygons(map)), | ||
) | ||
.filter_map(|jam| { | ||
let epicenter = map.get_i(jam.epicenter).polygon.clone(); | ||
Polygon::convex_hull(jam.all_polygons(map)) | ||
.ok() | ||
.map(move |entire_shape| (epicenter, entire_shape)) | ||
}) | ||
.collect() | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,10 +73,12 @@ impl RenderCells { | |
let color = cell_color.alpha(1.0).shade(0.2); | ||
// If possible, try to erase where the cell boundary touches the perimeter road. | ||
if let Some(ref neighbourhood_boundary) = neighbourhood_boundary { | ||
batch.extend(color, boundary.difference(neighbourhood_boundary)); | ||
} else { | ||
batch.push(color, boundary); | ||
if let Ok(list) = boundary.difference(neighbourhood_boundary) { | ||
batch.extend(color, list); | ||
} | ||
continue; | ||
} | ||
batch.push(color, boundary); | ||
} | ||
} | ||
batch | ||
|
@@ -245,7 +247,12 @@ impl RenderCellsBuilder { | |
// can just clip the result. | ||
let mut clipped = Vec::new(); | ||
for p in cell_polygons { | ||
clipped.extend(p.intersection(&result.boundary_polygon)); | ||
// If clipping fails, just use the original polygon. | ||
if let Ok(list) = p.intersection(&result.boundary_polygon) { | ||
clipped.extend(list); | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's definitely up to your discretion, but do you want to log these kinds of failures? I can imagine it being tricky to track down strange geometry bugs if they're now being silently ignored, especially if they were previously crashing. If you're worried about log spam, even just a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, maybe. In this case, there will be a very loud visual bug anyway. Logs could be helpful if I ask people to include them in bug reports, but in general, it's easier to just reproduce what they're seeing |
||
clipped.push(p); | ||
} | ||
} | ||
|
||
result.polygons_per_cell.push(clipped); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,14 +181,17 @@ fn clip_map(map: &mut RawMap, timer: &mut Timer) { | |
|
||
let mut result_areas = Vec::new(); | ||
for orig_area in map.areas.drain(..) { | ||
for polygon in map | ||
// If clipping fails, giving up on some areas is fine | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A real example in Milwaukee: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it'd be great if One Day™ we added some validation rules, and maybe some heuristics for "guessing" at how to best make an existing invalid geometry valid. e.g. something like https://github.com/libgeos/geos/blob/main/src/operation/valid/MakeValid.cpp FYI there's a related long standing issue open about implementing just the validity checks: georust/geo#123 but it hasn't seen activity in a long time. |
||
if let Ok(list) = map | ||
.streets | ||
.boundary_polygon | ||
.intersection(&orig_area.polygon) | ||
{ | ||
let mut area = orig_area.clone(); | ||
area.polygon = polygon; | ||
result_areas.push(area); | ||
for polygon in list { | ||
let mut area = orig_area.clone(); | ||
area.polygon = polygon; | ||
result_areas.push(area); | ||
} | ||
} | ||
} | ||
map.areas = result_areas; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a reminder, this is the "fade irrelevant" effect. In my manual tests so far, it all still works everywhere. The fallback of not fading stuff seems fine.