Fix growth inpaint #649
Fix growth inpaint #649
Conversation
bruniss
requested review from
SuperOptimizer and
hendrikschilling
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
This PR fixes growth inpaint functionality and adds two new growth modes: "Inside" (fills holes within the surface boundary) and "FillBounds" (fills all invalid points within the bounding box). The changes include core algorithm updates, a new alpha shape utility for concave hull computation, UI additions for the new modes, and keyboard shortcuts (6 and 7) for quick access.
Key changes:
- Refactored growth direction parsing to support mode-based configuration with mutual exclusivity enforcement
- Implemented alpha shape (concave hull) algorithm to identify interior regions for hole filling
- Added UI checkboxes and keyboard shortcuts for the new growth modes with automatic mutual exclusivity handling
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| GrowPatch.cpp | Refactored growth direction parsing into structured config; added compute_inside_mask function using alpha shapes; implemented three distinct growth modes (directional, inside, fillbounds) with separate candidate generation logic |
| AlphaShape.hpp | New header-only utility implementing alpha shape (concave hull) computation via Delaunay triangulation with edge length filtering |
| SegmentationWidget.hpp | Added checkbox pointers for Inside and FillBounds modes |
| SegmentationWidget.cpp | Implemented UI logic for new checkboxes with mutual exclusivity enforcement; updated mask handling and enable/disable state management |
| SegmentationModule_Input.cpp | Added keyboard shortcuts (Key_6 for Inside, Key_7 for FillBounds); simplified correction point annotation toggle logic (unrelated change) |
| SegmentationGrowth.hpp | Extended SegmentationGrowthDirection enum with Inside and FillBounds; added string conversion support |
| SegmentationGrowth.cpp | Updated parameter building logic to handle new modes; added mutual exclusivity enforcement; missing FillBounds case in directionToString function |
Comments suppressed due to low confidence (2)
volume-cartographer/apps/VC3D/segmentation/SegmentationGrowth.cpp:294
- Missing grow_extra_rows and grow_extra_cols configuration for Inside mode. The code handles FillBounds (lines 287-290) by setting both to 0, and handles other directional modes, but Inside mode falls through to the default case (lines 291-294) which sets both to request.steps. For Inside mode, which only fills holes without expanding outward, these should be set to 0 similar to FillBounds. Add a case for SegmentationGrowthDirection::Inside that sets grow_extra_rows and grow_extra_cols to 0.
if (request.direction == SegmentationGrowthDirection::Left || request.direction == SegmentationGrowthDirection::Right) {
params["grow_extra_cols"] = std::max(0, request.steps);
params["grow_extra_rows"] = 0;
} else if (request.direction == SegmentationGrowthDirection::Up || request.direction == SegmentationGrowthDirection::Down) {
params["grow_extra_rows"] = std::max(0, request.steps);
params["grow_extra_cols"] = 0;
} else if (request.direction == SegmentationGrowthDirection::FillBounds) {
// FillBounds doesn't expand outward, only fills within existing bounds
params["grow_extra_rows"] = 0;
params["grow_extra_cols"] = 0;
} else {
params["grow_extra_rows"] = std::max(0, request.steps);
params["grow_extra_cols"] = std::max(0, request.steps);
}
volume-cartographer/apps/VC3D/segmentation/SegmentationGrowth.cpp:145
- Missing case for SegmentationGrowthDirection::FillBounds in directionToString function. While the default case will handle it by returning "all", this is semantically incorrect and inconsistent with the similar function segmentationGrowthDirectionToString in SegmentationGrowth.hpp which does handle FillBounds explicitly. Add a case for FillBounds that returns "fillbounds" to maintain consistency and correctness.
QString directionToString(SegmentationGrowthDirection direction)
{
switch (direction) {
case SegmentationGrowthDirection::Up:
return QStringLiteral("up");
case SegmentationGrowthDirection::Down:
return QStringLiteral("down");
case SegmentationGrowthDirection::Left:
return QStringLiteral("left");
case SegmentationGrowthDirection::Right:
return QStringLiteral("right");
case SegmentationGrowthDirection::Inside:
return QStringLiteral("inside");
case SegmentationGrowthDirection::All:
default:
return QStringLiteral("all");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing edge case handling for insufficient valid points. If valid_points has fewer than 3 points, computeAlphaShapeMask returns an all-zeros mask (as seen in AlphaShape.hpp line 20-22), which is correct. However, this means inside_mode would have no candidates to grow into, potentially causing confusion. Consider adding a check after line 614 to log a warning or info message when valid_points.size() < 3, informing the user that inside mode requires at least 3 valid points to compute the concave hull.
| if (valid_points.size() < 3) { | |
| std::cerr << "[GrowPatch] compute_inside_mask: inside mode requires at least 3 " | |
| << "valid points to compute a concave hull; inside mask will be empty." | |
| << std::endl; | |
| } |
| // Skip triangles with vertices outside bounds | ||
| if (!bounds_rect.contains(cv::Point(static_cast<int>(p0.x), static_cast<int>(p0.y))) || | ||
| !bounds_rect.contains(cv::Point(static_cast<int>(p1.x), static_cast<int>(p1.y))) || | ||
| !bounds_rect.contains(cv::Point(static_cast<int>(p2.x), static_cast<int>(p2.y)))) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The bounds check on line 44-46 has an off-by-one error. cv::Rect::contains() returns true for points strictly inside the rectangle, meaning it returns true for x in [rect.x, rect.x + rect.width) and y in [rect.y, rect.y + rect.height). However, the bounds_rect is initialized with cv::Rect(0, 0, mask_size.width, mask_size.height), which means valid coordinates are [0, width) and [0, height). When casting float coordinates to int, a point at exactly (width-0.1, height-0.1) would cast to (width-1, height-1) which is valid, but a point at exactly (width, height) would be outside. This is technically correct, but the real issue is that points inserted into Subdiv2D can generate triangulation vertices slightly outside the input point set. Consider using a small epsilon when checking bounds, or ensure the bounds_rect is slightly larger than needed to avoid edge cases where valid triangulation vertices fall just outside due to floating-point precision.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
volume-cartographer/apps/VC3D/segmentation/SegmentationGrowth.cpp:145
- Missing case for SegmentationGrowthDirection::FillBounds in directionToString function. This function should handle FillBounds similar to how it handles Inside. Without this case, FillBounds will fall through to the default case and return "all" which is incorrect and inconsistent with the enum-to-string conversion in SegmentationGrowth.hpp.
QString directionToString(SegmentationGrowthDirection direction)
{
switch (direction) {
case SegmentationGrowthDirection::Up:
return QStringLiteral("up");
case SegmentationGrowthDirection::Down:
return QStringLiteral("down");
case SegmentationGrowthDirection::Left:
return QStringLiteral("left");
case SegmentationGrowthDirection::Right:
return QStringLiteral("right");
case SegmentationGrowthDirection::Inside:
return QStringLiteral("inside");
case SegmentationGrowthDirection::All:
default:
return QStringLiteral("all");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Compute a mask of "inside" points: points that are NOT valid but are | ||
| // enclosed by the valid region's concave hull (alpha shape). | ||
| static cv::Mat compute_inside_mask( | ||
| const cv::Mat_<uint8_t>& state_mat, |
There was a problem hiding this comment.
Edge case not fully handled: When valid_points.size() < 3, a warning message is printed but the function continues to call computeAlphaShapeMask with fewer than 3 points. The computeAlphaShapeMask function returns an empty mask for < 3 points (line 20-22 of AlphaShape.hpp), but it would be clearer and more efficient to return early from compute_inside_mask when this condition is detected.
| mask = kGrowDirAllMask; | ||
| } | ||
|
|
||
| setGrowthDirectionMask(mask); |
There was a problem hiding this comment.
Critical bug: setGrowthDirectionMask calls normalizeGrowthDirectionMask which strips away Inside and FillBounds bits. When mask is set to kGrowDirInsideBit or kGrowDirFillBoundsBit earlier in this function, calling setGrowthDirectionMask will cause normalizeGrowthDirectionMask to perform mask &= kGrowDirAllMask. Since kGrowDirAllMask only contains the 4 directional bits (line 53), the Inside and FillBounds bits will be masked away and replaced with kGrowDirAllMask. This breaks the new Inside and FillBounds functionality. The normalizeGrowthDirectionMask function needs to be updated to preserve Inside and FillBounds bits, or setGrowthDirectionMask should not call normalize when these special modes are set.
also add inside growth only mode and fill bounding box growth mode + ui support