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.

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

Add support for convex hulls #83

Closed
wants to merge 4 commits into from
Closed

Add support for convex hulls #83

wants to merge 4 commits into from

Conversation

Riley19280
Copy link
Contributor

@Riley19280 Riley19280 commented Feb 28, 2023

Add support for using the ST_ConvexHull funciton.

I broke this out of the SpatialBuilder class because it does not operate on a model, only on geometry. For example, this query

select ST_CONVEXHULL(ST_GeomFromText('MULTIPOINT(-1 -1, 1 -1, 1 1, -1 1, -1 -1, 0 0)', 0, 'axis-order=long-lat')) as convex_hull

results from
SpatialQuery::make()->convexHull($geometry)

@MatanYadaev
Copy link
Owner

Hi @Riley19280, thanks for the PR!
Can you please explain to me what convex hull means? I'm not familiar with this functionality.
And, please explain me the use case. Why do you need this function?

@Riley19280
Copy link
Contributor Author

Riley19280 commented Feb 28, 2023

Hi @Riley19280, thanks for the PR! Can you please explain to me what convex hull means? I'm not familiar with this functionality. And, please explain me the use case. Why do you need this function?

A convex hull is a shape that encompasses all points given
Screen Shot 2023-02-28 at 11 19 18 AM

Im my use case, a polygon that contains all the points is what I need

@MatanYadaev
Copy link
Owner

Thanks for the explanation. Isn't it better to implement this algorithm in PHP and save the IO operation to the DB?

@Riley19280
Copy link
Contributor Author

Thanks for the explanation. Isn't it better to implement this algorithm in PHP and save the IO operation to the DB?

Could be, I am not sure though. I don't want to write it myself when I can have the database do it though. I'm sure whoever implemented it on a database did it as efficiently as they could

@MatanYadaev
Copy link
Owner

I'm sorry. I appreciate your time, but I prefer not to mess up this package with a new builder class. I assume this use case isn't very common.

I suggest you use this code in your project, on top of this package.

Let me know if there's anything else I can help with.

@Riley19280
Copy link
Contributor Author

Riley19280 commented Feb 28, 2023

I feel that having a go-to package for all spatial database queries is important. There are plenty of other spatial database functions available that could be added to the builder class, further extending the capabilities provided by this package.
For example, intersections between geometries.
Converting to/from another packages Geometry classes would be a hassle to do such an operation.

I do agree that this specific function may not be commonly used, but that should not prevent similar operations from being added to this package in the future. The new builder class would be the foundation for these things. (May be better named GeometryQuery)

I also don't mind adding a few more operations to it if you feel this PR would be more valuable this way. There is a list here

@MatanYadaev
Copy link
Owner

MatanYadaev commented Feb 28, 2023

Ok, I see what you mean. Can you make the class not inherit from Builder?
I don't have an ideal design yet, but the direction should be something like this GeometryUtils::make($connection)->convexHull($multiPoint), when make's first parameter is optional, and the function returns a Polygon without the need to do ->first()->convex_hull

@Riley19280
Copy link
Contributor Author

Ok, I see what you mean. Can you make the class not inherit from Builder? I don't have an ideal design yet, but the direction should be something like this GeometryUtils::make($connection)->convexHull($multiPoint), when make's first parameter is optional, and the function returns a Polygon without the need to do ->first()->convex_hull

I think returning a geometry class directly makes the most sense. Wanted to do that originally but it was too closely tied to queries. I like the direction it's headed now. Let me know what you think

@MatanYadaev
Copy link
Owner

MatanYadaev commented Mar 1, 2023

It looks great, I made some refactoring, can you check it? 4237bf0
I removed the distanceSphere, I prefer it to be in a separated PR.

@Riley19280
Copy link
Contributor Author

Riley19280 commented Mar 14, 2023

It looks great, I made some refactoring, can you check it? 4237bf0 I removed the distanceSphere, I prefer it to be in a separated PR.

Looks good to me!

@MatanYadaev
Copy link
Owner

@Riley19280 Can you fix the PR to include my refactoring?

@MatanYadaev MatanYadaev closed this Jun 8, 2023
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