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

Make the conversion from geo::Polygon to geom::Polygon fallible #980

Merged
merged 4 commits into from
Sep 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions apps/game/src/edit/roads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1013,11 +1013,19 @@ fn fade_irrelevant(app: &App, r: RoadID) -> GeomBatch {
}

// The convex hull illuminates a bit more of the surrounding area, looks better
let fade_area = Polygon::with_holes(
map.get_boundary_polygon().clone().into_ring(),
vec![Polygon::convex_hull(holes).into_ring()],
);
GeomBatch::from(vec![(app.cs.fade_map_dark, fade_area)])
match Polygon::convex_hull(holes) {
Ok(hole) => {
let fade_area = Polygon::with_holes(
map.get_boundary_polygon().clone().into_ring(),
vec![hole.into_ring()],
);
GeomBatch::from(vec![(app.cs.fade_map_dark, fade_area)])
}
Err(_) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot from 2022-08-30 10-15-59
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.

// Just give up and don't fade anything
GeomBatch::new()
}
}
}

fn draw_drop_position(app: &App, r: RoadID, from: usize, to: usize) -> GeomBatch {
Expand Down
30 changes: 20 additions & 10 deletions apps/game/src/edit/traffic_signals/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -944,11 +944,13 @@ fn squish_polygons_together(mut polygons: Vec<Polygon>) -> Vec<(f64, f64)> {

// Do we hit anything if we move this way?
let translated = polygons[idx].translate(pt.x(), pt.y());
if polygons
.iter()
.enumerate()
.any(|(i, p)| i != idx && !translated.intersection(p).is_empty())
{
if polygons.iter().enumerate().any(|(i, p)| {
i != idx
&& !translated
.intersection(p)
.map(|list| list.is_empty())
.unwrap_or(true)
}) {
// Stop moving this polygon
} else {
translations[idx].0 += pt.x();
Expand Down Expand Up @@ -976,9 +978,17 @@ pub fn fade_irrelevant(app: &App, members: &BTreeSet<IntersectionID>) -> GeomBat
}
}
// The convex hull illuminates a bit more of the surrounding area, looks better
let fade_area = Polygon::with_holes(
app.primary.map.get_boundary_polygon().clone().into_ring(),
vec![Polygon::convex_hull(holes).into_ring()],
);
GeomBatch::from(vec![(app.cs.fade_map_dark, fade_area)])
match Polygon::convex_hull(holes) {
Ok(hole) => {
let fade_area = Polygon::with_holes(
app.primary.map.get_boundary_polygon().clone().into_ring(),
vec![hole.into_ring()],
);
GeomBatch::from(vec![(app.cs.fade_map_dark, fade_area)])
}
Err(_) => {
// Just give up and don't fade anything
GeomBatch::new()
}
}
}
12 changes: 7 additions & 5 deletions apps/game/src/layer/traffic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an annoying consequence of strict checks and fallible conversions

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 abstreet::Polygon <-> geo::Polygon conversion, right?

Maybe being able to tesselate from a geo::Polygon -> abstreet::Tesselation rather than geo::Polygon -> abstreet::Polygon -> abstreet::Tesselation would help mitigate the issue.

related? feature request: georust/geo#568

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the 3 callers of convex_hull, they just need to tessellate, not do anything with a full polygon. I'm going to just change the API to return Tessellation or make a second call...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 impl From<geo::Polygon> for Tessellation. I'll go ahead and commit it, even though it's unused right now.

// 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()
}
Expand Down
39 changes: 27 additions & 12 deletions apps/game/src/sandbox/dashboards/commuter.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::collections::{BTreeSet, HashMap, HashSet};

use anyhow::Result;
use maplit::hashset;

