feat(mujoco): implement off-screen rendering and generic actuator ctrl#43
feat(mujoco): implement off-screen rendering and generic actuator ctrl#43shubhambaid wants to merge 3 commits intodora-rs:mainfrom
Conversation
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
If we have 4 key changes, how about splitting this PR to 4 PRs?
| print( | ||
| f"Model loaded: nq={self.m.nq} nv={self.m.nv} " | ||
| f"nu={self.m.nu} ncam={self.m.ncam}", | ||
| flush=True, | ||
| ) |
There was a problem hiding this comment.
We remove this debug print when this is ready, right?
There was a problem hiding this comment.
Yes! I left those in during my local testing to ensure the dynamic camera discovery was actually working. I have removed the prints and the warning, so the node will now run silently if no cameras are present.
| def _cam_topic(cam_name: str) -> str: | ||
| """Derive a Dora-safe output topic name from a camera's XML name. | ||
| self.node = Node(config["name"]) | ||
| Hyphens are replaced with underscores so the name is valid as a YAML | ||
| key without quoting. No other transformation is applied, keeping the | ||
| mapping transparent: "camera-wrist-right" → "camera_wrist_right". | ||
| """ | ||
| return cam_name.replace("-", "_") |
There was a problem hiding this comment.
Do we need this?
I think that camera-wrist-right is also a valid YAML key. It doesn't need quoting. For example, this is a valid YAML:
camera-wrist-right: valueThere was a problem hiding this comment.
I originally converted them to underscores because some older data serialization libraries occasionally complain about hyphens in dictionary keys. But since standard YAML handles it perfectly fine and it matches the XML exactly, I completely agree. I've removed the conversion logic!
| #Image constants (must match the downstream pipeline expectations) | ||
| _IMG_W: int = 960 | ||
| _IMG_H: int = 600 | ||
| _JPEG_QUALITY: int = 90 |
There was a problem hiding this comment.
Can we make them cusotmizable by command line arguments and/or environment variables?
| if not self._cameras: | ||
| print("WARNING: no cameras found in model — no image output", flush=True) |
There was a problem hiding this comment.
Can we make image output optional (no warning)?
| parser = argparse.ArgumentParser(description="Generic MuJoCo simulation node") | ||
| parser.add_argument("--name", type=str, default="mujoco") | ||
| parser.add_argument("--scene", type=str, help="Path to the MuJoCo XML scene file") |
There was a problem hiding this comment.
Why did you change them instead of using the current code?
There was a problem hiding this comment.
Since the node no longer explicitly requires the --config JSON to map joint names (as it dynamically uses data.ctrl), I had cleaned up the arguments to reflect only what it currently uses. However, I want to avoid unnecessary diff noise, so I have restored your original argparse boilerplate and just appended the new image arguments.
| continue | ||
|
|
||
| scene = os.getenv("SCENE", args.scene) | ||
| eid = event["id"] |
There was a problem hiding this comment.
Why did you rename event_id to eid? I feel that event_id is more readable than eid.
There was a problem hiding this comment.
I abbreviated it to eid simply to keep the line lengths a bit shorter inside the nested event loop.
| elif eid == "action": | ||
| self._handle_action(event["value"]) | ||
|
|
||
| def _handle_action(self, value: pa.Array | pa.ChunkedArray) -> None: |
There was a problem hiding this comment.
I think that event["value"] must be pa.Array.
There was a problem hiding this comment.
I added those explicitly to catch silent NumPy broadcasting errors just in case a mismatched array slipped through from an upstream node. However, since dora-rs already strictly enforces these schemas at the routing level, doing it again here is definitely redundant. I've stripped the checks out to keep the fast path clean.
| if not isinstance(value, (pa.Array, pa.ChunkedArray)): | ||
| print( | ||
| f"WARNING: action expected pa.Array, got {type(value).__name__} — ignored", | ||
| flush=True, | ||
| ) | ||
| return |
There was a problem hiding this comment.
Do we need this check? I think that value must be pa.Array.
Since the physics and camera changes are tightly coupled to make the simulation drop-in ready for our inference graph, I'd love to keep this in a single PR, but I will gladly strip out all the unnecessary formatting and address your inline feedback to make it much easier to review. But if you feel, this isn't the right way to do it, i can split the PR. |
- Restored original argparse boilerplate and argument to minimize diff noise - Added CLI arguments for , , and - Removed hyphen-to-underscore conversion for camera topic names - Removed redundant pa.Array type checking and length validation - Removed debug prints, warnings, and verbose comments
2 participants
This PR upgrades the mujoco-client from a passive viewer to a fully functional, generic simulation node capable of handling both vision and physics-based control.
Key Changes
Off-Screen Rendering: Replaced the deprecated mujoco.Renderer with mujoco.GLContext and mujoco.MjrContext. The node now dynamically discovers all elements in the loaded XML and publishes high-fidelity JPEG arrays for each.
Physics-Accurate Actuation: Switched the action input handler to write directly to data.ctrl rather than teleporting joints via data.qpos. This ensures that all actuator limits, gear ratios, and damping parameters defined in the user's MJCF are properly respected by the physics engine.
Decoupled Event Loops: Separated the high-frequency physics step (tick) from the lower-frequency vision step (render). This prevents high-res image encoding from bottlenecking the control loop.
Dynamic Configuration: Removed legacy JSON configuration and hardcoded joint assumptions. The node now fully configures its inputs and outputs dynamically based purely on the provided --scene XML.