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

Get rid of geom::Polygon union operations. They always produced invalid #981

Merged
merged 2 commits into from
Aug 30, 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
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions apps/game/src/info/intersection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,11 +238,11 @@ fn current_demand_body(ctx: &mut EventCtx, app: &App, id: IntersectionID) -> Wid
batch,
tooltips,
Box::new(|arrow| {
let mut list = vec![(Color::hex("#EE702E"), arrow.clone())];
let mut batch = GeomBatch::from(vec![(Color::hex("#EE702E"), arrow.clone())]);
if let Ok(p) = arrow.to_outline(Distance::meters(1.0)) {
list.push((Color::WHITE, p));
batch.push(Color::WHITE, p);
}
GeomBatch::from(list)
batch
}),
),
])
Expand Down
20 changes: 12 additions & 8 deletions apps/game/src/ungap/trip/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,14 @@ impl TripPlanner {

let main_route = RouteDetails::main_route(ctx, app, self.waypoints.get_waypoints());
self.main_route = main_route.details;
world
.add(ID::MainRoute)
.hitbox(main_route.hitbox)
.zorder(1)
.draw(main_route.draw)
.build(ctx);
if self.waypoints.get_waypoints().len() > 1 {
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 15-42-46
For routes with >= 3 waypoints, there are multiple hitboxes per route object in the World. It was most straightforward to just make World handle multiple hitboxes.

world
.add(ID::MainRoute)
.hitboxes(main_route.hitboxes)
.zorder(1)
.draw(main_route.draw)
.build(ctx);
}

self.files.autosave(app);
// This doesn't depend on the alt routes, so just do it here
Expand All @@ -107,7 +109,9 @@ impl TripPlanner {
avoid_stressful_roads: true,
},
] {
if app.session.routing_preferences == preferences {
if app.session.routing_preferences == preferences
|| self.waypoints.get_waypoints().len() < 2
{
continue;
}
let mut alt = RouteDetails::alt_route(
Expand All @@ -124,7 +128,7 @@ impl TripPlanner {
self.alt_routes.push(alt.details);
world
.add(ID::AltRoute(self.alt_routes.len() - 1))
.hitbox(alt.hitbox)
.hitboxes(alt.hitboxes)
.zorder(0)
.draw(alt.draw)
.hover_alpha(0.8)
Expand Down
13 changes: 4 additions & 9 deletions apps/game/src/ungap/trip/results.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub struct BuiltRoute {
pub details: RouteDetails,
pub details_widget: Widget,
pub draw: ToggleZoomedBuilder,
pub hitbox: Polygon,
pub hitboxes: Vec<Polygon>,
pub tooltip_for_alt: Option<Text>,
}

Expand Down Expand Up @@ -99,7 +99,7 @@ impl RouteDetails {
preferences: RoutingPreferences,
) -> BuiltRoute {
let mut draw_route = ToggleZoomed::builder();
let mut hitbox_pieces = Vec::new();
let mut hitboxes = Vec::new();
let mut draw_high_stress = GeomBatch::new();
let mut draw_traffic_signals = GeomBatch::new();
let mut draw_unprotected_turns = GeomBatch::new();
Expand Down Expand Up @@ -176,7 +176,7 @@ impl RouteDetails {
.zoomed
.push(route_color.alpha(0.5), shape.clone());

hitbox_pieces.push(shape);
hitboxes.push(shape);

if let Some(color) = outline_color {
if let Some(outline) =
Expand Down Expand Up @@ -229,12 +229,7 @@ impl RouteDetails {
},
details_widget,
draw: draw_route,
hitbox: if hitbox_pieces.is_empty() {
// Dummy tiny hitbox
Polygon::rectangle(0.0001, 0.0001)
} else {
Polygon::union_all(hitbox_pieces)
},
hitboxes,
tooltip_for_alt: None,
}
}
Expand Down
4 changes: 2 additions & 2 deletions apps/ltn/src/connectivity.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use geom::{ArrowCap, Distance, PolyLine, Polygon};
use geom::{ArrowCap, Distance, PolyLine};
use street_network::Direction;
use widgetry::mapspace::{DummyID, World};
use widgetry::tools::PopupMsg;
Expand Down Expand Up @@ -284,7 +284,7 @@ fn setup_editing(

highlight_cell
.add_unnamed()
.hitbox(Polygon::union_all(polygons.clone()))
.hitboxes(polygons.clone())
// Don't draw cells by default
.drawn_in_master_batch()
.draw_hovered(batch)
Expand Down
4 changes: 2 additions & 2 deletions apps/ltn/src/draw_cells.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ impl RenderCells {
pub fn draw_island_outlines(&self) -> GeomBatch {
let neighbourhood_boundary = self
.boundary_polygon
.to_outline(Distance::meters(25.0))
.ok();
.get_outer_ring()
.map(|r| r.to_outline(Distance::meters(25.0)));

let mut batch = GeomBatch::new();
for (cell_color, polygons) in self.colors.iter().zip(self.polygons_per_cell.iter()) {
Expand Down
14 changes: 9 additions & 5 deletions apps/ltn/src/edit/shortcuts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,16 @@ pub fn make_world(
if let Some(ref focus) = focus {
let mut draw_path = GeomBatch::new();
let path = &focus.paths[focus.current_idx];
let poly = path
.trace_v2(&app.map)
.unwrap_or_else(|_| path.trace_all_polygons(&app.map));
let color = app.cs.good_to_bad_red.0.last().unwrap().alpha(0.8);

let color = *app.cs.good_to_bad_red.0.last().unwrap();
draw_path.push(color.alpha(0.8), poly);
match path.trace_v2(&app.map) {
Ok(poly) => {
draw_path.push(color, poly);
}
Err(_) => {
draw_path.extend(color, path.trace_all_polygons(&app.map));
}
}

let first_pt = path.get_req().start.pt(&app.map);
let last_pt = path.get_req().end.pt(&app.map);
Expand Down
11 changes: 11 additions & 0 deletions geom/src/bounds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,17 @@ impl Bounds {
b
}

/// Create a boundary covering some polygons.
pub fn from_polygons(polygons: &[Polygon]) -> Bounds {
let mut b = Bounds::new();
for poly in polygons {
for pt in poly.points() {
b.update(*pt);
}
}
b
}

/// Update the boundary to include this point.
pub fn update(&mut self, pt: Pt2D) {
self.min_x = self.min_x.min(pt.x());
Expand Down
44 changes: 13 additions & 31 deletions geom/src/polygon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,30 +340,6 @@ impl Polygon {
Polygon::maybe_rounded_rectangle(w, h, r).unwrap_or_else(|| Polygon::rectangle(w, h))
}

// TODO Result won't be a nice Ring
pub fn union(self, other: Polygon) -> Polygon {
let mut points = self.points;
let mut indices = self.indices;
let offset = points.len() as u16;
points.extend(other.points);
for idx in other.indices {
indices.push(offset + idx);
}
Polygon {
points,
indices,
rings: None,
}
}

pub fn union_all(mut list: Vec<Polygon>) -> Polygon {
let mut result = list.pop().unwrap();
for p in list {
result = result.union(p);
}
result
}

/// Union all of the polygons into one geo::MultiPolygon
pub fn union_all_into_multipolygon(mut list: Vec<Polygon>) -> geo::MultiPolygon {
// TODO Not sure why this happened, or if this is really valid to construct...
Expand Down Expand Up @@ -414,16 +390,22 @@ impl Polygon {
self.to_geo().intersects(&pl.to_geo())
}

/// Creates the outline around the polygon, with the thickness half straddling the polygon and
/// half of it just outside. Only works for polygons that're formed from rings. Those made from
/// PolyLines won't work, for example.
pub fn to_outline(&self, thickness: Distance) -> Result<Polygon> {
/// Creates the outline around the polygon (both the exterior and holes), with the thickness
/// half straddling the polygon and half of it just outside. Only works for polygons that're
/// formed from rings.
///
/// Returns a `Tessellation` that may union together the outline from the exterior and multiple
/// holes. Callers that need a `Polygon` must call `to_outline` on the individual `Rings`.
pub fn to_outline(&self, thickness: Distance) -> Result<Tessellation> {
if let Some(ref rings) = self.rings {
Ok(Polygon::union_all(
rings.iter().map(|r| r.to_outline(thickness)).collect(),
Ok(Tessellation::union_all(
rings
.iter()
.map(|r| Tessellation::from(r.to_outline(thickness)))
.collect(),
))
} else {
Ring::new(self.points.clone()).map(|r| r.to_outline(thickness))
Ring::new(self.points.clone()).map(|r| Tessellation::from(r.to_outline(thickness)))
}
}

Expand Down
21 changes: 20 additions & 1 deletion geom/src/tessellation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl Tessellation {
Bounds::from(&self.points)
}

fn center(&self) -> Pt2D {
pub fn center(&self) -> Pt2D {
self.get_bounds().center()
}

Expand Down Expand Up @@ -135,6 +135,25 @@ impl Tessellation {
);
}
}

pub fn union(self, other: Self) -> Self {
let mut points = self.points;
let mut indices = self.indices;
let offset = points.len() as u16;
points.extend(other.points);
for idx in other.indices {
indices.push(offset + idx);
}
Self { points, indices }
}

pub fn union_all(mut list: Vec<Self>) -> Self {
let mut result = list.pop().unwrap();
for p in list {
result = result.union(p);
}
result
}
}

pub fn downsize(input: Vec<usize>) -> Vec<u16> {
Expand Down
10 changes: 7 additions & 3 deletions map_gui/src/render/area.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use geom::Polygon;
use geom::{Pt2D, Tessellation};
use map_model::{Area, AreaID, AreaType, Map};
use widgetry::{Color, EventCtx, Fill, GeomBatch, GfxCtx, Line, Text};

Expand Down Expand Up @@ -54,8 +54,12 @@ impl Renderable for DrawArea {

fn draw(&self, _: &mut GfxCtx, _: &dyn AppLike, _: &DrawOptions) {}

fn get_outline(&self, map: &Map) -> Polygon {
fn get_outline(&self, map: &Map) -> Tessellation {
// Since areas are so big, don't just draw the outline
map.get_a(self.id).polygon.clone()
Tessellation::from(map.get_a(self.id).polygon.clone())
}

fn contains_pt(&self, pt: Pt2D, map: &Map) -> bool {
map.get_a(self.id).polygon.contains_pt(pt)
}
}
12 changes: 7 additions & 5 deletions map_gui/src/render/bike.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use geom::{ArrowCap, Circle, Distance, Line, PolyLine, Polygon, Pt2D};
use geom::{ArrowCap, Circle, Distance, Line, PolyLine, Pt2D, Tessellation};
use map_model::Map;
use sim::{CarID, DrawCarInput, Intent, Sim};
use widgetry::{Drawable, GeomBatch, GfxCtx, Prerender};
Expand Down Expand Up @@ -139,10 +139,12 @@ impl Renderable for DrawBike {
g.redraw(&self.draw_default);
}

fn get_outline(&self, _: &Map) -> Polygon {
Circle::new(self.body_circle.center, Distance::meters(2.0))
.to_outline(OUTLINE_THICKNESS)
.unwrap()
fn get_outline(&self, _: &Map) -> Tessellation {
Tessellation::from(
Circle::new(self.body_circle.center, Distance::meters(2.0))
.to_outline(OUTLINE_THICKNESS)
.unwrap(),
)
}

fn contains_pt(&self, pt: Pt2D, _: &Map) -> bool {
Expand Down
6 changes: 3 additions & 3 deletions map_gui/src/render/building.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::cell::RefCell;

use geom::{Angle, Distance, Line, Polygon, Pt2D, Ring};
use geom::{Angle, Distance, Line, Polygon, Pt2D, Ring, Tessellation};
use map_model::{Building, BuildingID, Map, OffstreetParking};
use widgetry::{Color, Drawable, EventCtx, GeomBatch, GfxCtx, Line, Text};

Expand Down Expand Up @@ -237,12 +237,12 @@ impl Renderable for DrawBuilding {
0
}

fn get_outline(&self, map: &Map) -> Polygon {
fn get_outline(&self, map: &Map) -> Tessellation {
let b = map.get_b(self.id);
if let Ok(p) = b.polygon.to_outline(OUTLINE_THICKNESS) {
p
} else {
b.polygon.clone()
Tessellation::from(b.polygon.clone())
}
}

Expand Down
Loading