Add option to deduplicate edges in edge triangulation #64
Conversation
WalkthroughThe pull request introduces enhanced polygon triangulation capabilities by adding functionality to handle polygons with repeated edges. A new function Changes
Sequence DiagramsequenceDiagram
participant User
participant TriangulateFn as Triangulate Function
participant SplitEdgesFn as Split Edges Function
User->>TriangulateFn: Call with polygon and split_edges=True
TriangulateFn->>SplitEdgesFn: Preprocess polygon
SplitEdgesFn-->>TriangulateFn: Return split polygons
TriangulateFn->>TriangulateFn: Triangulate each polygon
TriangulateFn-->>User: Return triangulation result
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/PartSegCore_compiled_backend/triangulate.pyx (3)
431-431: Add docstring references for the new parameter.The function gained a new parameter
split_edges, but the docstring does not mention it. Consider updating the docstring to clarify its usage and expected behavior.
434-434: Limit scope ofedge_split_listto avoid accidental carryover.Declaring
edge_split_listinside the loop might reduce accidental data retention and improve maintainability.@@ -431,2 +431,3 @@ def triangulate_polygon_with_edge_numpy_li(polygon_li: list[np.ndarray], split_edges: bool=False): cdef vector[Point] polygon_vector + cdef vector[vector[Point]] edge_split_list cdef vector[vector[Point]] polygon_vector_list
519-535: Clarify behavior for small polygons.When the input polygon has fewer than two points, this function may return empty or degenerate results. Consider validating input or documenting this behavior.
src/PartSegCore_compiled_backend/triangulation/triangulate.hpp (2)
6-6: Remove unused include if not needed.
#include <queue>does not appear to be referenced. Consider removing it.
1304-1346: Remove unusedvisitedvector.Line 1307 declares a local
visitedvector that is never used, which can cause confusion. Also confirm if zero-length polygons need handling.- std::vector<bool> visited(polygon.size(), false);pyproject.toml (1)
12-12: Consider making Debug mode configurable.Setting
cmake.build-type = "Debug"globally could impact performance in production builds. Debug builds:
- Include debugging symbols
- Disable optimizations
- Increase binary size
- Result in slower runtime performance
Consider one of these approaches:
- Use environment variables to control build type
- Make it configurable via command-line arguments
- Use different settings for development vs. production builds
Here's an example implementation using environment variables:
-cmake.build-type = "Debug" +cmake.build-type = { env = "CMAKE_BUILD_TYPE", default = "Release" }This allows developers to enable Debug mode when needed:
CMAKE_BUILD_TYPE=Debug pip install .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyproject.toml(1 hunks)src/PartSegCore_compiled_backend/triangulate.pyx(4 hunks)src/PartSegCore_compiled_backend/triangulation/triangulate.hpp(2 hunks)src/tests/test_triangulate.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test on windows-latest with Python 3.12
- GitHub Check: Test on windows-latest with Python 3.11
- GitHub Check: Test on windows-latest with Python 3.10
- GitHub Check: Test on windows-latest with Python 3.9
🔇 Additional comments (6)
src/PartSegCore_compiled_backend/triangulate.pyx (2)
97-97: Consider adding usage doc or reference to docstring.Everything looks correct with the function signature. It might be helpful to document it thoroughly in the Python layer for discoverability.
459-466: Double-check concurrency safety under nogil.Invoking
split_polygon_on_repeated_edgesandtriangulate_path_edgewithincython.nogilsuggests thread safety. Verify that these routines do not modify shared global state.✅ Verification successful
Thread safety verified for nogil blocks
The functions
split_polygon_on_repeated_edgesandtriangulate_path_edgeare thread-safe. They operate purely on their input parameters and local variables, with no shared mutable state between threads. All vector operations are performed on thread-local instances.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for shared global or thread_local usage in triangulation/*.hpp ast-grep --pattern $'static $_ = $_;' ast-grep --pattern $'thread_local $_ = $_;'Length of output: 83
Script:
#!/bin/bash # Find relevant source files echo "=== Searching for relevant source files ===" fd -e pyx -e pxd -e cpp -e h -e hpp echo -e "\n=== Searching for function definitions ===" rg -l "split_polygon_on_repeated_edges|triangulate_path_edge" echo -e "\n=== Checking for global variables ===" rg "^[[:space:]]*(static|thread_local|global)\s+\w+"Length of output: 3527
Script:
#!/bin/bash # Check implementation details echo "=== Function implementations in triangulate.pyx ===" rg -A 10 "def (split_polygon_on_repeated_edges|triangulate_path_edge)" src/PartSegCore_compiled_backend/triangulate.pyx echo -e "\n=== C++ implementations in triangulate.hpp ===" rg -A 10 "(split_polygon_on_repeated_edges|triangulate_path_edge)" src/PartSegCore_compiled_backend/triangulation/triangulate.hpp echo -e "\n=== Vector operations and memory management ===" rg "vector[<]" src/PartSegCore_compiled_backend/triangulation/triangulate.hppLength of output: 6822
src/tests/test_triangulate.py (3)
13-13: No issues with the updated import.Adding
split_polygon_on_repeated_edges_pyaligns cleanly with the new functionality.
665-673: Test thoroughly verifies the no-split scenario.The test checks that a square polygon remains a single outer boundary without splits. Good basic coverage.
684-691: Parametrized test successfully covers both split and non-split modes.Checking triangle counts for both
split_edges=Trueandsplit_edges=Falseensures robust test coverage.src/PartSegCore_compiled_backend/triangulation/triangulate.hpp (1)
1290-1302: Simple, clear adjacency structures.
GraphEdgeandGraphNodeproperly capture necessary state for repeated-edge tracking. No immediate issues found.
| def test_split_polygon_on_repeated_edges_py_square_in_square(): | ||
| res = split_polygon_on_repeated_edges_py() | ||
| assert len(res) == 2 | ||
| assert len(res[0]) == 5 | ||
| assert len(res[1]) == 5 | ||
| # idx = res[0].index((0, 0)) | ||
| # assert res[0][idx:] + res[0][:idx] == [(0, 0), (0, 1), (1, 1), (1, 0)] | ||
|
|
There was a problem hiding this comment.
Fix the missing polygon argument.
split_polygon_on_repeated_edges_py() requires a polygon parameter. Calling it without arguments will raise an error.
-def test_split_polygon_on_repeated_edges_py_square_in_square():
- res = split_polygon_on_repeated_edges_py()
+def test_split_polygon_on_repeated_edges_py_square_in_square():
+ square_in_square = [
+ [0, 0], [0, 1], [1, 1], [1, 0], [0, 0],
+ [0, 1], [1, 1], [1, 0] # Example: second square overlapping the first
+ ]
+ res = split_polygon_on_repeated_edges_py(square_in_square)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_split_polygon_on_repeated_edges_py_square_in_square(): | |
| res = split_polygon_on_repeated_edges_py() | |
| assert len(res) == 2 | |
| assert len(res[0]) == 5 | |
| assert len(res[1]) == 5 | |
| # idx = res[0].index((0, 0)) | |
| # assert res[0][idx:] + res[0][:idx] == [(0, 0), (0, 1), (1, 1), (1, 0)] | |
| def test_split_polygon_on_repeated_edges_py_square_in_square(): | |
| square_in_square = [ | |
| [0, 0], [0, 1], [1, 1], [1, 0], [0, 0], | |
| [0, 1], [1, 1], [1, 0] # Example: second square overlapping the first | |
| ] | |
| res = split_polygon_on_repeated_edges_py(square_in_square) | |
| assert len(res) == 2 | |
| assert len(res[0]) == 5 | |
| assert len(res[1]) == 5 | |
| # idx = res[0].index((0, 0)) | |
| # assert res[0][idx:] + res[0][:idx] == [(0, 0), (0, 1), (1, 1), (1, 0)] |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/PartSegCore_compiled_backend/triangulation/triangulate.hpp (4)
1291-1295: Add documentation for the GraphEdge structure.The structure is well-designed but would benefit from documentation explaining its purpose and member variables.
Add documentation like this:
+/** + * Represents an edge in a graph structure used for polygon processing. + * Each edge contains a reference to its opposite point and a flag to track + * if it has been visited during graph traversal. + */ struct GraphEdge { point::Point opposite_point; //< The point at the other end of the edge bool visited; //< Flag to mark if edge has been visited explicit GraphEdge(point::Point p) : opposite_point(p), visited(false) {} };
1297-1303: Add documentation and consider const correctness.The structure would benefit from documentation and potential const member functions.
Add documentation and consider these improvements:
+/** + * Represents a node in a graph structure used for polygon processing. + * Each node contains its edges, a sub-index for traversal tracking, + * and a visited flag for graph traversal. + */ struct GraphNode { std::vector<GraphEdge> edges; std::size_t sub_index; bool visited; GraphNode() : sub_index(0), visited(false) {} + + // Consider adding const member functions for querying state + bool is_visited() const { return visited; } + std::size_t get_sub_index() const { return sub_index; } + const std::vector<GraphEdge>& get_edges() const { return edges; } };
1312-1323: Consider extracting graph construction into a separate function.The graph construction logic could be clearer if extracted into a helper function.
Consider refactoring like this:
+inline std::unordered_map<point::Point, GraphNode> build_edges_graph( + const std::vector<point::Point> &polygon, + const std::unordered_set<point::Segment> &edges_set) { + std::unordered_map<point::Point, GraphNode> edges_map; + point::Segment segment; + + for (std::size_t i = 0; i < polygon.size() - 1; i++) { + segment = {polygon[i], polygon[i + 1]}; + if (edges_set.count(segment) > 0) { + edges_map[polygon[i]].edges.emplace_back(polygon[i + 1]); + } + } + + segment = {polygon.back(), polygon.front()}; + if (edges_set.count(segment) > 0) { + edges_map[polygon.back()].edges.emplace_back(polygon.front()); + } + + return edges_map; +}
1324-1345: Extract polygon construction logic into a separate function.The polygon construction logic is complex and would benefit from being extracted into a helper function with clearer variable names.
Consider refactoring like this:
+inline std::vector<point::Point> construct_new_polygon( + point::Point start_point, + GraphNode* start_node, + std::unordered_map<point::Point, GraphNode>& edges_map) { + std::vector<point::Point> new_polygon; + new_polygon.push_back(start_point); + + auto* current_node = start_node; + while (current_node->sub_index < current_node->edges.size()) { + auto* prev_node = current_node; + auto next_point = current_node->edges[current_node->sub_index].opposite_point; + current_node = &edges_map.at(next_point); + prev_node->sub_index++; + current_node->visited = true; + new_polygon.push_back(next_point); + } + + // Remove duplicate points at start/end + while (new_polygon.size() > 1 && new_polygon.front() == new_polygon.back()) { + new_polygon.pop_back(); + } + + return new_polygon; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/PartSegCore_compiled_backend/triangulation/triangulate.hpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test on windows-latest with Python 3.12
- GitHub Check: Test on windows-latest with Python 3.11
- GitHub Check: Test on windows-latest with Python 3.10
- GitHub Check: Test on windows-latest with Python 3.9
🔇 Additional comments (1)
src/PartSegCore_compiled_backend/triangulation/triangulate.hpp (1)
6-6: LGTM! Good documentation of platform-specific requirement.The comment clearly explains why the memory header is required on Linux but not on macOS.
| inline std::vector<std::vector<point::Point>> split_polygon_on_repeated_edges( | ||
| const std::vector<point::Point> &polygon) { | ||
| auto edges_dedup = calc_dedup_edges({polygon}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for the function and validate input.
The function lacks documentation explaining its purpose, algorithm, and parameters. It also needs input validation.
Add documentation and input validation:
+/**
+ * Splits a polygon into multiple polygons where edges are repeated.
+ *
+ * This function processes a polygon and splits it into multiple polygons
+ * at points where edges are repeated. It uses a graph-based approach
+ * where each point becomes a node and edges are used to traverse and
+ * construct the resulting polygons.
+ *
+ * @param polygon Vector of points representing the input polygon
+ * @return Vector of polygons (each a vector of points) after splitting
+ * @throws std::invalid_argument if polygon has less than 3 points
+ */
inline std::vector<std::vector<point::Point>> split_polygon_on_repeated_edges(
const std::vector<point::Point> &polygon) {
+ if (polygon.size() < 3) {
+ throw std::invalid_argument("Polygon must have at least 3 points");
+ }
auto edges_dedup = calc_dedup_edges({polygon});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline std::vector<std::vector<point::Point>> split_polygon_on_repeated_edges( | |
| const std::vector<point::Point> &polygon) { | |
| auto edges_dedup = calc_dedup_edges({polygon}); | |
| /** | |
| * Splits a polygon into multiple polygons where edges are repeated. | |
| * | |
| * This function processes a polygon and splits it into multiple polygons | |
| * at points where edges are repeated. It uses a graph-based approach | |
| * where each point becomes a node and edges are used to traverse and | |
| * construct the resulting polygons. | |
| * | |
| * @param polygon Vector of points representing the input polygon | |
| * @return Vector of polygons (each a vector of points) after splitting | |
| * @throws std::invalid_argument if polygon has less than 3 points | |
| */ | |
| inline std::vector<std::vector<point::Point>> split_polygon_on_repeated_edges( | |
| const std::vector<point::Point> &polygon) { | |
| if (polygon.size() < 3) { | |
| throw std::invalid_argument("Polygon must have at least 3 points"); | |
| } | |
| auto edges_dedup = calc_dedup_edges({polygon}); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/PartSegCore_compiled_backend/triangulation/triangulate.hpp (1)
1301-1313: Consider adding const member functions for better const correctness.The GraphNode structure is well-documented and designed. Consider adding const member functions if you need to access the node's state in a const context.
Apply this diff to improve const correctness:
struct GraphNode { std::vector<GraphEdge> edges; std::size_t sub_index; bool visited; GraphNode() : sub_index(0), visited(false) {} + + [[nodiscard]] bool is_visited() const { return visited; } + [[nodiscard]] std::size_t get_sub_index() const { return sub_index; } + [[nodiscard]] const std::vector<GraphEdge>& get_edges() const { return edges; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/PartSegCore_compiled_backend/triangulation/triangulate.hpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test on windows-latest with Python 3.12
- GitHub Check: Test on windows-latest with Python 3.11
- GitHub Check: Test on windows-latest with Python 3.10
- GitHub Check: Test on windows-latest with Python 3.9
🔇 Additional comments (3)
src/PartSegCore_compiled_backend/triangulation/triangulate.hpp (3)
6-6: LGTM! Clear documentation of platform-specific requirement.The memory header inclusion with its explanatory comment helps maintain cross-platform compatibility.
1290-1300: LGTM! Well-documented and properly designed structure.The GraphEdge structure is well-documented and follows good design practices with proper member initialization.
1314-1327: Add documentation for edge cases and exceptions.The function documentation should include information about:
- Edge cases (empty polygon, single point, etc.)
- Exceptions that may be thrown
- Return value for edge cases
| inline std::vector<std::vector<point::Point>> split_polygon_on_repeated_edges( | ||
| const std::vector<point::Point> &polygon) { | ||
| auto edges_dedup = calc_dedup_edges({polygon}); | ||
| std::vector<std::vector<point::Point>> result; | ||
| point::Segment segment; | ||
|
|
||
| std::unordered_set edges_set(edges_dedup.begin(), edges_dedup.end()); | ||
| std::unordered_map<point::Point, GraphNode> edges_map; | ||
| for (std::size_t i = 0; i < polygon.size() - 1; i++) { | ||
| segment = {polygon[i], polygon[(i + 1)]}; | ||
| if (edges_set.count(segment) > 0) { | ||
| edges_map[polygon[i]].edges.emplace_back(polygon[i + 1]); | ||
| } | ||
| } | ||
| segment = {polygon.back(), polygon.front()}; | ||
| if (edges_set.count(segment) > 0) { | ||
| edges_map[polygon.back()].edges.emplace_back(polygon.front()); | ||
| } | ||
| for (auto &edge : edges_map) { | ||
| if (edge.second.visited) { | ||
| continue; | ||
| } | ||
| edge.second.visited = true; | ||
| std::vector<point::Point> new_polygon; | ||
| new_polygon.push_back(edge.first); | ||
| auto *current_edge = &edge.second; | ||
| while (current_edge->sub_index < current_edge->edges.size()) { | ||
| auto *prev = current_edge; | ||
| auto next_point = | ||
| current_edge->edges[current_edge->sub_index].opposite_point; | ||
| current_edge = &edges_map.at(next_point); | ||
| prev->sub_index++; | ||
| current_edge->visited = true; | ||
| new_polygon.push_back(next_point); | ||
| } | ||
| while (new_polygon.front() == new_polygon.back()) { | ||
| new_polygon.pop_back(); | ||
| } | ||
| result.push_back(new_polygon); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Guard against potential undefined behavior in map access.
The at() function on line 1358 could throw an out_of_range exception if the point is not found in the map. Consider using find() to safely handle this case.
Apply this diff to fix the issue:
- current_edge = &edges_map.at(next_point);
+ auto it = edges_map.find(next_point);
+ if (it == edges_map.end()) {
+ throw std::runtime_error("Invalid polygon: disconnected edge detected");
+ }
+ current_edge = &it->second;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline std::vector<std::vector<point::Point>> split_polygon_on_repeated_edges( | |
| const std::vector<point::Point> &polygon) { | |
| auto edges_dedup = calc_dedup_edges({polygon}); | |
| std::vector<std::vector<point::Point>> result; | |
| point::Segment segment; | |
| std::unordered_set edges_set(edges_dedup.begin(), edges_dedup.end()); | |
| std::unordered_map<point::Point, GraphNode> edges_map; | |
| for (std::size_t i = 0; i < polygon.size() - 1; i++) { | |
| segment = {polygon[i], polygon[(i + 1)]}; | |
| if (edges_set.count(segment) > 0) { | |
| edges_map[polygon[i]].edges.emplace_back(polygon[i + 1]); | |
| } | |
| } | |
| segment = {polygon.back(), polygon.front()}; | |
| if (edges_set.count(segment) > 0) { | |
| edges_map[polygon.back()].edges.emplace_back(polygon.front()); | |
| } | |
| for (auto &edge : edges_map) { | |
| if (edge.second.visited) { | |
| continue; | |
| } | |
| edge.second.visited = true; | |
| std::vector<point::Point> new_polygon; | |
| new_polygon.push_back(edge.first); | |
| auto *current_edge = &edge.second; | |
| while (current_edge->sub_index < current_edge->edges.size()) { | |
| auto *prev = current_edge; | |
| auto next_point = | |
| current_edge->edges[current_edge->sub_index].opposite_point; | |
| current_edge = &edges_map.at(next_point); | |
| prev->sub_index++; | |
| current_edge->visited = true; | |
| new_polygon.push_back(next_point); | |
| } | |
| while (new_polygon.front() == new_polygon.back()) { | |
| new_polygon.pop_back(); | |
| } | |
| result.push_back(new_polygon); | |
| } | |
| return result; | |
| } | |
| inline std::vector<std::vector<point::Point>> split_polygon_on_repeated_edges( | |
| const std::vector<point::Point> &polygon) { | |
| auto edges_dedup = calc_dedup_edges({polygon}); | |
| std::vector<std::vector<point::Point>> result; | |
| point::Segment segment; | |
| std::unordered_set edges_set(edges_dedup.begin(), edges_dedup.end()); | |
| std::unordered_map<point::Point, GraphNode> edges_map; | |
| for (std::size_t i = 0; i < polygon.size() - 1; i++) { | |
| segment = {polygon[i], polygon[(i + 1)]}; | |
| if (edges_set.count(segment) > 0) { | |
| edges_map[polygon[i]].edges.emplace_back(polygon[i + 1]); | |
| } | |
| } | |
| segment = {polygon.back(), polygon.front()}; | |
| if (edges_set.count(segment) > 0) { | |
| edges_map[polygon.back()].edges.emplace_back(polygon.front()); | |
| } | |
| for (auto &edge : edges_map) { | |
| if (edge.second.visited) { | |
| continue; | |
| } | |
| edge.second.visited = true; | |
| std::vector<point::Point> new_polygon; | |
| new_polygon.push_back(edge.first); | |
| auto *current_edge = &edge.second; | |
| while (current_edge->sub_index < current_edge->edges.size()) { | |
| auto *prev = current_edge; | |
| auto next_point = | |
| current_edge->edges[current_edge->sub_index].opposite_point; | |
| auto it = edges_map.find(next_point); | |
| if (it == edges_map.end()) { | |
| throw std::runtime_error("Invalid polygon: disconnected edge detected"); | |
| } | |
| current_edge = &it->second; | |
| prev->sub_index++; | |
| current_edge->visited = true; | |
| new_polygon.push_back(next_point); | |
| } | |
| while (new_polygon.front() == new_polygon.back()) { | |
| new_polygon.pop_back(); | |
| } | |
| result.push_back(new_polygon); | |
| } | |
| return result; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/PartSegCore_compiled_backend/triangulate.pyx (1)
523-537: Enhance the Python wrapper with validation and documentation.The function would benefit from input validation, better type hints, and a more detailed docstring.
-def split_polygon_on_repeated_edges_py(polygon: Sequence[Sequence[float]]) -> list[list[tuple[float, float]]]: - """ Split polygon on repeated edges""" +def split_polygon_on_repeated_edges_py( + polygon: Sequence[Sequence[float]], +) -> list[list[tuple[float, float]]]: + """Split a polygon into multiple polygons where edges are repeated. + + Parameters + ---------- + polygon : Sequence[Sequence[float]] + Input polygon as a sequence of 2D points (x, y coordinates). + Each point should be a sequence of two float values. + + Returns + ------- + list[list[tuple[float, float]]] + List of polygons, where each polygon is a list of points. + Each point is a tuple of (x, y) coordinates. + + Raises + ------ + ValueError + If the polygon has less than 3 points or contains invalid coordinates. + """ + if len(polygon) < 3: + raise ValueError("Polygon must have at least 3 points") + if not all(len(p) == 2 for p in polygon): + raise ValueError("All points must have exactly 2 coordinates") + cdef vector[Point] polygon_vector cdef vector[vector[Point]] result cdef Point p1, p2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/PartSegCore_compiled_backend/triangulate.pyx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test on macos-latest with Python 3.9
- GitHub Check: Test on windows-latest with Python 3.12
- GitHub Check: Test on windows-latest with Python 3.11
- GitHub Check: Test on windows-latest with Python 3.10
- GitHub Check: Test on windows-latest with Python 3.9
🔇 Additional comments (4)
src/PartSegCore_compiled_backend/triangulate.pyx (4)
97-97: LGTM! Well-defined C++ function declaration.The declaration follows Cython best practices with proper exception handling and GIL release.
431-431: LGTM! Clear function signature update.The new
split_edgesparameter is well-documented and follows the function's naming convention.
498-502:⚠️ Potential issueFix potential off-by-one error in triangle indexing.
The
triangle_countvariable is updated after using it for offsetting triangle indices, which could lead to incorrect indexing. Consider updating it before processing the triangles.for i in range(edge_result.size()): + triangle_count = sum(edge_result[k].centers.size() for k in range(i)) for j in range(edge_result[i].triangles.size()): edge_triangles[edge_triangle_index, 0] = edge_result[i].triangles[j].x + triangle_count edge_triangles[edge_triangle_index, 1] = edge_result[i].triangles[j].y + triangle_count edge_triangles[edge_triangle_index, 2] = edge_result[i].triangles[j].z + triangle_count edge_triangle_index += 1 - triangle_count += edge_result[i].centers.size()Likely invalid or redundant comment.
Line range hint
431-537: Verify test coverage and usage of the new feature.Please ensure that:
- The new edge splitting functionality is covered by unit tests
- The
split_edgesparameter is used appropriately in the codebase✅ Verification successful
Test coverage is comprehensive and properly validates the new functionality
The new edge splitting feature is well-tested with:
- Basic polygon triangulation
- No-split scenarios
- Complex split cases (square in square)
- Explicit verification of split_edges parameter behavior with expected triangle counts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for test coverage of the new functionality echo "Checking test coverage..." rg -l "split_polygon_on_repeated_edges" "src/tests/" # Check for usage of the new parameter echo "Checking usage of split_edges parameter..." rg "triangulate_polygon_with_edge_numpy_li\([^)]*split_edges\s*=\s*True"Length of output: 311
Script:
#!/bin/bash # Check test content echo "Checking test content..." rg -A 10 -B 10 "split_polygon_on_repeated_edges" "src/tests/test_triangulate.py" # Look for potential indirect usage echo "Checking for indirect usage..." rg "triangulate_polygon_with_edge_numpy_li" -A 3 -B 3Length of output: 4104
| if split_edges: | ||
| with cython.nogil: | ||
| edge_split_list = split_polygon_on_repeated_edges(polygon_vector) | ||
| for edge_li in edge_split_list: | ||
| edge_result.push_back(triangulate_path_edge(edge_li, True, 3.0, False)) | ||
| else: | ||
| with cython.nogil: | ||
| edge_result.push_back(triangulate_path_edge(polygon_vector, True, 3.0, False)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for edge splitting.
The edge splitting logic should handle potential error cases, such as empty polygons or invalid splits.
if split_edges:
with cython.nogil:
edge_split_list = split_polygon_on_repeated_edges(polygon_vector)
+ if edge_split_list.empty():
+ # Fall back to original polygon if splitting fails
+ edge_result.push_back(triangulate_path_edge(polygon_vector, True, 3.0, False))
+ else:
for edge_li in edge_split_list:
edge_result.push_back(triangulate_path_edge(edge_li, True, 3.0, False))
else:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if split_edges: | |
| with cython.nogil: | |
| edge_split_list = split_polygon_on_repeated_edges(polygon_vector) | |
| for edge_li in edge_split_list: | |
| edge_result.push_back(triangulate_path_edge(edge_li, True, 3.0, False)) | |
| else: | |
| with cython.nogil: | |
| edge_result.push_back(triangulate_path_edge(polygon_vector, True, 3.0, False)) | |
| if split_edges: | |
| with cython.nogil: | |
| edge_split_list = split_polygon_on_repeated_edges(polygon_vector) | |
| if edge_split_list.empty(): | |
| # Fall back to original polygon if splitting fails | |
| edge_result.push_back(triangulate_path_edge(polygon_vector, True, 3.0, False)) | |
| else: | |
| for edge_li in edge_split_list: | |
| edge_result.push_back(triangulate_path_edge(edge_li, True, 3.0, False)) | |
| else: | |
| with cython.nogil: | |
| edge_result.push_back(triangulate_path_edge(polygon_vector, True, 3.0, False)) |
Summary by CodeRabbit
New Features
Tests