-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix SurfaceGripper.update #3894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…eturns list[str], not list[int])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Fixed type mismatch bug in SurfaceGripper.update() where GripperView.get_surface_gripper_status() returns list[str] but was incorrectly typed as list[int] and directly converted to tensor, causing ValueError: too many dimensions 'str'.
Key changes:
- Corrected type annotation from
list[int]tolist[str]on surface_gripper.py:178 - Added explicit string-to-float mapping dictionary (
{"Open": -1.0, "Closing": 0.0, "Closed": 1.0}) - Implemented proper conversion using
map()before tensor creation - Updated documentation comments to remove incorrect state-to-int mappings
Impact:
- Resolves runtime crash in
scripts/demos/pick_and_place.pyand other code usingSurfaceGripper - Maintains same numerical output values (-1.0, 0.0, 1.0) for neural network compatibility
Confidence Score: 4/5
- Safe to merge with minor consideration for error handling
- Fix correctly addresses the root cause (type mismatch) and maintains expected behavior. One minor improvement suggested: add error handling for unexpected state values to prevent potential KeyError exceptions
- No files require special attention - single file change is well-contained
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/assets/surface_gripper/surface_gripper.py | 4/5 | Fixed type annotation and string-to-float conversion logic for gripper state; potential KeyError if unexpected state value returned |
Sequence Diagram
sequenceDiagram
participant ENV as Environment/Scene
participant SG as SurfaceGripper
participant GV as GripperView (Isaac Sim)
participant TORCH as PyTorch Tensor
Note over ENV,TORCH: Every simulation step
ENV->>SG: update(dt)
SG->>GV: get_surface_gripper_status()
GV-->>SG: list[str] ["Open", "Closing", "Closed"]
Note over SG: OLD: tried to convert list[str] to tensor directly (ValueError)
Note over SG: NEW: map strings to floats using dictionary
SG->>SG: map("Open" -> -1.0, "Closing" -> 0.0, "Closed" -> 1.0)
SG->>TORCH: tensor(state_values, dtype=float32)
TORCH-->>SG: gripper_state tensor
Note over SG: State now usable for NN/RL
1 file reviewed, 1 comment
source/isaaclab/isaaclab/assets/surface_gripper/surface_gripper.py
Outdated
Show resolved
Hide resolved
…r.py Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Simon Fetzel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Fixes runtime error in SurfaceGripper.update() where GripperView.get_surface_gripper_status() returns list[str] but code incorrectly tried to convert strings directly to tensor and subtract 1.0.
Key changes:
- Corrected type annotation from
list[int]tolist[str]on line 178 - Added explicit string-to-float mapping dictionary for the three gripper states
- Used
map()withmapping.get(s, 0.0)to convert state strings to numeric values - Updated docstring comments to remove misleading numeric mappings (lines 165-167)
Analysis:
The fix correctly handles the API contract where Isaac Sim's GripperView returns string states ("Open", "Closing", "Closed") rather than integers. The use of .get() with a default value of 0.0 provides fallback handling if an unexpected state is returned, which maps to "Closing" behavior. This is a reasonable default that won't trigger immediate gripper actions.
Confidence Score: 4/5
- Safe to merge - fixes critical runtime bug with correct type handling
- The fix directly addresses the root cause of the ValueError by properly converting string states to floats. The mapping is consistent with documented behavior and the fallback default is sensible. Minor deduction for using silent default (0.0) instead of raising an error for unexpected states, though this is acceptable for robustness.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/assets/surface_gripper/surface_gripper.py | 4/5 | Fixes type error by converting string states from get_surface_gripper_status() to floats using explicit mapping instead of incorrect arithmetic. Uses .get() with default 0.0 for unknown states. |
Sequence Diagram
sequenceDiagram
participant Demo as pick_and_place.py
participant Scene as InteractiveScene
participant SG as SurfaceGripper
participant GV as GripperView (Isaac Sim)
Demo->>Scene: scene.update(dt)
Scene->>SG: surface_gripper.update(dt)
SG->>GV: get_surface_gripper_status()
GV-->>SG: ["Open", "Closing", "Closed"]
Note over SG: OLD: torch.tensor(state_list) - 1.0<br/>❌ Fails with "too many dimensions 'str'"
Note over SG: NEW: Map strings to floats<br/>{"Open": -1.0, "Closing": 0.0, "Closed": 1.0}
SG->>SG: state_values = [map states to floats]
SG->>SG: _gripper_state = torch.tensor(state_values)
SG-->>Scene: ✓ Update complete
Scene-->>Demo: ✓ Scene updated
1 file reviewed, no comments
Description
Example
scripts/demos/pick_and_place.pyfails with error:Fixes issue #3893: GripperView.get_surface_gripper_status() returns list[str], not list[int]
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there. I guess that's not necessary.