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

Conversation

dabreegster
Copy link
Collaborator

.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.

self.body
.to_thick_boundary(CAR_WIDTH, OUTLINE_THICKNESS)
.unwrap_or_else(|| self.body_polygon.clone())
fn get_outline(&self, _: &Map) -> Tessellation {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very slight change here.
Screenshot from 2022-08-30 15-49-44
Screenshot from 2022-08-30 15-49-28
Most of the time, the body polygon narrows in the front. The outline has always just been the simpler rectanglish shape. Before, the hitbox was based on the more detailed narrowed shape. But now the hitbox is also just the simple rectangle. Barely noticeable.

fn contains_pt(&self, pt: Pt2D, map: &Map) -> bool {
self.get_outline(map).contains_pt(pt)
}
fn get_outline(&self, map: &Map) -> Tessellation;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Necessary because some buildings have holes. Polygon::to_outline returns a Tessellation now to handle that case. Returning a Vec<Polygon> (one for exterior, one for each hole) is another option, but it's a gross API. For the callers that want Polygon outlines for each individual ring, they can still do that themselves

self
}

/// Specifies the geometry of the object as a multipolygon.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The change here is to let something in a World have multiple polygons as the hitbox. If more places in the code wind up needing something like this, then a MultiPolygon type becomes useful. :)

@@ -38,10 +38,13 @@ impl ColorLegend {
.map(|(idx, color)| ((idx as f64) / ((n - 1) as f64), *color))
.collect(),
}),
Polygon::union_all(
Tessellation::union_all(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of the real reasons why unioning tessellations is actually necessary. This is a bunch of rectangles in a row. Why not just one big rectangle? Because we need the points in the middle in order for the shader logic to actually have a chance to evaluate here
Screenshot from 2022-08-30 15-54-59

@dabreegster dabreegster merged commit daf3ed3 into master Aug 30, 2022
@dabreegster dabreegster deleted the geo_union branch August 30, 2022 14:56
Copy link
Collaborator

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

2 participants