DYN-9099 Voronoi Delaunay nodes fix#16862
Conversation
Previously the Voronoi diagram in 2D (UV space) vs the one generated by the Voronoi.ByParametersOnSurface node were different due that the 2D version was being mapped to a Surface (3D). Then for fixing the problem we had to scale UVs by the aspect radio to a “metric-corrected” vertex and then build the Voronoi, this fix still improves consistency but can’t guarantee perfect equality with 2D unless the surface mapping is linear.
Updating the MIConvexHull to the latest version and fixing the errors that it generated. Also by consequence the Delaunay algorithm was updated.
after updating the MIConvexHull package the ByPoints method was sending an error when compiling Dynamo due that in triResult.Faces was not found inside the structure so I had to apply a minor fix.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9099
There was a problem hiding this comment.
Pull request overview
This PR fixes issues with Voronoi diagram and Delaunay triangulation calculations on surfaces by introducing aspect-ratio-aware scaling. The changes address a problem where transforming points before computing a Voronoi diagram produces different results than computing it first and then transforming (Voronoi(Stretch(points)) != Stretch(Voronoi(points))). The fix scales UV coordinates by the surface's aspect ratio before computation, then maps results back to the original UV space.
Changes:
- Added aspect-ratio-aware scaling to Voronoi and Delaunay algorithms to correct for non-uniform UV-to-physical mappings
- Updated MIConvexHull package from version 1.1.17.411 to 1.1.19.1019
- Added input validation and tolerance parameters for improved robustness
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Voronoi.cs | Implements aspect-ratio scaling by measuring physical distances at surface corners, scaling UV coordinates proportionally before Voronoi computation, then mapping back to original UV space |
| Delaunay.cs | Applies the same aspect-ratio scaling approach to Delaunay triangulation, adds plane distance tolerance parameter, and fixes variable naming consistency (l1→l2, l1→l3) |
| Tessellation.csproj | Updates MIConvexHull package dependency to version 1.1.19.1019 |
| ConvexHull.cs | Updates ConvexHull API call to match new package version (removes tolerance parameter, accesses Result.Faces) |
| @@ -19,10 +19,10 @@ public static IEnumerable<Curve> ByPoints(IEnumerable<Point> points) | |||
| { | |||
| var verts = points.Select(Vertex3.FromPoint).ToList(); | |||
| const double DefaultPlaneDistanceTolerance = 1e-6; | |||
There was a problem hiding this comment.
The constant DefaultPlaneDistanceTolerance is defined but no longer used after the API update. This unused constant should be removed to avoid confusion.
| const double DefaultPlaneDistanceTolerance = 1e-6; |
|
GIF Voronoi showing the expected behavior for Surface (10 x 17) |
Removing unused const
Build failing due that is checking the dlll version but should not be checked.
aparajit-pratap
left a comment
aparajit-pratap
left a comment
There was a problem hiding this comment.
Can you add tests?
Fixing wrongly added packages (after merging master to feature branch)
two test were added for validating that the algorithms updated for Voronoi and Delaunay will work correctly (also both tests were executed using the previous algorithms and are failing).
|
I've updated the Tessellation.csproj due that after merging master into my feature branch some extra packages entries were added.
Both ByParametersOnSurface methods in Delaunay and Voronoi were using the same code for Normalize UV scales so the code was moved to a method in Delaunay class and reused in Voronoi (this avoid code duplication). By consequence after adding the method the Tessellation.xml file was updated with the new method.
Fixing conflicts in the Tessellation.csproj file
Fixing conflicts
I've added the tests and now all the check are passing, any other comment @aparajit-pratap ? |
Fixing error produced when merging master into the feature branch.
re-tiggering job
Updated the LibG reference in the Tessellation.Test.csproj
I've updated the ComputeDelaunayTrianglesInScaledUvSpace method for extracting the triangles from the actual Delaunay implementation using the same algorithm as production code.
Fixing the tests so now the results of ByParametersOnSurface are compared against the result of ComputeDelaunayTrianglesInScaledUvSpace
| var triangles = ComputeDelaunayTrianglesInScaledUvSpace(uvs, surface); | ||
| Assert.That(triangles.Count, Is.GreaterThan(0)); | ||
|
|
||
| AssertTrianglesSatisfyEmptyCircumcircle(triangles); |
There was a problem hiding this comment.
The empty circumcircle property is verified only on the output of the test helper function (ComputeDelaunayTrianglesInScaledUvSpace), not on what ByParametersOnSurface actually returns. This is essentially testing MIConvexHull itself — not the production code - Delaunay.ByParametersOnSurface .
There was a problem hiding this comment.
To truly test the empty circumcircle property on the production code, you'd need to reconstruct triangles from the edges returned by Delaunay.ByParametersOnSurface — grouping shared edges back into triangles — and then run AssertTrianglesSatisfyEmptyCircumcircle on those. Not sure yet on how you can do this easily, though.
There was a problem hiding this comment.
Related to your first comment I've added an extra validation AssertApiEdgesSatisfyEmptyCircumcircle (and other helper methods) in the Delaunay test so now we are verifying empty-circumcircle on triangles reconstructed from the production API edge output. see commit: f80ed6f
For your second comment as we already comment we need to add a new node which returns the triangles instead a flat list of lines, so this need to be handled in a different task.
In the UvScalingUtilities.cs file I've modified the GetNormalizedUvScales(Surface face) method, basically updated the UV scale computation to use iso-curve lengths on the surface edges and dispose the temporary curves. Also added an extra validation AssertApiEdgesSatisfyEmptyCircumcircle (and other helper methods) in the Delaunay test so now we are verifying empty-circumcircle on triangles reconstructed from the production API edge output.
Fixes done after merging master into feature branch
Fixes done after merging master into feature branch
Removed project reference to Tessellation.csproj due that is not needed
| const double spatialTolerance = 1e-3; | ||
| const double minEdgeLength = 0.1; |
There was a problem hiding this comment.
How did you come up with these constants? They seem like relatively large tolerances. Usually tolerances are in the order of at least 1e-6.
There was a problem hiding this comment.
Previously with 1e-6 the tests were failing (so were modified until we reach 1e-3), it is technically possible in some cases, but for this test setup it is unlikely to be stable at 1e-6.
Why 1e-6 is risky here:
- This assertion compares world-space endpoints generated through multiple geometric conversions:
- scaled UV triangulation
- unscale back to UV
- Surface.PointAtParameter
- API-generated curve endpoints
- Those paths are not guaranteed bit-identical; tiny numeric differences accumulate.
- On a 1000 x 1 anisotropic surface, strict 1e-6 endpoint matching tends to be too brittle for regression tests.
aparajit-pratap
left a comment
aparajit-pratap
left a comment
There was a problem hiding this comment.
- One more question about your choice of tolerances used in the test functions.
- I'm also curious whether these tests fail without UV (aspect-ratio) scaling.
- Finally, are there any existing tests for Voronoi and Delaunay that continue to pass after these changes? If there were bugs, then how were these tests passing earlier, if at all they were passing?
Then, LGTM.
About the second question, yes, I asked copilot to remove the UV (aspect-ratio) scaling fix and the tests were failing and then after the change was restored they continue passing. 
About your last question the only test that I found checking Voronoi was VoronoiByParameterOnSurface_MAGN_8039(). |
|
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="MIConvexHull" version="1.1.19.1019" CopyPDB="true" /> | ||
| <PackageReference Include="StarMath" version="2.0.17.1019" CopyPDB="true" /> |
There was a problem hiding this comment.
@RobertGlobant20 have you explored updating the StarMath library as well? It's used specifically in this project for Voronoi and Delaunay computations.
There was a problem hiding this comment.
StarMath is used as a small linear-algebra helper for circumcenter calculations. I can see that inside Cell2 class we are calling StarMath.determinant to compute triangle circumcenters from determinant formulas.
Purpose
Previously the manually calculated Voronoi diagram in 2D (in this case the Voronoi first was calculated in UV space and then mapped to the surface) was different than the one generated by the Voronoi.ByParametersOnSurface node due that for the latest first create the surface (using a Rectangle 10 x 17) and then we create the Voronoi.
Voronoi(Stretch(points)) != Stretch(Voronoi(points))Then for fixing the problem we had to scale UVs by the aspect radio to a “metric-corrected” vertex (if the aspect ratio is 1:1 no change) and then build the Voronoi, this fix still improves consistency but can’t guarantee perfect equality with 2D unless the surface mapping is linear.
Also the MIConvexHull package inside the Tessellation project was updated to the latest version.
Declarations
Check these if you believe they are true
Release Notes
Fixing Voronoi diagram and Delaunay triangulation and updating the MIConvexHull package inside the Tessellation project.
Reviewers
@aparajit-pratap @zeusongit @chubakueno
FYIs