brick / geo

GIS geometry library for PHP
MIT License
220 stars 31 forks source link

Add `GeometryCollection::toSet()` method #18

Closed Kolyunya closed 3 years ago

Kolyunya commented 3 years ago

I'd like to propose to add a functionality of making geometry collections unique.

This is just a quick proof of concept. Maybe the method should be moved to Geometry. Maybe it should be named differently. Maybe it should modify the collection instead of creating a new one. Maybe it should be a flag in one of the factory methods. There are really a lot of possible options to implement this...

I would be glad to hear any comments and/or recommendations.

P.S. Figuring out that splatting gotcha was a nightmare :rofl:

BenMorel commented 3 years ago

Hi, I'm not sure to want to add this feature to the library: where do you get duplicate geometries from? Understanding the use case might help here.

If we do add this feature:

So, apart maybe from the naming, your implementation looks OK to me. The only things that bother me really, are:

BenMorel commented 3 years ago

Another thing that comes to my mind is: if the GeometryCollection contains other collections like MultiPoint, say:

GEOMETRYCOLLECTION(
    MULTIPOINT((1 2), (3 4)),
    MULTIPOINT((3 4), (1 2))
)

Your algorithm (converting to WKB) will not detect that the contained geometries are equal, and will fail to deduplicate them.

Using Geometry::equals() on each pair of geometries would get us closer to the goal, but would be computationally expensive.

All in all, I think this is a problem that's hard to solve well, and it would be maybe better tackled in your own application if you just need a simple binary comparison, or in a third-party library that would extend brick/geo's capabilities if you want a deeper comparison algorithm!

BenMorel commented 3 years ago

As it stands, I don't think this functionality should be implemented in the library, as there are many ways to implement it, each with its own drawbacks (binary or ST_Equals() equality, performance). It's probably better done in your own app, where you can apply your own algo that works for you.

@Kolyunya Please let me know if you're planning to PR other changes that could break BC, otherwise I think we're ready to release version 0.4.0!

Kolyunya commented 3 years ago

@BenMorel exactly the same geometries may occur for example when they are stored in the database, queried by separate routines and then merged together. I don't know, maybe my use case is too specific...

Not planning to make any breaking changes at the moment. Thanks!

BenMorel commented 3 years ago

Thanks for the heads up, @Kolyunya. Version 0.4.0 is released!

I don't know if your use-case is that specific, but it's easy enough to include your deduplication algorithm in a helper class in your app!