Revisit geom validity checks about geom HOT 6 OPEN

Ah, a-b-street/abstreet#980 walks through the same reasoning. Time to actually do it!

from geom.

Ah, a-b-street/abstreet#980 walks through the same reasoning. Time to actually do it!

I consider it a good sign that your internal monologue is consistent. 🤣

As far as whether this is a good idea, I'm not really sure either way. Your rationale seems sound, and no one is better positioned to make the judgement call than you.

One significant difference between abstreet:geom and geo::geometry is that geo doesn't have a Ring type, it only has LineStrings, which may or may not be closed. I'm not sure if that has implications for abstreet.

projecting polylines left/right

That may be this PR: georust/geo#935, though it's been kind of quiet lately (Oh! I see you already left a comment in there at one point).

but the ideal direction forward should be to contribute those algorithms there directly.

❤️

from geom.

It certainly is tricky to decide where/how those kinds of constraints should be represented. If typical uses are unwraping all over the place and crashing, that's an indication that the constraints on the type are too strong (at least for some of those uses).

I don't think the base geometry types should be as fussy as they currently are. For example, using a PolyLine to represent a list of GPS samples seems reasonable, and sometimes I don't care if a Line has zero length. Anything that's not going to crash rendering code seems ok to me.

georust/geo seems pretty light on the checks: geo::geometry::Triangle states that "the three vertices must not be collinear and they must be distinct", but doesn't verify it anywhere; geo::geometry::Line::slope might divide by zero, and there are debug_assert!s all over the implementation. It does have HasDimensions which can be used to detect degenerate Lines etc.

I would prefer to use the type system to provide more safety than that.

E.g. for a Line type that doesn't enforce distinct points, I prefer safe implementations for methods that depend on that, like Line::angle(&self) -> Option<T>. Providing Line::is_proper(&self) -> bool and optionally Line::angle_unchecked(&self) -> T might help the ergonomics in some situations. (I really like how shift_right returns a Result for this reason).

Another idea is to provide wrapper types or newtypes around the basic types that enforce additional constraints. This would be super helpful for users, because they can have fields of type ProperLine and NonOverlappingLineString as applicable. Ring is an example of this that I have found useful for its semantics.

This would be pretty easy to implement if the user is expected to cast to cast to the base type in order to work with the geometry (using Into or AsRef or whatever). For extra credit, there's also the possibility for a super ergenomic API that encodes the limitation of all the operations. Like, ProperLine::angle(&self) -> T would be safe, and Ring could have grow(dist) -> Ring which can't fail, but shrink(dist) -> Option<Ring>, because the ring might shrink into nothing.

from geom.

georust doesn't have everything we need today (projecting polylines left/right or slicing/clipping them to a [start, end] distance), but the ideal direction forward should be to contribute those algorithms there directly.

We should consider implementing our own traits and trait impls for the georust type for these algorithms. Ideally they eventually land upstream, but we get the nice interface right away.

from geom.

E.g. for a Line type that doesn't enforce distinct points, I prefer safe implementations for methods that depend on that, like Line::angle(&self) -> Option

This makes sense to me -- the upfront check of a non-zero length line is too strict, but if something later on actually cares about angle, zero-length does matter.

A similar piece of unnecessarily strict checks is dist_along_of_point, which I've been trying to change to use https://docs.rs/geo/latest/geo/algorithm/line_locate_point/trait.LineLocatePoint.html. If we feed in a distance that's slightly negative or slightly past the end of the polyline, currently we fail, and this occasionally causes weirdness in intersection geometry code.

Some specific tasks I'll try to tackle:

• Use LineLocatePoint instead of dist_along_of_point, and maybe make the API infallible
• Revisit duplicate point checks in all the constructors, and maybe adjust things like angle to return Option
• Try to replace some of the line/polyline intersection math with georust things
• Rewrite the polyline slicing operations more carefully, and try to upstream in georust

from geom.

If we feed in a distance that's slightly negative or slightly past the end of the polyline, currently we fail...

A version of the method that "clamps" to the line (i.e. using 0 if a negative dist is passed in) would be ergenomic for some usecases.

from geom.