-
-
Notifications
You must be signed in to change notification settings - Fork 345
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
Finish the Polygon cutover #988
Conversation
} else { | ||
ring.to_outline(THICKNESS) | ||
} | ||
// TODO If the below isn't true, show it as an outline instead? Can't make that a Polygon, |
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.
Slight regression in functionality here, but this debug tool is not used much at all and has pre-existing problems
/// degrees. Be warned the resulting polygon doesn't have a ring as its boundary! | ||
pub fn to_partial_polygon(&self, percent_full: f64) -> Polygon { | ||
/// Renders some percent, between [0, 1], of the circle. The shape starts from 0 degrees. | ||
pub fn to_partial_tessellation(&self, percent_full: f64) -> Tessellation { |
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.
It'd take a bit of effort to make a Ring
out of this. The center point needs to exist, unless percent_full = 1.0
. And it's valid to pass 0
in, in which case the tessellation is empty. That's no good for a Ring, so it'd require an API change anyway. All of the callers just draw this.
rings: Option<Vec<Ring>>, | ||
// [0] is the outer/exterior ring, and the others are holes/interiors | ||
pub(crate) rings: Vec<Ring>, | ||
// For performance reasons, some callers may want to generate the polygon's Rings and |
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.
There is only one use of this -- PolyLine
. I still have problems doing profiling on my Linux setup. When I sort that out, I'll look into just using earcutr
for those cases too.
self.rings.remove(0) | ||
} | ||
|
||
/// The [centroid](https://docs.rs/geo/latest/geo/algorithm/centroid/trait.Centroid.html) |
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.
I didn't carefully check through all the callers of this. I don't think any of them care that much about exactly what "center" means.
But also, geo's algorithm can fail. Skimming through the source, I'm not totally sure if it's possible for the polygons we send in here -- it's stuff like the holes taking up the entire polygon.
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.
it's stuff like the holes taking up the entire polygon.
I think in this case actually, centroid calculation will succeed, however proceeds with a zero-area centroid - treating the polygon as if it were a linestring.
IIRC the only time we'll return a None
centroid is for empty geometries.
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.
Hm, looking at your current method a bit, I'd expect this change to be a potentially serious perf degradation if it's on any hot paths. You were previously taking the centroid of the points, whereas geo's implementation considers area which is relatively a lot more expensive to compute.
I mean, it's still "fast" for what it does, it's just solving a more complicated problem than what you were doing before. If what you had before was working for your use case you might want to revert this portion.
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.
Good call -- reverted in 272e18ee8714c8e80a86e71a88148f0612ededc5
. I don't thing polygon.center
is on any hot paths, but average of outer points is simpler anyway and was working fine.
@@ -507,29 +485,19 @@ impl Polygon { | |||
} | |||
|
|||
// A less verbose way of invoking the From/Into impl. Note this hides a potentially expensive | |||
// clone. The eventual goal is for Polygon to directly wrap a geo::Polygon, at which point this | |||
// cost goes away. | |||
// clone. |
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.
Removing this comment, because I'm not sure we want to directly wrap.
/// The resulting polygon is manually triangulated and may not have a valid outer Ring (but it | ||
/// usually does). | ||
/// This produces a `Polygon` with a valid `Ring`. It may crash if this polyline was made with | ||
/// `unchecked_new` | ||
pub fn make_polygons(&self, width: Distance) -> Polygon { |
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.
So what's going on here? Calling to_outline
on rings does PolyLine::unchecked_new
, which ignores the first = last point that'd normally be illegal. This is used for drawing a thick border around a ring:
The result right now isn't a Polygon
. But I think it wouldn't be too hard for it to be. We're easily making a ring for the outer part of the polyline. The inner part isn't trimming the line segments back at that last intersection yet, causing the funky corner. But it wouldn't be hard to specialize this logic for Ring
. to_outline
should produce an outer and inner ring. It might fail if the original ring is too small, and the inner ring ends up collapsing, so the API would have to change.
Also related: https://github.com/lelongg/geo-offset could maybe help here. I haven't tried it in a while. georust/geo#641 is also relevant.
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.
Just a thought... would things get really crazy if the polygon created by to_outline
wasn't centered on the polygon, and instead its interior ring was the exterior of the outlined polygon.
That is, the generated outline polygon would be strictly outside of its input.
I could imagine this being hairy - like maybe there'd be little pixel gaps between the input and its outline. Anyway, just spit-ballin.
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.
Ah, I see! Instead of having to both shrink and expand a Ring
to make an outline, we could just expand and use the original ring as the inner hole. The shrinking case probably has more edge cases with some line segments disappearing entirely.
From a design perspective, I'd actually prefer if the outlines for polygons were entirely contained in the original polygon. The half-straddling approach now is particularly weird for hovering over lane polygons:
@@ -180,9 +205,44 @@ impl Tessellation { | |||
} | |||
result | |||
} | |||
|
|||
/// Produces a GeoJSON multipolygon consisting of individual triangles. Optionally map the |
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.
I was hoping to get rid of this silliness, but osm2streets
currently draws arrow outlines via GeoJSON. So until making to_outline
return a real polygon, this stays
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.
You've probably already thought about this, but maybe it makes sense to not tessellate the line string arrow...
Like maybe:
fn make_outline_arrow(thickness, cap) -> geom::Polygon {
let exterior = make_arrow(thickness * 2, cap);
let interior = make_arrow(thickness / 2, cap );
Polygon::with_holes(exterior, interior)
}
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.
... What a good idea! It never occurred to me, this is exactly why reviews are awesome. :)
I haven't quite replicated the same nice outline style with this method though. The polyline has to be trimmed back partly for the arrowhead to appear correctly. I'll keep playing with it, and remove this technical debt of geojson triangles once it works, or once to_outline
can always produce nice rings.
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.
Or maybe one day the revolution will finally come.
geo::Polygon::new(exterior, Vec::new()) | ||
} | ||
|
||
// TODO After making to_outline return a real Polygon, get rid of this |
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.
Likewise, this is used in the LTN tool to try and clean up a visual artifact from coloured cells
This doesn't quite work, though -- circles and polylines actually do not produce valid Rings.
Tessellation when thickening Rings for outlines, since the result isn't a valid Ring. #951
0e2122c
to
fbd59bb
Compare
I want to include this change in a release soon, so I'm going to merge now. Async feedback welcome. I won't close a-b-street/geom#2 until figuring out the |
Oh yeah, and the build will be momentarily broken, until I finish the osm2streets dependency dance |
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.
LGTM - I added some thoughts and minor feedback. It's been fun watching this epic refactor. Hopefully the distraction is nearly over and it pays dividends.
Self::new(exterior, Vec::new()) | ||
} | ||
fn from(mut poly: Polygon) -> Self { | ||
let exterior = poly.rings.remove(0); |
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.
🎉
/// The resulting polygon is manually triangulated and may not have a valid outer Ring (but it | ||
/// usually does). | ||
/// This produces a `Polygon` with a valid `Ring`. It may crash if this polyline was made with | ||
/// `unchecked_new` | ||
pub fn make_polygons(&self, width: Distance) -> Polygon { |
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.
Just a thought... would things get really crazy if the polygon created by to_outline
wasn't centered on the polygon, and instead its interior ring was the exterior of the outlined polygon.
That is, the generated outline polygon would be strictly outside of its input.
I could imagine this being hairy - like maybe there'd be little pixel gaps between the input and its outline. Anyway, just spit-ballin.
} | ||
|
||
/// Just produces a Tessellation | ||
pub fn thicken_tessellation(&self, width: Distance) -> Tessellation { |
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.
maybe fn tessellate(&self, width: Distance)
would be clear enough while being more concise.
Just a naming nit, so feel free to ignore.
@@ -180,9 +205,44 @@ impl Tessellation { | |||
} | |||
result | |||
} | |||
|
|||
/// Produces a GeoJSON multipolygon consisting of individual triangles. Optionally map the |
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.
You've probably already thought about this, but maybe it makes sense to not tessellate the line string arrow...
Like maybe:
fn make_outline_arrow(thickness, cap) -> geom::Polygon {
let exterior = make_arrow(thickness * 2, cap);
let interior = make_arrow(thickness / 2, cap );
Polygon::with_holes(exterior, interior)
}
@@ -19,16 +22,38 @@ pub struct Triangle { | |||
} | |||
|
|||
impl From<Polygon> for Tessellation { | |||
fn from(polygon: Polygon) -> Self { | |||
fn from(mut polygon: Polygon) -> Self { |
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.
I wonder if it'd be better for this be a borrowed Polygon. I see several clones
elsewhere in this PR like:
Tessellation::from(self.blob.clone())
It seems like only the pre-tessellated polygons ever take advantage of owning the input, and for those cases, maybe it'd be better to have a consuming take_tessellation
or into_tessellation
method on Polygon.
No biggie, just a perf note.
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.
The normal code path winds up making a copy of every ring's points too... This seems unfortunate, since ultimately earcutr just needs a &Vec. Currently we're creating this intermediate nested GeoJSON form, then calling flatten
. Probably copying the points could be avoided entirely with a little bit of cleverness, and maybe changing earcutr
to take points as some kind of iterator.
Anyway, thanks for raising this. I'll make a note to get a profiling setup working and play around with variations of this idea!
data/system/gb/london/maps/camden.bin | ||
1034 single blocks (2 failures to blockify), 2 partial merges, 0 failures to blockify partitions | ||
1036 single blocks (1 failures to blockify), 2 partial merges, 0 failures to blockify partitions |
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.
I don't really know what to make of these. I guess converting between these representations seems to have slightly altered some topological computation - but not by much?
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.
I didn't look into these changes too carefully, except to load the changed maps in the LTN tool and spot-check that adjusting boundaries and default neighbourhood partitioning still looks sane. When I've looked closely into "failures to blockify" in the past, it's always stuff near like
I'd prefer to spend time fixing the root problems in OSM importing (now part of osm2streets
, with good unit tests over there) than making things like the block tracing robust to extremely broken inputs
Finally the conclusion of a-b-street/geom#2 --
Polygon
just storesVec<Ring>
now. It's turned into aTessellation
only when needed. Places that may not be able to produce validRings
instead useTessellation
.Turning a
geom::Polygon
into ageo::Polygon
is now straightforward. I don't think directly storing one makes sense yet, given thef64
rounding and stricter rules aboutRings
. Still open to relaxing the strictness in A/B Street and re-thinking all of this, but later.A practical consequence of this change: serialized file size drops a bit. A 28MB Bristol map is now about 27MB.
TODO for me before pushing this PR: regenerate everything and manually test some important things. And then make sure to update
osm2streets
repo in sync.