Repository navigation
Add a camera-renderer setting to lab_sim - #970
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Summary
Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Human Review Check | |
PASS. The PR adds renderer selection for simulation cameras, related launch logic, tests, and simulation configuration. The changes are limited to lab_sim, april_tag_sim, and `hand_eye_calibration… |
| Description check | |
The pull request description directly matches the changes. It explains the renderer modes, Nyx integration, camera-topic behavior, configuration updates, and reported testing. |
Full details: Human Review Check
-
Explanation
PASS. The PR adds renderer selection for simulation cameras, related launch logic, tests, and simulation configuration. The changes are limited to
lab_sim,april_tag_sim, andhand_eye_calibration_sim. They do not modify auth, permissions, secrets, CI/CD pipelines, deployment or production hosting configuration, behavior packages, SDKs, or files underbin/scripts. The 342 added lines form one focused simulation feature, not a large cross-cutting change.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
Comment @coderabbitai help to get the list of available commands.
6fd0cc1 to
5818f52
Compare
|
|
1 similar comment
|
5818f52 to
c756c89
Compare
|
1 similar comment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
|
|
|
[written by AI]
Running lab_sim in
Unlike Either extend the setting to |
|
This PR is proof of compatibility with user-installed Nyx feeds - not fine tuning of assets/lighting 
|
nbbrooks
left a comment
nbbrooks
left a comment
There was a problem hiding this comment.
See note above about
MOVEIT_PRO_CAMERA_RENDERER covers scene_camera only, and wrist_camera stays on MuJoCo in every mode
| <!-- Whether to launch the MuJoCo interactive viewer --> | ||
| <param name="mujoco_viewer">$(arg mujoco_viewer)</param> | ||
| </hardware> | ||
| <!-- nyx: the Nyx node publishes scene_camera/color and camera_info, so MuJoCo publishes only depth and points. --> |
There was a problem hiding this comment.
MOVEIT_PRO_CAMERA_RENDERER covers scene_camera only, and wrist_camera stays on MuJoCo in every mode. Should apply to both.
42973c6 to
a53f35e
Compare
|
![picknik-baymax-bot[bot]](https://avatars.githubusercontent.com/in/4231697?s=60&v=4){kind=small}
There was a problem hiding this comment.
[written by AI]
Reviewed at 375dc44. CI is green and wrist_camera is now covered. Thanks.
Requesting changes because of what this PR is for. moveit_pro#23927 documents this PR as the way to wire MOVEIT_PRO_CAMERA_RENDERER and the Nyx launch into a robot config. #1075 then copies the pattern to every MuJoCo config. So the bar is "a pattern users copy into their own configs", not "works for lab_sim".
The force-push from 4363050 to 375dc44 dropped several pieces that pattern needs. The earlier version had all of them, and #1075 still carries some:
- An empty value fails the description. Details inline.
moveit_pro runaccepts empty, and the launch file treats empty asopengl. - The value is evaluated, not compared. Details inline. Both the xacro
dict[...]lookup and the launch$(eval ...)paste the raw variable into a Python expression.4363050bound it to a property first and explained why. - The camera list is hard-coded twice. A config with other cameras has to copy and edit two
<sensor>blocks per mode, plus one<include>per camera.4363050had anyx_camerasarg and acamera_renderer_sensorsmacro. #23927 links readers to that macro (See the camera_renderer argument and the camera_renderer_sensors macro in lab_sim/description/picknik_ur.xacro), and the macro doesn't exist at this head. #1075 builds on it too (hand_eye_calibration_simsetsnyx_camerasfor its three cameras). - Pinned through
urdf_paramsvs. "set it in the environment". This head pinscamera_rendererinurdf_paramsfor april_tag_sim and hand_eye_calibration_sim. #1075's description says: "Set it in the environment, noturdf_params: the launch file reads only the environment." The two PRs need one rule before #23927 documents it. Pinning is right for configs that start no Nyx nodes, but the xacro comment should say that pins are only for configs that start no Nyx nodes. Otherwise a user config that inherits lab_sim's launch file and pins the renderer ends up with the description in one mode and the launch file in another. - Lost safeguards.
test_camera_renderer.pyand its CMake entry were removed. That test checked the modes, the camera list, and the invalid value, and it would have caught item 1.- The
MOVEIT_PRO_CAMERA_RENDERER=openglpins on the lab_sim and april_tag_sim integration-test environments were removed. Without them, a developer who exportsbothand runscolcon testsees the wrist-camera Objectives fail. - The
.nyx_scenes/entry in.gitignorewas removed. #23927 states "The example workspace's.gitignoreexcludes.nyx_scenes/", and exports can be hundreds of MB inside$USER_WS.
- Missing-scene handling.
4363050'snyx_cameras.launch.pyfailed at launch when the Nyx package was missing, whenUSER_WSwas unset, or when the exported scene directory didn't exist. Each message said what to do, because MuJoCo color is already off innyxmode and those cases would otherwise leave the cameras with no color. At this head,find-pkg-share picknik_nyx_renderergives a bare launch error, and a missing scene is reported only by the node after it starts. Either approach is fine, but whichever is canonical should fail loudly and say what to do.
Suggested path: restore the 4363050 design (arg + macro + guard, the launch checks, the test, the env pins, .gitignore) on top of the wrist_camera and pin changes. Then rebase #1075 onto it. #1075 is stacked on the older 80592ed commit and will need the rebase anyway. After that, recheck #23927's lab_sim references against the merged result.
Not blocking, but please state it where users will see it: in both, nothing publishes wrist_camera/color. Ten lab_sim Objectives read that topic (take_picture_with_wrist_camera, pick_all_pill_bottles, ml_segment_point_cloud*, select_region_*, addbottlestoplanningscene, …), so both is for comparison only. #23927 says this; the xacro comment and this PR's description should too.
Verified: xacro on a reduced copy of the selector (unset/opengl/nyx/both OK; empty → error: ''; a quoted value is evaluated); moveit_pro main launcher validation, nyx_camera.launch.py and camera_node.py. No ROS launch was run.
| name="nyx_namespace" | ||
| value="$(eval '"nyx" if "$(var camera_renderer)" == "both" else ""')" | ||
| /> | ||
| <include |
There was a problem hiding this comment.
[written by AI]
In nyx mode, MuJoCo color is already disabled. When the Nyx package is missing, USER_WS is unset, or .nyx_scenes/lab_sim was never exported, the result is a bare find-pkg-share error or a node that exits after it starts. Either way the cameras have no color. 4363050's nyx_cameras.launch.py checked all three before starting and said how to fix each one. For the canonical example, I'd restore those checks or something like them.
|
Consider whether the change should land upstream in Overlapping files
|
|
|
[written by AI] Items 1 and 2 are real and are fixed in 2542cba, which also restores the Item 3, a proposalPut the arg, the validation and the macro in <xacro:include filename="$(find picknik_accessories)/descriptions/sensors/camera_renderer.urdf.xacro" />
<xacro:arg name="nyx_cameras" default="scene_camera wrist_camera" />
...
<xacro:camera_renderer_sensors cameras="$(arg nyx_cameras)" />That package is already an Two changes from your version while moving it. The renderer value is bound to a property before it is compared, which is also what closes item 2: in Verified on the full Pushed as c21e61f so the shape is visible rather than described. Happy to revert it and put the macro back in Item 4The pins in Item 6This could be broken out into a separate PR together with the launch file's two One note on the rationale: the sentence quoted from moveit_pro#23927 pointing at the |
|
There was a problem hiding this comment.
[written by AI]
Re-reviewed at c21e61f (2542cba + c21e61f since my last review). Approving: the description side is now a pattern worth copying, and CI is green (lab_sim 34, hangar_sim 19).
Checked by running xacro 2.1.1 (the Jazzy release) on camera_renderer.urdf.xacro from this head with a two-camera caller:
- unset, empty,
opengl→ no<sensor>blocks;nyx→color_enabledfalse for both;both→opengl/topics for both. bogus,Nyx,nyx,nyx'] or True or [',nyx' or 'x,${"nyx"},$(env HOME)→ all fail with exit 2 and the message naming the value. Binding to a property closed the injection, guard included.- A
camera_renderer:=openglpin overridesMOVEIT_PRO_CAMERA_RENDERER=nyx;nyx_cameras:="a b c"emits three blocks.
Status of my earlier items
- Empty value: fixed.
- Evaluated value: fixed in the xacro. The two
$(eval)lets inruntime.launch.xmlstill paste the value in, but any value they'd misread now fails the description first, so deferring that is fine. - Shared macro: fixed, and
picknik_accessoriesis a better home thanlab_sim, since every MuJoCo config already depends on it. - Pins: the reasoning in the comments and the YAML (no description of their own, no Nyx nodes, remove in #1075) settles it.
.gitignoreand the integration-test pins are restored.test_camera_renderer.pyis not. With the macro now shared by every config in #1075, a small test inpicknik_accessorieswould protect all of them; fine as a follow-up.- Launch-time checks: OK as a separate PR, along with the Python launch file.
On the #23927 quote: it is in that PR's diff, not the description. In the new tutorial page: "See the camera_renderer argument and the camera_renderer_sensors macro in lab_sim/description/picknik_ur.xacro". Since the macro has moved to picknik_accessories/descriptions/sensors/camera_renderer.urdf.xacro, that line now needs to change in #23927. I'll flag it there.
Before #23927 merges
- #23927 points to
lab_sim/launch/runtime.launch.xmlas the launch example. If the follow-up for item 6 replaces that file with a Python launch file, land the follow-up first or recheck the link. - #1075 is stacked on
80592edand carries its own copy of the macro. Rebase it onto this head and change it to include the shared file. - The overlap bot flagged
.gitignore. Since #23927 tells users to add.nyx_scenes/to their own workspace, adding it tomoveit_pro_empty_wstoo means new workspaces get it automatically.
Two small inline notes; neither blocks.
| <robot xmlns:xacro="http://ros.org/wiki/xacro"> | ||
| <!-- Which renderer produces a MuJoCo config's camera color topics. Set it in the environment: the | ||
| config's runtime.launch.xml reads MOVEIT_PRO_CAMERA_RENDERER directly to start one Nyx node per | ||
| camera, and cannot see urdf_params. An empty value means opengl, and an unrecognized one fails |
There was a problem hiding this comment.
[written by AI]
Nit: "Set it in the environment … cannot see urdf_params" is the right rule for configs that start Nyx nodes. But april_tag_sim and hand_eye_calibration_sim pin camera_renderer in urdf_params deliberately, and someone copying this file will read only this comment. One sentence would settle it: a config that starts no Nyx nodes pins camera_renderer: opengl in urdf_params, so the environment can't disable its cameras' color.
| selected. Call it inside <ros2_control>. opengl emits nothing, since MuJoCo's defaults already | ||
| publish <camera>/color. --> | ||
| <xacro:macro name="camera_renderer_sensors" params="cameras"> | ||
| <xacro:if value="${len(cameras.split()) > 0}"> |
There was a problem hiding this comment.
[written by AI]
Nit: the list is split on whitespace only, so nyx_cameras: "a,b" emits a single <sensor name="a,b"> without any error. Since this is now the shared entry point, either state "space-separated, no commas" in the nyx_cameras comment in lab_sim or fail on a comma. Also worth knowing: an empty nyx_cameras:= falls back to the arg default rather than emitting nothing, so a config can't use it to opt all cameras out. Probably fine; the pin covers that.
The base branch was changed.
MOVEIT_PRO_CAMERA_RENDERER picks opengl (the default), nyx, or both. The robot description places scene_camera's MuJoCo topics from it, and runtime.launch.xml reads the same variable to decide whether to start picknik_nyx_renderer and where it publishes, so the two halves cannot disagree. nyx: the camera's <sensor> block sets color_enabled=false, so MuJoCo publishes only depth and points and the Nyx node publishes scene_camera/color and camera_info. both: MuJoCo's color and camera_info move to scene_camera/opengl/ and Nyx publishes on scene_camera/nyx/, leaving scene_camera/color empty so no consumer reads one renderer while expecting the other. Depth and points stay where they are. An unrecognized value fails the description rather than leaving the config with no camera. Needs the Nyx renderer package and the color_enabled param, which are not in a released image yet, so anything other than the default requires a moveit_pro build that carries them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019YCk2yNsEQkVUSvhAmsJ4m
MOVEIT_PRO_CAMERA_RENDERER covered scene_camera only, so wrist_camera stayed on MuJoCo in every mode. The nyx and both overrides now name both cameras, and runtime.launch.xml starts a Nyx node for each, since the node renders one camera per instance. The renderer comment drops to one line, since Pro documents the values, and the launch file binds the two expressions once rather than repeating them per camera. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both configs use lab_sim's robot description but ship their own runtime launch file with no Nyx nodes, so MOVEIT_PRO_CAMERA_RENDERER disabled MuJoCo's color for their cameras with nothing to replace it. Pinning camera_renderer in urdf_params overrides the environment default and keeps them on MuJoCo. hand_eye_calibration_sim is the one that mattered: its scene has no scene_camera, so the setting was inert there until wrist_camera joined it, and wrist_camera is the camera it calibrates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An empty MOVEIT_PRO_CAMERA_RENDERER failed the robot description. moveit_pro's validator accepts an empty value and docker-compose forwards it as-is, so a bare assignment in the workspace .env took the description down. The value is now bound to a property and read as data, with an empty value treated as opengl, which also keeps a quote in the value from closing the string literal and being evaluated instead of compared. The lab_sim and april_tag_sim integration tests pin the renderer to opengl so colcon test does not depend on a developer's exported value, and .nyx_scenes/ is ignored, where nyx_export_scene writes hundreds of megabytes inside $USER_WS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MOVEIT_PRO_CAMERA_RENDERER applies to every MuJoCo config, so the arg, the validation and the per-camera <sensor> blocks move into picknik_accessories, which the configs already depend on, and lab_sim includes them and passes its camera list. A config with other cameras sets nyx_cameras in urdf_params or calls the macro with its own list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c21e61f to
fbca8a4
Compare
|
nbbrooks
left a comment
nbbrooks
left a comment
There was a problem hiding this comment.
lgtm, will file followup items for nit feedback
[written by AI]
Refs PickNikRobotics/moveit_pro#20960
Motivation
PickNikRobotics/moveit_pro#22738 added an optional path-traced camera renderer that no configuration starts. This lets
lab_simstart it.Brief description
MOVEIT_PRO_CAMERA_RENDERERselects which renderer produces the color topics oflab_sim's RGB cameras,scene_cameraandwrist_camera:<camera>/color,camera_info<camera>/depth,points<camera>/opengl/*<camera>/nyx/*opengl(default)nyxbothopenglis the only option on images without the Nyx runtime, which is every image except amd64 NVIDIA.picknik_ur.xacroadds a<sensor>block per camera fornyx(color_enabledfalse) and forboth(rgb_topicandcamera_info_topicunderopengl/).runtime.launch.xmlstarts one Nyx node per camera, since the node renders a single camera per instance, and setstopic_namespace(empty fornyx,nyxforboth).bothleaves<camera>/colorempty so no consumer reads one renderer while expecting the other. Depth and points publish in every mode. Ten oflab_sim's Objectives readwrist_camera/color, sonyxandbothchange what they see.An unrecognized value fails the description instead of leaving the config without a camera.
april_tag_simandhand_eye_calibration_simreuse this description with their own runtime launch files and no Nyx nodes, so both pincamera_renderer: "opengl"inurdf_params. Without the pin, setting the variable for the deployment disables their cameras' color with nothing to replace it, andwrist_camerais the camerahand_eye_calibration_simcalibrates.Release notes
lab_simcan renderscene_cameraandwrist_camerawith the Nyx path-traced renderer by settingMOVEIT_PRO_CAMERA_RENDERERtonyxorboth.How it was tested
pre-commit runon the changed files.opengl,nyx,both, unset, andbogus: the first three emit the intended<sensor>block for both cameras with unchanged publish rates, unset behaves asopengl, andbogusfails naming the value.ros2 launchof the launch file's<let>bindings in a Pro image:nyx_enabledis False for unset andopengland True fornyxandboth, andnyx_namespaceis empty fornyxandnyxforboth.lab_simunderros2_control_nodewith a moveit_pro#22738 build ofpicknik_mujoco_ros, probing everyscene_cameratopic in each mode: MuJoCo's publishers match the table.ros2 launchofruntime.launch.xmlpasses--topic-namespace=to the Nyx node innyxmode and--topic-namespace=nyxinboth.Claude agent checks
code-reviewerdocumentation-botlicensing-privacy-botplatform-architect-botroboticist-botfrontend-noah-botsecurity-auditorcompatibility-botMOVEIT_PRO_CAMERA_RENDERERunset, xacro andros2 launchwere run in an image withoutpicknik_nyx_rendererand behave as beforesonar-bottest-runner🤖 Generated with Claude Code