Add region-coverage-path demo for GetRegionFromUser (wrist camera) - #819
Add region-coverage-path demo for GetRegionFromUser (wrist camera)#819L4co77 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds the “Select Region and Cover” BehaviorTree objective, region-to-mask and surface-coverage behavior nodes, package and plugin registration, unit tests, and an integration-test skip for the unavailable UI server. ChangesSelect Region and Cover
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (3 passed)
Comment |
|
0a9c6b8 to
498816c
Compare
|
498816c to
9f5f87a
Compare
|
9f5f87a to
a878e83
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lab_sim_behaviors/src/get_mask2d_from_region.cpp (1)
78-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
cv::fillConvexPolysilently mis-rasterizes non-convex input.The function assumes a convex polygon (per the header doc) but doesn't validate it. Since
GetRegionFromUseris currently constrained to box-drawing, risk is low today, but as a public utility this could silently produce a wrong mask (rather than the documented explicit failure modes) if ever fed a concave/self-intersecting region.Consider using
cv::fillPoly(handles arbitrary simple polygons) instead ofcv::fillConvexPoly, or explicitly validate convexity and return an error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lab_sim_behaviors/src/get_mask2d_from_region.cpp` around lines 78 - 87, Replace the cv::fillConvexPoly call in the mask construction flow with cv::fillPoly so arbitrary simple polygons are rasterized correctly without relying on an unstated convexity assumption. Preserve the existing polygon coordinates, mask dimensions, and fill value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lab_sim_behaviors/src/get_mask2d_from_region.cpp`:
- Around line 78-87: Replace the cv::fillConvexPoly call in the mask
construction flow with cv::fillPoly so arbitrary simple polygons are rasterized
correctly without relying on an unstated convexity assumption. Preserve the
existing polygon coordinates, mask dimensions, and fill value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 071c0df6-0959-4941-a36e-cb7fa48b389c
📒 Files selected for processing (13)
src/lab_sim/objectives/select_region_and_cover.xmlsrc/lab_sim/test/objectives_integration_test.pysrc/lab_sim_behaviors/CMakeLists.txtsrc/lab_sim_behaviors/include/lab_sim_behaviors/generate_surface_coverage_path.hppsrc/lab_sim_behaviors/include/lab_sim_behaviors/get_mask2d_from_region.hppsrc/lab_sim_behaviors/package.xmlsrc/lab_sim_behaviors/src/generate_surface_coverage_path.cppsrc/lab_sim_behaviors/src/get_mask2d_from_region.cppsrc/lab_sim_behaviors/src/register_behaviors.cppsrc/lab_sim_behaviors/test/CMakeLists.txtsrc/lab_sim_behaviors/test/test_behavior_plugins.cppsrc/lab_sim_behaviors/test/test_generate_surface_coverage_path.cppsrc/lab_sim_behaviors/test/test_get_mask2d_from_region.cpp
a878e83 to
f51bf35
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lab_sim_behaviors/src/generate_surface_coverage_path.cpp`:
- Around line 85-102: The coverage path count calculations in
generateSurfaceCoveragePath must use ceil-based counts so non-divisible spacing
reaches both region edges; clamp the final X and Y coordinates to the positive
bounds, and apply kMaxCoveragePoses to these revised counts. In
src/lab_sim_behaviors/test/test_generate_surface_coverage_path.cpp lines 77-94,
add a non-divisible-spacing test verifying both edges are reached and adjacent
samples never exceed the requested spacing.
- Around line 48-86: Extend validation in generate_surface_coverage_path to
require all three region_dimensions values to be finite and positive before any
calculations or narrowing casts. Also reject negative params.margin and
params.standoff while preserving the existing finite-parameter checks; allow
zero for these clearance inputs.
In `@src/lab_sim_behaviors/src/get_mask2d_from_region.cpp`:
- Around line 61-86: Validate every point’s x and y coordinates before the
min/max bounding-box loop in GetMask2DFromRegion. Reject any non-finite value or
value outside the normalized [0,1] range before calling to_px_x/to_px_y,
returning the function’s established unexpected-error result; only compute the
bounding box and rasterization polygon after all vertices pass validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a99da853-4a8b-4de0-8ad6-8ec59ceae597
📒 Files selected for processing (13)
src/lab_sim/objectives/select_region_and_cover.xmlsrc/lab_sim/test/objectives_integration_test.pysrc/lab_sim_behaviors/CMakeLists.txtsrc/lab_sim_behaviors/include/lab_sim_behaviors/generate_surface_coverage_path.hppsrc/lab_sim_behaviors/include/lab_sim_behaviors/get_mask2d_from_region.hppsrc/lab_sim_behaviors/package.xmlsrc/lab_sim_behaviors/src/generate_surface_coverage_path.cppsrc/lab_sim_behaviors/src/get_mask2d_from_region.cppsrc/lab_sim_behaviors/src/register_behaviors.cppsrc/lab_sim_behaviors/test/CMakeLists.txtsrc/lab_sim_behaviors/test/test_behavior_plugins.cppsrc/lab_sim_behaviors/test/test_generate_surface_coverage_path.cppsrc/lab_sim_behaviors/test/test_get_mask2d_from_region.cpp
|
f51bf35 to
131591f
Compare
|
|
I want to see green grid lines of the coverage path trajectory in the video: An example of what I'm talking about is in the product tour here on the Cold Spray slide: This graphic visualization will help users understand what is going on. |
|
I know this is the wrong PR, but I feel like the mouse icon should change when we're in rectangle selection mode, maybe into like target sites or just a hand-click icon: 
Overall, thanks for making this cool demo! |
|
2d-2026-08-05_13.15.31.mp4 |
|
[written by AI] Added the green coverage-path visualization you asked for, @davetcoleman. Verified live end-to-end in |
|
It's hard to see what's happening in the video once the robot starts moving, can you change the bottom right feed to be scene camera color? From what I can tell, the green grid line seems to be under the table for some reason. Do you know why it's doing that? At the end of the scan and plan objective, can you have the robot move back to the home position to restart it? |
|
Add VisualizePath after GenerateSurfaceCoveragePath so the operator sees the serpentine sweep before it is planned and executed. Lower PlanCartesianPath blending_radius to 0.001 so the near-duplicate waypoints the raster emits at row turns do not trip the '2 * blending_radius' check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
87d56ad to
eacc791
Compare
|
Did you mean to close this? The video looks much better! However, if you're up to the challenge of making this even better, can you select the corners of the burner stove top pad, and have it trace along that surface much closer? Right now, the bottles are in the way of the surface and feel kind of weird and arbitrary. Let's pretend it's spraying down or wiping down the flat surface of the burner in this demonstration. |
|
Claude broke this PR - a force-push from a shallow clone rewrote the branch history, so GitHub auto-closed it and it can't be reopened. Continuing the same work in a new PR: #835 |
2 participants
region-2026-07-30_17.28.03.mp4
Demo requested by @davetcoleman on PR #20768 (GetRegionFromUser): rather than an orphan feature, an objective in lab sim that uses the wrist camera to let the user draw a bounding box and then runs a coverage sweep over that region.
What this adds
select_region_and_cover.xml— the user draws a box on the wrist camera image; the box is lifted to 3D and swept with a serpentine coverage path, then planned and executed with approval.GetMask2DFromRegion(lab_sim_behaviors) — rasterizes thePolygonStampedregion fromGetRegionFromUserinto aMask2D.GetRegionFromUserreturns normalized [0..1] image coordinates, so this behavior takescamera_infoand denormalizes them to pixels before filling the mask, so it aligns with the point-cloud projection inGetMasks3DFromMasks2D.GenerateSurfaceCoveragePath(lab_sim_behaviors) — pure geometry: a boustrophedon (serpentine) raster of tool poses over the top face of the fitted oriented box, at a configurable standoff/line/point spacing. NamedSurface...deliberately — core MoveIt Pro already ships aGenerateCoveragePath(a corner+area lawnmower); this one is OBB-driven with a standoff and tool-into-surface orientation.Pipeline
SwitchUIPrimaryView→SwitchController(activate JTAC) →GetRegionFromUser→GetPointCloud/GetCameraInfo→GetMask2DFromRegion→GetMasks3DFromMasks2D→GetPointCloudFromMask3D→GetOrientedBoundingBoxFromPointCloud→GenerateSurfaceCoveragePath→PlanCartesianPath→WaitForJointTrajectoryApproval→ExecuteTrajectory.The 2D→3D chain mirrors the shipped
ml_auto_grasp_object_from_clicked_point.xml, swapping the SAM click for the drawn region.GetMask2DFromRegionandGenerateSurfaceCoveragePathare the only new behaviors; everything else already exists. The oriented box'sreference_poseis an identity pose in the wrist-camera frame (CreatePoseStamped), which disambiguates the OBB orientation against the camera axes.Approach / why
The geometry is extracted into pure free functions (
regionToMask2D,generateRasterPath) and unit-tested in isolation — thetick()methods are thin I/O wrappers. This keeps the coverage math deterministic and testable without a running stack.regionToMask2Dusescv::fillConvexPoly, valid becauseGetRegionFromUserreturns a convex box.Testing
test_behavior_plugins.cppextended so pluginlib load is verified for both new behaviors.skip_objectivesinobjectives_integration_test.py(it needs an interactive UI prompt, likeMarker Visualization Example/Teleoperate).Live verification
Run end-to-end in
lab_sim(mock hardware) against a #20768-built backend: draw a box on the wrist camera → the full pipeline recovers the 3D region, fits an OBB, generates the coverage path, andPlanCartesianPathplans it into an approvable ~1300-point trajectory thatExecuteTrajectoryruns. Live testing surfaced and fixed four issues the objective-skipping CI could not: the reference-pose behavior name, the normalized-coordinate denormalization above,point_spacingvsblending_radius, and activating the admittance controller before execution.Limitations (demo, not production)
GenerateSurfaceCoveragePathassumes the fitted box's +Z is the outward surface normal. A PCA-fit OBB does not guarantee axis assignment or sign; the camera-framereference_poseis the only disambiguator today.line_spacing/point_spacing/standoff) are tuned forlab_sim.