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 Voronoi diagrams with zero dependency delaunay triangulation #1144

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

doc-E-brown
Copy link

@doc-E-brown doc-E-brown commented Jan 30, 2024

This PR adds the VoronoiDiagram trait to create Voronoi diagrams from Polygon and MultiPoint types. The Voronoi diagram is created using the property that voronoi diagrams are the dual of delaunay triangulation.

The VoronoiDiagram trait uses an added TriangulateDelaunay trait that computes Delaunay triangles without any additional dependencies. As per the conversation here this work was started before the introduction of the spade methodology. As requested in the conversation I've added a bench performance test to compare the different methods and these are the results when running on my laptop.

Delaunay Triangulation  time:   [2.2839 µs 2.3687 µs 2.4780 µs]
                        change: [-21.682% -14.728% -7.0971%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

Spade Unconstrained Triangulation
                        time:   [1.4216 µs 1.4831 µs 1.5582 µs]
                        change: [+39.411% +46.250% +53.808%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 7 outliers among 100 measurements (7.00%)
  5 (5.00%) high mild
  2 (2.00%) high severe

Constrained Outer Triangulation
                        time:   [3.5185 µs 3.7830 µs 4.1260 µs]

Constrain Triangulation time:   [3.5962 µs 3.7355 µs 3.8970 µs]
Found 15 outliers among 100 measurements (15.00%)
  2 (2.00%) low mild
  4 (4.00%) high mild
  9 (9.00%) high severe

the TriangulateDelaunay trait is faster then the Constrained Outer Triangulation method but not the Unconstrained Method.

The original intent behind implementing Delaunay triangulation in geo arose when we needed a method that was compliant with the Apache license using geo-types and was not otherwise available.

Thank you for considering this PR.

  • 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.

@doc-E-brown doc-E-brown force-pushed the franklin-to-upstream branch 5 times, most recently from b5f6b05 to d5a6cd6 Compare January 30, 2024 00:43
@doc-E-brown doc-E-brown changed the title Franklin to upstream Add Voronoi diagrams with zero dependency delaunay triangulation Jan 30, 2024
@doc-E-brown doc-E-brown marked this pull request as ready for review January 30, 2024 00:56
Copy link
Contributor

@RobWalt RobWalt left a comment

Choose a reason for hiding this comment

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

Sorry it took me a while to review this. I started with just the Delauny triangulation part. This is just a first iteration cleaning the code up here and there. I didn't really go through how the algorithm works yet. Will do that in a follow up review.

Overall, even though I left some comments, I think the code is really clean and the comments were really helpful so far! Also thanks for taking the time to create nice Errors which implement Display! And I'm really happy to see this implemented natively in geo. Although I also like spade for its API, I can't understand its code which isn't so nice 🙈

geo/src/algorithm/triangulate_delaunay.rs Outdated Show resolved Hide resolved
geo/src/algorithm/triangulate_delaunay.rs Outdated Show resolved Hide resolved
geo/src/algorithm/triangulate_delaunay.rs Outdated Show resolved Hide resolved
geo/src/algorithm/triangulate_delaunay.rs Outdated Show resolved Hide resolved
geo/src/algorithm/triangulate_delaunay.rs Outdated Show resolved Hide resolved
geo/src/algorithm/triangulate_delaunay.rs Outdated Show resolved Hide resolved
geo/src/algorithm/triangulate_delaunay.rs Outdated Show resolved Hide resolved
processed_points.push(*pt);
}
}
let triangles = remove_super_triangle(&triangles, &super_triangle);
Copy link
Contributor

Choose a reason for hiding this comment

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

Questions:

  • Can it happen that the super_triangle gets removed during the algorithm?
  • If not, can it happen that the super_triangle gets moved away from the first position to somewhere else in the Vec

Depending on the answers, the removal might be easier than a full search through the whole vec as done in remove_super_triangle

Copy link
Author

Choose a reason for hiding this comment

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

No the super_triangle isn't removed during the process. The scaffolding provided by the super_triangle will remain as well as other triangles formed from it https://commons.wikimedia.org/wiki/File:Bowyer-Watson_6.png#/media/File:Bowyer-Watson_6.png In this diagram the super_triangle and corresponding artefacts are shown in red while the final delaunay triangulation is shown in below.

The process of removing the super_triangle also requires removing triangles that share vertices with the super_triangle and these triangles can be located anywhere in the vec.

geo/src/algorithm/triangulate_delaunay.rs Outdated Show resolved Hide resolved
geo/src/algorithm/triangulate_delaunay.rs Outdated Show resolved Hide resolved
@doc-E-brown
Copy link
Author

doc-E-brown commented Feb 6, 2024

Thanks so much for your thorough review @RobWalt! I'll work through them and have an update soon.

@doc-E-brown
Copy link
Author

Hi @RobWalt sorry for the delay, I've updated the PR as per your comments. It definitely looks much cleaner! Thanks!

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

2 participants