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

Replace GTS with CDT #617

Merged
merged 7 commits into from
Aug 16, 2024
Merged

Replace GTS with CDT #617

merged 7 commits into from
Aug 16, 2024

Conversation

mjcarroll
Copy link
Contributor

This removes the dependency on GTS.

  • It removes the Mesh boolean operations functions, which were part of the building editor in Gazebo-classic but are unused in modern gazebo.
  • It replaces the GTS delaunay triangulation with the header-only CDT library.

Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll self-assigned this Jul 24, 2024
Signed-off-by: Michael Carroll <[email protected]>
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

some small whitespace nits

and I've prepared a change for homebrew-simulation for after this lands: osrf/homebrew-simulation#2704

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
graphics/src/MeshManager.cc Show resolved Hide resolved
graphics/src/MeshManager.cc Show resolved Hide resolved
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

The MeshCSG code was used in Gazebo-classic to implement the Building Editor. Since there is no plan to port this to modern Gazebo (we can potentially use rmf_site for this in the future), I'd say let's go ahead and remove it from gz-common.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put CDT in a vendor subdirectory with the a README to indicate its source and version info?

Copy link
Member

Choose a reason for hiding this comment

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

👍

@iche033
Copy link
Contributor

iche033 commented Aug 12, 2024

gts dep removal in the gz-common6-release repo when this lands: gazebo-release/gz-common6-release#4

@iche033
Copy link
Contributor

iche033 commented Aug 14, 2024

I tested this with the polylines.sdf world and I think some of the triangles may be oriented incorrectly, i.e. normals are in the opposite direction:

left: GTS vs right: CDT

cdt_tri

@scpeters
Copy link
Member

I tested this with the polylines.sdf world and I think some of the triangles may be oriented incorrectly, i.e. normals are in the opposite direction:

I added a printout to the DelaunayTriangulation test to show the vertices for the triangles:

patch

diff --git a/graphics/src/DelaunayTriangulation_TEST.cc b/graphics/src/DelaunayTriangulation_TEST.cc
index df919d3..c441257 100644
--- a/graphics/src/DelaunayTriangulation_TEST.cc
+++ b/graphics/src/DelaunayTriangulation_TEST.cc
@@ -69,6 +69,32 @@ TEST_F(DelaunayTriangulation, DelaunayTriangulation)
   // same as number of vertices in the path
   EXPECT_EQ(subMesh.VertexCount(), 8u);
 
+  std::cerr << subMesh.Max() << '\n';
+  std::cerr << subMesh.Min() << '\n';
+  // Print vertices
+  std::cerr << "vertices: \n";
+  for (int v = 0; v < subMesh.VertexCount(); ++v)
+  {
+    std::cerr << subMesh.Vertex(v) << '\n';
+  }
+  // Print normals
+  std::cerr << "normals: \n";
+  for (int v = 0; v < subMesh.NormalCount(); ++v)
+  {
+    std::cerr << subMesh.Normal(v) << '\n';
+  }
+
   // there should be 8 triangles.
   EXPECT_EQ(subMesh.IndexCount() / 3u, 8u);
+  // Print triangles
+  std::cerr << "triangles: \n";
+  for (int i = 0; i < subMesh.IndexCount() / 3u; ++i)
+  {
+    int i1 = subMesh.Index(i*3 + 0);
+    int i2 = subMesh.Index(i*3 + 1);
+    int i3 = subMesh.Index(i*3 + 2);
+    std::cerr << '[' << subMesh.Vertex(i1) << "]\n"
+              << '[' << subMesh.Vertex(i2) << "]\n"
+              << '[' << subMesh.Vertex(i3) << "]\n\n";
+  }
 }

output with GTS

[ RUN      ] GTSMeshUtils.DelaunayTriangulation
1 1 0
0 0 0
vertices: 
0.75 0.75 0
1 0 0
1 1 0
0.75 0.25 0
0.25 0.75 0
0 1 0
0.25 0.25 0
0 0 0
normals: 
triangles: 
[0.75 0.75 0]
[1 1 0]
[1 0 0]

[0.75 0.25 0]
[0.75 0.75 0]
[1 0 0]

[0.25 0.75 0]
[0.25 0.25 0]
[0 1 0]

[0.75 0.25 0]
[1 0 0]
[0.25 0.25 0]

[1 0 0]
[0 0 0]
[0.25 0.25 0]

[0.25 0.25 0]
[0 0 0]
[0 1 0]