use abstutil::{prettyprint_usize, Counter, MultiMap, Timer};
Expand All @@ -8,7 +9,7 @@ use map_gui::tools::checkbox_per_mode;
use map_model::{osm, BuildingID, BuildingType, IntersectionID, LaneID, Map, RoadID, TurnType};
use sim::TripInfo;
use synthpop::{TripEndpoint, TripMode};
use widgetry::tools::ColorLegend;
use widgetry::tools::{ColorLegend, PopupMsg};
use widgetry::{
Color, Drawable, EventCtx, GeomBatch, GfxCtx, HorizontalAlignment, Key, Line, Outcome, Panel,
RewriteColor, Slider, State, Text, TextExt, Toggle, VerticalAlignment, Widget,
Expand Down Expand Up @@ -73,10 +74,23 @@ type BlockID = usize;

impl CommuterPatterns {
pub fn new_state(ctx: &mut EventCtx, app: &mut App) -> Box<dyn State<App>> {
let (bldg_to_block, border_to_block, blocks) = ctx
.loading_screen("group buildings into blocks", |_, timer| {
group_bldgs(app, timer)
});
let maybe_groups = ctx.loading_screen("group buildings into blocks", |_, timer| {
group_bldgs(app, timer)
});
let (bldg_to_block, border_to_block, blocks) = match maybe_groups {
Ok(result) => result,
Err(_) => {
// TODO If concave hull fails anywhere, give up on this tool entirely. This is
// pretty harsh. Revisit this tool anyway and consider 2 bigger changes -- using
// the LTN-style blockfinding and using World. Find a way to create simpler
// fallback geometry that's still selectable.
return PopupMsg::new_state(
ctx,
"Error",
vec!["Problem rendering the grouped buildings; this tool won't work"],
);
}
};

let mut trips_from_block: Vec<Vec<TripInfo>> = std::iter::repeat_with(Vec::new)
.take(blocks.len())
Expand Down Expand Up @@ -458,16 +472,16 @@ impl State<App> for CommuterPatterns {
fn group_bldgs(
app: &App,
timer: &mut Timer,
) -> (
) -> Result<(
HashMap<BuildingID, BlockID>,
HashMap<IntersectionID, BlockID>,
Vec<Block>,
) {
)> {
let mut bldg_to_block = HashMap::new();
let mut blocks = Vec::new();

let map = &app.primary.map;
for block in timer.parallelize(
for maybe_block in timer.parallelize(
"draw neighborhoods",
partition_sidewalk_loops(app)
.into_iter()
Expand All @@ -493,14 +507,15 @@ fn group_bldgs(
hull_points.append(&mut lane_line.points().clone());
}
}
Block {
Polygon::concave_hull(hull_points, 10).map(|shape| Block {
id: block_id,
bldgs: group.bldgs,
borders: HashSet::new(),
shape: Polygon::concave_hull(hull_points, 10),
}
shape,
})
},
) {
let block = maybe_block?;
for b in &block.bldgs {
bldg_to_block.insert(*b, block.id);
}
Expand Down Expand Up @@ -535,7 +550,7 @@ fn group_bldgs(
});
}

(bldg_to_block, border_to_block, blocks)
Ok((bldg_to_block, border_to_block, blocks))
}

enum BorderType {
Expand Down
15 changes: 11 additions & 4 deletions apps/ltn/src/draw_cells.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 debug! (or trace!) message which normal users wouldn't see might be helpful if you're actively trying to understand strange behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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);
Expand Down
6 changes: 4 additions & 2 deletions apps/parking_mapper/src/mapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,8 @@ fn find_overlapping_stuff(app: &App, timer: &mut Timer) -> Vec<Polygon> {
if !b
.polygon
.intersection(&map.get_r(r).get_thick_polygon())
.is_empty()
.map(|list| list.is_empty())
.unwrap_or(true)
{
polygons.push(b.polygon.clone());
}
Expand All @@ -659,7 +660,8 @@ fn find_overlapping_stuff(app: &App, timer: &mut Timer) -> Vec<Polygon> {
if !pl
.polygon
.intersection(&map.get_r(r).get_thick_polygon())
.is_empty()
.map(|list| list.is_empty())
.unwrap_or(true)
{
polygons.push(pl.polygon.clone());
}
Expand Down
11 changes: 7 additions & 4 deletions convert_osm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A real example in Milwaukee:
Screenshot from 2022-08-30 09-50-18
The red polygon disappears with this PR. For some reason, clipping https://www.openstreetmap.org/relation/6041452#map=17/43.05795/-87.87511 to the map's boundary produces an invalid ring somewhere. But as you can see from the "before" screenshot above, the final result used to work fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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;
Expand Down
Loading