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

dabreegster · 2022-08-30T09:14:35Z

Towards the quest of making geom::Polygon always internally just wrap a geo::Polygon.

Why is this necessary?

geom tries to be stricter than geo, rejecting "bad" geometry upfront to try to isolate a problem at its source. A Ring must not have any duplicate points (adjacent or not), except for the first = last point. This is further complicated by "duplicate" points being defined a bit strictly. Every Pt2D trims its f64's down to a few decimal places.

I'm starting to question if this attempt at upfront strictness is even helpful. In the beginning, it was an attempt to "clean up" a bunch of crazy OSM geometry I was encountering or algorithmically producing, but I've lost context on many of the original motivating problems. Many might've been solved in another way by now. Also, the strictness isn't even complete -- my mental model of a Ring is that there are no self-intersections at all, but nothing looks for this right now.

The strictness is actually only useful in geom::PolyLine code anyway. Most of the hand-written geometry logic is there now. Operations like finding a point some distance along the polyline, or projecting a polyline left or right for buffering are easier to reason about without duplicate adjacent points, or even without internal points that're "redundant" from an angle perspective (aka a polyline [(0, 0), (10, 0), (20, 0)] has a redundant middle point, it could just be [(0, 0), (20, 0)]).

There've been countless times somebody sends me a bug report with an unhelpful error about duplicate points in a ring. Even when the stack trace shows something, it's not easy to reconstruct context about which object in the map caused the problem. My fear has been that if invalid geometry "sneaks in", then there will be rendering, hitbox testing, etc bugs downstream that're hard to catch. But in reality, I can't really say all the strict checks have caught that many problems.

Alternative

I feel like I just convinced myself that the validity checks should actually go away now. The heftier bits of logic in PolyLine can just be made to deal with duplicate adjacent points and the like. Any opinions?

Next steps

Back to the original goal of making geom::Polygon less weird. After this PR (or rethinking the validity checks), I think it'll be:

Dealing with geom::Polygon transforms sometimes destroying the internal rings through operations like scaling. For this reason, strip_rings is often used. But then, if the validity checks go away, maybe this isn't actually a problem.
Figuring out what Circle::to_polygon and PolyLine::make_polygons should return. They pre-tessellate right now, which is useful for performance. But many callers also need the real Polygon too, to produce outlines or do other operations.
Changing the internal geom::Polygon representation to either wrap geo::Polygon or just be exterior: Ring, holes: Vec<Ring>

dabreegster · 2022-08-30T09:15:30Z

Validation

Oh yeah, and this PR has some impacts on bits of downstream code. I'll comment inline in the changed code. I will need to reimport all maps, where I might hit a few new problems, but I think I understand all the effect right now

dabreegster · 2022-08-30T09:16:36Z

apps/game/src/edit/roads.rs

+            );
+            GeomBatch::from(vec![(app.cs.fade_map_dark, fade_area)])
+        }
+        Err(_) => {


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.

dabreegster · 2022-08-30T09:17:10Z

apps/game/src/layer/traffic.rs

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


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

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

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

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.

dabreegster · 2022-08-30T09:18:49Z

convert_osm/src/lib.rs

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


A real example in Milwaukee:

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.

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.

dabreegster · 2022-08-30T09:19:31Z

geom/src/polygon.rs

@@ -587,8 +568,12 @@ impl Polygon {
        Ok(results)
    }

+    /// If simplification fails, just keep the original polygon


This is an example where all the callers of this would end up doing this kind of thing anyway

michaelkirk

The changes seem reasonable, but the regressions are unfortunate. I leave it to you wether you want to address them before merging or Move Fast And Break Things. 😉

michaelkirk · 2022-08-31T17:09:41Z

convert_osm/src/lib.rs

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


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.

michaelkirk · 2022-08-31T17:16:40Z

apps/game/src/layer/traffic.rs

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


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

michaelkirk · 2022-08-31T17:20:46Z

apps/ltn/src/draw_cells.rs

+                // If clipping fails, just use the original polygon.
+                if let Ok(list) = p.intersection(&result.boundary_polygon) {
+                    clipped.extend(list);
+                } else {


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.

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

has stricter rules for Rings. Finally remove Polygon::buggy_new, and instead make many Polygon methods return a Result. #951 Just internal geom changes here, no changes downstream yet.

skipped. This also fixes the bus route names recorded per road.

…pass strict Ring checks. Unused right now. #951

dabreegster · 2022-09-01T08:47:38Z

The regressions are in code complexity, not observable behavior -- I haven't found a single example where convex hull fails to return something valid, for example.

I'm still in a mild existential crisis about whether or not geom should try to be enforcing all of this extra strictness about duplicate points or not. But for the moment, this PR makes things more consistent and gets us closer to cleaning up geom::Polygon, so i'll go forward with it. Thanks for the review!

dabreegster commented Aug 30, 2022

View reviewed changes

michaelkirk approved these changes Aug 31, 2022

View reviewed changes

dabreegster added 2 commits September 1, 2022 08:45

Make the conversion from geo::Polygon to Polygon falliable, since geom

4dcede2

has stricter rules for Rings. Finally remove Polygon::buggy_new, and instead make many Polygon methods return a Result. #951 Just internal geom changes here, no changes downstream yet.

Changing all code to handle falliable polygon operations. #951

964749d

dabreegster force-pushed the geo_from branch from 4e38a7f to 964749d Compare September 1, 2022 07:46

dabreegster added 2 commits September 1, 2022 09:28

Regenerate all data. Some areas clipped to the map boundary are now

b44d1a2

skipped. This also fixes the bus route names recorded per road.

Make a way to just tessellate a geo::Polygon without requiring it to …

f1e54c2

…pass strict Ring checks. Unused right now. #951

dabreegster merged commit ac77aef into master Sep 1, 2022

dabreegster deleted the geo_from branch September 1, 2022 08:58

dabreegster mentioned this pull request Feb 9, 2023

Revisit geom validity checks a-b-street/geom#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dabreegster commented Aug 30, 2022

dabreegster commented Aug 30, 2022

dabreegster Aug 30, 2022

dabreegster Aug 30, 2022

michaelkirk Aug 31, 2022

dabreegster Sep 1, 2022

dabreegster Sep 1, 2022

dabreegster Aug 30, 2022

michaelkirk Aug 31, 2022

dabreegster Aug 30, 2022

michaelkirk left a comment

michaelkirk Aug 31, 2022

michaelkirk Aug 31, 2022

michaelkirk Aug 31, 2022

dabreegster Sep 1, 2022

dabreegster commented Sep 1, 2022

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

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

Conversation

dabreegster commented Aug 30, 2022

Why is this necessary?

Alternative

Next steps

dabreegster commented Aug 30, 2022

Validation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabreegster commented Sep 1, 2022