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.

Sign up for GitHub

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

Jump to bottom

feat: add simplify_polygon_hull method #366

Merged
merged 3 commits into from Jan 7, 2024
Merged

feat: add simplify_polygon_hull method #366

merged 3 commits into from Jan 7, 2024

Conversation

oleksii-leonov
Copy link
Contributor

@oleksii-leonov oleksii-leonov commented Nov 24, 2023 •
edited

Summary

The feature is equal to ST_SimplifyPolygonHull in PostGIS.

Computes a simplified topology-preserving outer or inner hull of a polygonal geometry.
An outer hull completely covers the input geometry.
An inner hull is completely covered by the input geometry.
The result is a polygonal geometry formed by a subset of the input vertices.
MultiPolygons and holes are handled and produce a result with the same structure as the input.
https://postgis.net/docs/ST_SimplifyPolygonHull.html

Utilizes the GEOSPolygonHullSimplify method introduced in GEOS 3.11.0.

Other Information

@oleksii-leonov oleksii-leonov force-pushed the feat/add-simplify-polygon-hull branch 5 times, most recently from e6ed30b to 185dc79 Compare November 24, 2023 18:21
keithdoggett
keithdoggett approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@keithdoggett keithdoggett left a comment

Choose a reason for hiding this comment

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

Thanks @oleksii-leonov for the PR and the detailed tests. LGTM just one question about adding guards around one method, but looks great.

ext/geos_c_impl/geometry.c Outdated
Comment on lines 834 to 859
static VALUE
method_geometry_simplify_polygon_hull(VALUE self,
VALUE vertex_fraction,
VALUE is_outer)
{
VALUE result;
RGeo_GeometryData* self_data;
const GEOSGeometry* self_geom;
VALUE factory;

unsigned int is_outer_uint = RTEST(is_outer) ? 1 : 0;

result = Qnil;
self_data = RGEO_GEOMETRY_DATA_PTR(self);
self_geom = self_data->geom;
if (self_geom) {
factory = self_data->factory;
result = rgeo_wrap_geos_geometry(
factory,
GEOSPolygonHullSimplify(
self_geom, is_outer_uint, rb_num2dbl(vertex_fraction)),
Qnil);
}
return result;
}

Copy link
Member

Choose a reason for hiding this comment

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

Should we add method guards around this too? Not sure if having the undefined GEOSPolygonHullSimplify symbol will break compilation in older versions of not guarded or would only cause a segfault if it's called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@keithdoggett , you are right 👍
It's safer to wrap the method with a guard.

I have updated the PR.

@keithdoggett
Copy link
Member

Thanks @oleksii-leonov I'll give @BuonOmo another day or 2 to review, but if he's not available I'll merge it in.

BuonOmo
BuonOmo reviewed Dec 1, 2023
View reviewed changes
Copy link
Member

@BuonOmo BuonOmo left a comment

Choose a reason for hiding this comment

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

Thanks for the beautiful tests ! And for the overall feature. As always my concern is about the geos API. Can the hull generation method fail? And if it does, what happens?

Unfortunatily I cannot deep dive into geos codebase with my smartphone. So have verified that @oleksii-leonov ? You can also take advantage of running tests with MAINTAINER_MODE for more insights.

If you can't be sure, maybe @keithdoggett you could have a second pair of eyes on this ? 👀

Besides that, LGTM

@oleksii-leonov
Copy link
Contributor Author

@BuonOmo

It looks like the PolygonHullSimplifier (https://github.com/libgeos/geos/blob/3.12/src/simplify/PolygonHullSimplifier.cpp) has only one possible exception:
throw util::IllegalArgumentException("Input geometry must be polygonal"):
https://github.com/libgeos/geos/blob/5b60b76fc0dbab04ed2488f78c4239a8b82657b8/src/simplify/PolygonHullSimplifier.cpp#L105C1-L105C1

It's handled correctly by RGeo:

RGeo::Geos.factory.parse_wkt("LINESTRING (0 0, 0 6, 6 6, 6 0, 0 0)").simplify_polygon_hull(0.1, true)
# (irb):4:in `simplify_polygon_hull': Input geometry must be polygonal (RGeo::Error::InvalidGeometry)

I could move simplify_polygon_hull from ext/geos_c_impl/geometry.c to ext/geos_c_impl/polygon.c.

@BuonOmo
Copy link
Member

BuonOmo commented Dec 2, 2023

@oleksii-leonov thank you for verifying. I would add a test to reflect this.

On moving the method I think you are right, it would indeed be a better place.

keithdoggett
keithdoggett approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@keithdoggett keithdoggett left a comment

Choose a reason for hiding this comment

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

Thanks for moving the method @oleksii-leonov and checking for the exception. I think this is good to merge now. WDYT @BuonOmo

BuonOmo
BuonOmo reviewed Dec 12, 2023
View reviewed changes
Copy link
Member

@BuonOmo BuonOmo left a comment

Choose a reason for hiding this comment

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

@oleksii-leonov I’m sorry I don’t see the test catching the error we’ve talked about, have I missed it? I think it is an important inclusion!

@oleksii-leonov
Copy link
Contributor Author

@BuonOmo, Input geometry must be polygonal exception could happen only if you pass non-polygonal geometry to PolygonHullSimplifier.

Since I have moved simplify_polygon_hull from ext/geos_c_impl/geometry.c to ext/geos_c_impl/polygon.c, it's impossible to trigger that exception.

@BuonOmo
Copy link
Member

BuonOmo commented Jan 6, 2024 •
edited

@oleksii-leonov indeed got you! Sorry for the delay, and thank you for the clear contribution 🌮

May I rebase/squash before merging or would you rather do it?

@oleksii-leonov
feat: add simplify_polygon_hull method
6d095ce 
The feature is equal to [ST_SimplifyPolygonHull](https://postgis.net/docs/ST_SimplifyPolygonHull.html) in PostGIS.

> Computes a simplified topology-preserving outer or inner hull of a polygonal geometry.
> An outer hull completely covers the input geometry.
> An inner hull is completely covered by the input geometry.
> The result is a polygonal geometry formed by a subset of the input vertices.
> MultiPolygons and holes are handled and produce a result with the same structure as the input.
> https://postgis.net/docs/ST_SimplifyPolygonHull.html

Utilizes the `GEOSPolygonHullSimplify` method introduced in [GEOS 3.11.0](https://github.com/libgeos/geos/releases/tag/3.11.0).

- libgeos/geos#603
- locationtech/jts#861
- libgeos/geos@1b3521c
@oleksii-leonov
fix: wrap method_geometry_simplify_polygon_hull with a guard
aec1d8a
@oleksii-leonov
refactor: move simplify_polygon_hull to polygon.c
750f38f
@oleksii-leonov
Copy link
Contributor Author

oleksii-leonov commented Jan 6, 2024 •
edited

@BuonOmo , no worries :)
I have rebased the PR. Feel free to squish & merge.

@BuonOmo BuonOmo merged commit b931b95 into rgeo:main Jan 7, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BuonOmo BuonOmo BuonOmo approved these changes

@keithdoggett keithdoggett keithdoggett approved these changes

Assignees

@BuonOmo BuonOmo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@oleksii-leonov @keithdoggett @BuonOmo