Skip to content

Conversation

@michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Feb 27, 2025

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Note this builds on top of #1317, so review that first (draft until then). Merged!

PreparedGeometry is fast for some things, but it can't do everything a Geometry can do.

Since you can PreparedGeometry::from(&geometry), I'd typically follow a pattern where I'd keep the original geometry and the PreparedVersion of that geometry together, like:

let line_strings: Vec<LineString> = todo!();
let prepared_linestrings: Vec<(LineString, PreparedGeometry)> = line_strings.into_iter().map(|ls| (ls, PreparedGeometry::from(&ls))).collect();

// Then pass the (unprepared, prepared) tuple around - using the prepared geometry for fast Relate operations 
// and the unprepared geometry for everything else:

let (prepared, unprepared) = prepared_polygons.pop().unwrap();
let (other_prepared, other_unprepared) = prepared_polygons.pop().unwrap();
// It's faster to check if there *is* an intersection using a `PreparedGeometry`, 
// but to get the area of the intersection with a `BoolOp`, we need the actual geometry
if prepared.relate(&other_prepared).is_intersects() {
    unprepared.intersection(&other_unprepared)
}

That works, but it's kind of annoying to shuttle both around as a tuple, when I know internally the PreparedGeometry has a reference to the underlying Geometry. This PR exposes that:

Now you can:

let line_strings: Vec<LineString> = todo!();
// No more tuple!
let prepared_linestrings: Vec<PreparedGeometry> = line_strings.into_iter().map(PreparedGeometry::from).collect();

let prepared = prepared_polygons.pop().unwrap();
let other_prepared = prepared_polygons.pop().unwrap();
if prepared.relate(&other_prepared).is_intersects() {
    prepared.geometry().intersection(&other.geometry())
}

Note: that this makes GeometryCow public since it's a constraint on what the inner geometry of PreparedGeometry can be.

GeometryCow was previously private because it's a bit obscure - I'm not sure that it will be useful to people. But it seems stable, documented, and unlikely to be "dangerous", so I'm not worried about making it public.

@michaelkirk michaelkirk marked this pull request as draft February 27, 2025 22:10
@michaelkirk michaelkirk force-pushed the mkirk/get-geom-type-from-prepared-geom branch from 277417d to 89a5582 Compare February 27, 2025 22:11
@michaelkirk michaelkirk changed the title Mkirk/get geom type from prepared geom get geom type from prepared geom Feb 27, 2025
// by using the bounding_rect from the RTree
debug_assert_eq!(bounding_rect, geometry_graph.geometry().bounding_rect());

impl<F: GeoFloat> From<Point<F>> for PreparedGeometry<'_, F> {
Copy link
Member Author

Choose a reason for hiding this comment

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

These super repetitive implementations still exist, they were just moved to the same macro loop that spits out the impl Relate for each geometry, which is no less generated code, but much less typing.

https://github.com/georust/geo/pull/1318/files#diff-68ef9d4f18a931c4cac3275e080a0e37a6a12ec252e519820801e3fa3d716dfdR82

@michaelkirk michaelkirk force-pushed the mkirk/cache-bbox branch 3 times, most recently from 679606f to 2efea92 Compare March 3, 2025 17:57
Base automatically changed from mkirk/cache-bbox to main March 3, 2025 23:43
`PreparedGeometry` is fast for some things, but it can't do everything a
Geometry can do. Since it was previously possible to `PreparedGeometry::from(&geometry)` I'd typically do things like:

```
let line_strings: Vec<LineString> = todo!();
let prepared_linestrings: Vec<(LineString, PreparedGeometry)> = line_strings.into_iter().map(|ls| (ls, PreparedGeometry::from(&ls))).collect();

// Then pass the (unprepared, prepared) tuple around - using the
// prepared geometry for things Relate operations and the unprepared
// geometry for everything else:

let (prepared, unprepared) = prepared_polygons.pop().unwrap();
let (other_prepared, other_unprepared) = prepared_polygons.pop().unwrap();
// faster to check if there *is* an intersection, before trying BoolOps the intersection Geometry.
if prepared.relate(&other_prepared).is_intersects() {
    unprepared.intersection(&other_unprepared)
}
```

But that's kind of annoying.

Now its:
```
let line_strings: Vec<LineString> = todo!();
let prepared_linestrings: Vec<PreparedGeometry> = line_strings.into_iter().map(PreparedGeometry::from).collect();

let prepared = prepared_polygons.pop().unwrap();
let other_prepared = prepared_polygons.pop().unwrap();
// faster to check if there *is* an intersection, before trying BoolOps the intersection Geometry.
if prepared.relate(&other_prepared).is_intersects() {
    prepared.geometry().intersection(&other.geometry())
}
```

Note: that this makes `GeometryCow` public since it's a constraint on
what the inner geometry of PreparedGeometry can be.

GeometryCow was previously private because it's a bit obscure - I'm not
sure that it will be useful to people. But it seems stable, documented,
and unlikely to be "dangerous".
@michaelkirk michaelkirk force-pushed the mkirk/get-geom-type-from-prepared-geom branch from 89a5582 to 336ec33 Compare March 4, 2025 05:23
@michaelkirk michaelkirk marked this pull request as ready for review March 4, 2025 05:30
@michaelkirk michaelkirk requested a review from dabreegster March 4, 2025 05:30
Copy link
Contributor

@dabreegster dabreegster left a comment

Choose a reason for hiding this comment

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

This is a useful API change, will indeed make caller code simpler

G: Into<GeometryCow<'a, F>>,
{
pub(crate) fn geometry(&self) -> &GeometryCow<F> {
self.geometry_graph.geometry()
Copy link
Contributor

Choose a reason for hiding this comment

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

Just reasoning through part of the diff: there was already a GeometryCow available buried deeper, but the types you're exposing here are a little different... Into<GeometryCow>, not &GeometryCow. So it's indeed necessary to store the new field, not just try and get it from geometry_graph

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. This was probably worth spelling out more in my PR, but I'll do it now:

An alternate approach that doesn't add a new field/generic to PreparedGeometry could use the existing reference from the GeometryGraph's GeometryCow.

The problem is, because GeometryCow is an enum (just like geo::Geometry) holding some geometry type, we don't have a nice way to get the specific geometry type back out. Instead, the api would have to be fallible, and look like:

geometry_cow.as_point() -> Option<Point>
geometry_cow.as_line_string() -> Option<LineString>
geometry_cow.as_polygon() -> Option<Polygon>
// etc.

For a given GeometryCow, exactly one of those would return Some, the rest would return None. As a programmer using this API you'd have to keep track of what kind of geometry is in each PreparedGeometry.

@michaelkirk michaelkirk added this pull request to the merge queue Mar 4, 2025
Merged via the queue into main with commit dbe19b7 Mar 4, 2025
18 checks passed
@michaelkirk michaelkirk deleted the mkirk/get-geom-type-from-prepared-geom branch March 4, 2025 18:22
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