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

another convex hull / GrahamScan function added #401

Closed
wants to merge 2 commits into from

Conversation

ig-or
Copy link

@ig-or ig-or commented Apr 28, 2023

faster convex hull function added. I tested it an it works fast and reliable for my case in 10 millions hulls.
what is not perfect:

  • not using existing Meshes.jl data structures like Point etc.
  • Probably not work correctly with corner cases (like making a hull from just two points)

@juliohm
Copy link
Member

juliohm commented Apr 28, 2023

@ig-or appreciate the attempt to add a faster implementation here. Can you please consider improving our existing Graham scan implementation instead?

The low level function you implemented could be refactored and then used behind a more high-level interface with Vector{Point} etc.

@DanielVandH
Copy link
Member

This implementation seems to fail for this (contrived) example:

n = 4
p1 = [27.643564356435643, -21.881188118811881]
p2 = [83.366336633663366, 15.544554455445542]
p3 = [4.0, 4.0]
p4 = [73.415841584158414, 8.8613861386138595]
p = hcat(p1, p2, p3, p4)

h = zeros(Int32, n)
v = zeros(Float64, n)
s = zero(h)
u = hull_simple(p, h, s, v)
s = s[1:u]
push!(s, s[begin])

fig = Figure()
ax = Axis(fig[1, 1])
scatter!(ax, p[1, :], p[2, :], color=:red)
lines!(ax, p[1, s], p[2, s])

fig

image

Meshes.jl's existing implementation works, though.

_p = PointSet(p)
viz(hull(_p, GrahamScan()))

image

@juliohm
Copy link
Member

juliohm commented Apr 29, 2023

Thank you for constructing the example @DanielVandH .

@ig-or I will close the PR because it is not fitting our interface, and because it has the bug mentioned above. I am happy to review future PRs with improvements to our hull implementation.

@juliohm juliohm closed this Apr 29, 2023
@ig-or
Copy link
Author

ig-or commented May 3, 2023

Thanks for looking at my code. I fixed this issue with several points on one line on my side. But as I said make this fitting your interface might be more challenging (but possible). The main idea of the increase of speed is to remove all the memory allocations from the code.

I realized my use case is not common. I'm creating millions of convex hulls from just a few points. Usually people have millions of points and need make one convex hull out of it.

@DanielVandH
Copy link
Member

FYI @ig-or, you might be interested to test on some of the cases discussed in this paper https://doi.org/10.1016/j.comgeo.2007.06.003 (it's where my example from above comes from) for convex hulls. They are all for convex hulls of very small point sets.

@ig-or
Copy link
Author

ig-or commented May 3, 2023

Thanks Daniel!

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.

None yet

3 participants