Silence logs and light refactoring #9
Conversation
# Conflicts: # loop_cgal/bindings.cpp # src/clip.h # src/numpymesh.h
…esh-related files
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on reducing verbose logging output and applying formatting improvements to the codebase. The main goal is to silence logs that weren't controlled by the verbose argument and improve code readability through consistent formatting.
- Silenced unwanted log output by making verbose logging conditional and changing default verbose behavior
- Applied clang-format styling to C++ code for consistent formatting and readability
- Reorganized Python imports and simplified return statements
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/meshutils.cpp | Applied clang-format styling with consistent brace placement and whitespace |
| src/mesh.h | Reordered header includes and applied formatting to class and function declarations |
| src/mesh.cpp | Changed default verbose to false, removed unconditional logging, and applied formatting |
| src/clip.cpp | Reordered headers, applied formatting, and made verbose logging conditional |
| loop_cgal/init.py | Added future annotations, reorganized imports, and simplified return statement |
| void TriMesh::reverseFaceOrientation() { | ||
| // Reverse the face orientation of the mesh | ||
| PMP::reverse_face_orientations(_mesh); | ||
| if (!CGAL::is_valid_polygon_mesh(_mesh, true)) | ||
| { | ||
| std::cerr << "Error: Mesh is not valid after reversing face orientations." << std::endl; | ||
| } | ||
| // Reverse the face orientation of the mesh | ||
| PMP::reverse_face_orientations(_mesh); | ||
| if (!CGAL::is_valid_polygon_mesh(_mesh, false)) { |
There was a problem hiding this comment.
The verbose parameter was changed from true to false, but this appears to be inconsistent with the function's verbose parameter. If the function receives verbose=true, the validation should also use the verbose flag for consistency.
Copilot uses AI. Check for mistakes.
| #endif | ||
| if (!beautify_flag) { | ||
| std::cout << "Removing degenerate faces failed." << std::endl; | ||
| std::cerr << "Removing degenerate faces failed." << std::endl; |
There was a problem hiding this comment.
Changed from std::cout to std::cerr for error messages, which is correct, but this should be applied consistently to line 380 as well where the same error message still uses std::cerr.
Copilot uses AI. Check for mistakes.
| if (verbose) | ||
| std::cout << "Meshes do not intersect. Returning tm." << std::endl; |
There was a problem hiding this comment.
[nitpick] The verbose check is missing braces around the single statement. For consistency with the rest of the codebase and to prevent potential bugs when adding more statements, consider using braces: if (verbose) {
| if (verbose) | |
| std::cout << "Meshes do not intersect. Returning tm." << std::endl; | |
| if (verbose) { | |
| std::cout << "Meshes do not intersect. Returning tm." << std::endl; | |
| } |
Copilot uses AI. Check for mistakes.
| if (verbose) | ||
| CGAL::is_valid_polygon_mesh(_tm, true); |
There was a problem hiding this comment.
[nitpick] Same issue as line 264 - missing braces around the single statement after the verbose check.
| if (verbose) | |
| CGAL::is_valid_polygon_mesh(_tm, true); | |
| if (verbose) { | |
| CGAL::is_valid_polygon_mesh(_tm, true); | |
| } |
Copilot uses AI. Check for mistakes.
| if (verbose) | ||
| std::cout << "Meshes do not intersect. Returning tm." << std::endl; |
There was a problem hiding this comment.
[nitpick] Same issue as previous verbose checks - missing braces around the single statement.
| if (verbose) | |
| std::cout << "Meshes do not intersect. Returning tm." << std::endl; | |
| if (verbose) { | |
| std::cout << "Meshes do not intersect. Returning tm." << std::endl; | |
| } |
Copilot uses AI. Check for mistakes.
| if (verbose) | ||
| std::cout << "Found " << tm_1_shared_edges.size() | ||
| << " shared edges in tm1 and " << tm_2_shared_edges.size() | ||
| << " shared edges in tm2." << std::endl; |
There was a problem hiding this comment.
[nitpick] Same issue as previous verbose checks - missing braces around the single statement.
| if (verbose) | |
| std::cout << "Found " << tm_1_shared_edges.size() | |
| << " shared edges in tm1 and " << tm_2_shared_edges.size() | |
| << " shared edges in tm2." << std::endl; | |
| if (verbose) { | |
| std::cout << "Found " << tm_1_shared_edges.size() | |
| << " shared edges in tm1 and " << tm_2_shared_edges.size() | |
| << " shared edges in tm2." << std::endl; | |
| } |
Copilot uses AI. Check for mistakes.
| if (verbose) | ||
| std::cout << "Corefinement done." << std::endl; |
There was a problem hiding this comment.
[nitpick] Same issue as previous verbose checks - missing braces around the single statement.
| if (verbose) | |
| std::cout << "Corefinement done." << std::endl; | |
| if (verbose) { | |
| std::cout << "Corefinement done." << std::endl; | |
| } |
Copilot uses AI. Check for mistakes.
I opened this PR to silence all the logs we're getting that were not silenced by the
verboseargument.Also added some light reformatting on the Python and C++ code for better readability.
clang-format -i /loop-cgal/src/*More details (Copilot summary):
Python Code Improvements:
loop_cgal/__init__.pyto use future annotations, reordered imports for clarity, and grouped related imports together.clip_pyvista_polydatato directly return the result ofpv.PolyData.from_regular_faceswithout assigning it to an intermediate variable.C++ Code Improvements:
src/clip.cppfor better organization.src/clip.cppto improve code compactness. [1] [2]clip_plane,clip_surface, andcorefine_meshto remove redundant line breaks and improve readability. [1] [2] [3] [4] [5]Logging and Verbosity Adjustments:
verboseflag is set. Examples includeclip_plane,clip_surface, andcorefine_mesh. [1] [2] [3]verboseparameter tofalsein theTriMeshconstructor insrc/mesh.cpp, reducing unnecessary logging during object initialization.Formatting and Consistency:
src/mesh.cppfor consistency and readability. [1] [2] [3]std::coutwithstd::cerrfor error logs where appropriate. [1] [2]These changes collectively enhance the readability, maintainability, and reliability of the codebase while ensuring consistent behavior across the library.