feat: add planning group Viser panel#2563
Conversation
TomCC7
requested review from
leshy,
mustafab0,
paul-nechifor and
spomichter
as code owners
Greptile SummaryThis PR layers the full Viser planning-group UI on top of the core planning-group API from #2489, replacing the deleted
Confidence Score: 3/5Safe for single-robot setups; multi-robot plans bypass the joint-staleness gate before execution, which could allow executing a stale plan after a robot has moved. The new panel_backend.py, scene.py additions, and 73-test suite are well-structured. Two issues in state.py and gui.py affect multi-robot correctness and thread safety respectively. state.py (can_execute multi-group branch) and gui.py (_set_operation_error / _build_joint_slider_handles) need attention before use in a multi-robot context. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant U as User (GUI thread)
participant GUI as ViserPanelGui
participant TEW as TargetEvaluationWorker
participant OW as OperationWorker
participant BE as panel_backend
U->>GUI: drag transform control
GUI->>GUI: _on_transform_update(group_id, handle)
GUI->>TEW: submit(TargetEvaluationRequest)
TEW->>BE: evaluate_pose_target_set(pose_targets, seed)
BE-->>TEW: TargetSetEvaluation
TEW->>GUI: _apply_target_evaluation_result(request, result)
GUI->>GUI: update feasibility / target_status / refresh()
U->>GUI: click Plan
GUI->>GUI: _submit_plan() / _next_operation_id()
GUI->>OW: submit(operation)
OW->>BE: plan_to_joint_targets(targets)
BE-->>OW: ok
OW->>GUI: "_finish_operation(plan=True)"
GUI->>GUI: "plan_state.status = FRESH"
U->>GUI: click Execute
GUI->>GUI: can_execute() check
GUI->>OW: submit(execute_operation)
OW->>BE: manipulation_module.execute()
BE-->>OW: ok
OW->>GUI: "_finish_operation(execute=True)"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant U as User (GUI thread)
participant GUI as ViserPanelGui
participant TEW as TargetEvaluationWorker
participant OW as OperationWorker
participant BE as panel_backend
U->>GUI: drag transform control
GUI->>GUI: _on_transform_update(group_id, handle)
GUI->>TEW: submit(TargetEvaluationRequest)
TEW->>BE: evaluate_pose_target_set(pose_targets, seed)
BE-->>TEW: TargetSetEvaluation
TEW->>GUI: _apply_target_evaluation_result(request, result)
GUI->>GUI: update feasibility / target_status / refresh()
U->>GUI: click Plan
GUI->>GUI: _submit_plan() / _next_operation_id()
GUI->>OW: submit(operation)
OW->>BE: plan_to_joint_targets(targets)
BE-->>OW: ok
OW->>GUI: "_finish_operation(plan=True)"
GUI->>GUI: "plan_state.status = FRESH"
U->>GUI: click Execute
GUI->>GUI: can_execute() check
GUI->>OW: submit(execute_operation)
OW->>BE: manipulation_module.execute()
BE-->>OW: ok
OW->>GUI: "_finish_operation(execute=True)"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Multi-group freshness is checked by group snapshot when available. The | ||
| # robot-level current-joint fallback preserves one-group legacy tests. | ||
| if len(plan.start_joints_snapshot) == 1: | ||
| snapshot = next(iter(plan.start_joints_snapshot.values())) | ||
| if len(snapshot) != len(self.current_joints): | ||
| return False | ||
| return all( | ||
| abs(expected - current) <= current_tolerance | ||
| for expected, current in zip(snapshot, self.current_joints, strict=False) | ||
| ) | ||
| ) | ||
| return True |
There was a problem hiding this comment.
When plan.start_joints_snapshot has more than one entry (any multi-robot plan), the method skips the staleness guard entirely and returns True. A user can plan, wait for a robot to drift outside current_match_tolerance, and still have the Execute button enabled. The comment claims "Multi-group freshness is checked by group snapshot when available," but no such check exists in the else branch — it unconditionally returns True. For a single-group plan the tolerance gate works correctly; only multi-group plans bypass it.
| joint_limits_lower = config.joint_limits_lower if config is not None else None | ||
| joint_limits_upper = config.joint_limits_upper if config is not None else None | ||
| for index, (global_name, local_name) in enumerate( | ||
| zip(group.joint_names, group.local_joint_names, strict=False) | ||
| ): | ||
| joint_name = str(global_name) | ||
| local = str(local_name) | ||
| value = float( | ||
| target_by_name.get( | ||
| joint_name, | ||
| target_by_name.get( | ||
| local, current_by_name.get(joint_name, current_by_name.get(local, 0.0)) | ||
| ), | ||
| ) | ||
| ) | ||
| lower, upper = DEFAULT_JOINT_LIMITS | ||
| if joint_limits_lower is not None and index < len(joint_limits_lower): | ||
| lower = joint_limits_lower[index] | ||
| if joint_limits_upper is not None and index < len(joint_limits_upper): | ||
| upper = joint_limits_upper[index] |
There was a problem hiding this comment.
index here is the ordinal position of a joint within group.joint_names, not within the robot's full joint list. If a planning group covers only a subset of the robot's DOF (e.g., a wrist-only group whose joints are indices 4–6 of a 7-DOF arm), joint_limits_lower[index] will retrieve the limit for joints 0–2 rather than 4–6. The slider limits would then be silently wrong for such groups. Is it guaranteed that config.joint_limits_lower/upper is always indexed identically to group.joint_names? Is config.joint_limits_lower/upper always indexed identically to group.joint_names (i.e. the group always covers a prefix or the full robot joint list in the same order), or can a group reference an arbitrary subset of robot joints?
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## cc/spec/movegroup #2563 +/- ##
=====================================================
- Coverage 70.93% 69.84% -1.10%
=====================================================
Files 883 894 +11
Lines 79958 82622 +2664
Branches 7242 7566 +324
=====================================================
+ Hits 56722 57710 +988
- Misses 21361 22951 +1590
- Partials 1875 1961 +86
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 48 files with indirect coverage changes 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
ready-to-merge
1 participant
Summary
Stack
cc/spec/movegroup) keeps core planning-group APIs plus the thin visualization ABI compatibility layer.Verification
uv run pytest dimos/manipulation/visualization/viser/test_viser_visualization.py dimos/manipulation/visualization/viser/test_visualizer_lifecycle.py dimos/manipulation/visualization/viser/test_operation_worker.py dimos/manipulation/visualization/viser/test_gui_status.py dimos/manipulation/visualization/viser/test_state.py -quv run --group lint mypyuv run ruff check dimos/manipulation/visualization/viseruv run ruff format --check dimos/manipulation/visualization/viserrtk git diff --check