[0.75 0.75 0]
[0.25 0.75 0]
[1 1 0]

[0.25 0.75 0]
[0 1 0]
[1 1 0]

[       OK ] GTSMeshUtils.DelaunayTriangulation (1 ms)

output with CDT

[ RUN      ] DelaunayTriangulation.DelaunayTriangulation
1 1 0
0 0 0
vertices: 
0 0 0
1 0 0
1 1 0
0 1 0
0.25 0.25 0
0.25 0.75 0
0.75 0.75 0
0.75 0.25 0
normals: 
triangles: 
[0.75 0.75 0]
[1 1 0]
[0 1 0]

[0.25 0.75 0]
[0.75 0.75 0]
[0 1 0]

[0.25 0.25 0]
[0 1 0]
[0 0 0]

[0.75 0.25 0]
[0.25 0.25 0]
[1 0 0]

[0.25 0.75 0]
[0 1 0]
[0.25 0.25 0]

[0.75 0.25 0]
[1 0 0]
[0.75 0.75 0]

[1 0 0]
[1 1 0]
[0.75 0.75 0]

[1 0 0]
[0.25 0.25 0]
[0 0 0]

[       OK ] DelaunayTriangulation.DelaunayTriangulation (1 ms)

@iche033
Copy link
Contributor

iche033 commented Aug 15, 2024

I tested this with the polylines.sdf world and I think some of the triangles may be oriented incorrectly, i.e. normals are in the opposite direction:

I added a printout to the DelaunayTriangulation test to show the vertices for the triangles:

patch

output with GTS

output with CDT

Thanks, I created a fix for this in #623. It also addresses other feedback in this PR.

iche033 and others added 2 commits August 15, 2024 15:44
* Fix triangle dir, lint, style
* update migration guide
* Test that Delaunay triangles wind clockwise (#624)

---------

Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 60.56884% with 610 lines in your changes missing coverage. Please review.

Project coverage is 79.32%. Comparing base (316ea33) to head (b852307).
Report is 6 commits behind head on main.

Files Patch % Lines
graphics/src/CDT/Triangulation.hpp 63.66% 274 Missing ⚠️
graphics/src/CDT/KDTree.h 28.49% 128 Missing ⚠️
graphics/src/CDT/portable_nth_element.hpp 53.71% 56 Missing ⚠️
graphics/src/CDT/Triangulation.h 46.73% 49 Missing ⚠️
graphics/src/CDT/CDTUtils.hpp 61.20% 45 Missing ⚠️
graphics/src/CDT/predicates.h 76.68% 45 Missing ⚠️
graphics/src/DelaunayTriangulation.cc 82.14% 5 Missing ⚠️
graphics/src/CDT/CDTUtils.h 91.30% 4 Missing ⚠️
graphics/src/CDT/LocatorKDTree.h 76.47% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #617      +/-   ##
==========================================
- Coverage   80.54%   79.32%   -1.22%     
==========================================
  Files          80       87       +7     
  Lines       13588    14877    +1289     
==========================================
+ Hits        10944    11801     +857     
- Misses       2644     3076     +432     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

EXPECT_TRUE(result);

// same as number of vertices in the path
EXPECT_EQ(subMesh.VertexCount(), 8u);

// there should be 8 triangles.
EXPECT_EQ(subMesh.IndexCount() / 3u, 8u);

// verify that triangles have clockwise winding
for (int t = 0; t < subMesh.IndexCount() / 3u; ++t)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (int t = 0; t < subMesh.IndexCount() / 3u; ++t)
for (unsigned int t = 0; t < subMesh.IndexCount() / 3u; ++t)

apologies I missed the compiler warning in #624

@scpeters
Copy link
Member

I think we could ignore graphics/src/CDT in codecov.yml

Signed-off-by: Ian Chen <[email protected]>
@iche033
Copy link
Contributor

iche033 commented Aug 16, 2024

I think we could ignore graphics/src/CDT in codecov.yml

I updated the ignore files in 7ddc32a

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

@scpeters scpeters marked this pull request as ready for review August 16, 2024 17:25
@scpeters scpeters requested a review from marcoag as a code owner August 16, 2024 17:25
Signed-off-by: Steve Peters <[email protected]>
@iche033
Copy link
Contributor

iche033 commented Aug 16, 2024

removed gts dep from package.xml in a159b79

@iche033 iche033 merged commit 1f52701 into main Aug 16, 2024
10 checks passed
@iche033 iche033 deleted the mjcarroll/cdt branch August 16, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏛️ ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants