fix(sim): load_scene keeps an exported robot registered, and names the robots/objects/cameras it dropped - #3802
Conversation
…swap dropped, and how to put them back
load_scene replaces self._world with a fresh SimWorld: every registered robot,
object and camera is gone and the loaded file is the whole scene. The result
said only
Scene loaded from table.xml
Bodies: 3, Joints: 1, Actuators: 0
Through the agent tool the caller is Robot("so101", mode="sim") - a facade
named after the very arm this drops - and the loss surfaced two calls later
as "No robots registered in the simulation. Add a robot first". The wrist
camera and any add_object bodies went the same way, silently. The recovery
exists (add_robot mutates the loaded spec in place, so the arm goes back INTO
the loaded scene) but nothing said so.
The success result now names what the swap discarded and the verb that puts
each group back, when anything was registered:
REPLACED the live world: dropped robot(s) ['so101'], camera(s) ['wrist'] -
the loaded file is now the whole scene (robot-scoped actions refuse with
'No robots registered' until then). add_robot(name='so101',
data_config='so101') puts the arm back INTO the loaded scene; add_camera
re-adds cameras.
The add_robot call is spelled with the data_config the robot was registered
under (a placeholder when unknown). A json block carries dropped_robots /
dropped_objects / dropped_cameras. The seeded free camera "default" is not
reported as dropped: nobody added it and the loaded world renders from it
too. A fresh world with nothing registered keeps the historical text.
tests/simulation/mujoco/test_load_scene_names_what_it_dropped.py (5): dropped
robot named with its add_robot call; object and camera named, "default"
excluded; fresh world unchanged; the named recovery works (arm back,
table body still present); placeholder when data_config is unknown.
tests/simulation/mujoco scene/load/world/camera/object selection passes;
mypy clean.
… refused attach reports its reason
export_xml -> load_scene is the documented round trip, and with a robot in the
world it broke the robot: the file carries the arm's bodies and actuators
(Bodies: 9, Actuators: 6), but load_scene swapped in a fresh SimWorld, so the
arm was in the scene and unknown to the registry. get_robot_state refused
with "No robots registered in the simulation", and add_robot(name='so101')
- the recovery the previous commit spells - collided in MuJoCo on every mesh
name ("repeated name 'so101/base_motor_holder_so101_v1' in mesh") and reached
the caller as the bare "Failed to inject robot 'so101' into scene." with the
reason left in the log.
load_scene now carries every registered robot whose namespaced joints are ALL
present in the loaded model (robot_subtree_in_model): the SimRobot object
moves to the new world, robot_base_xml is kept, and joint / actuator ids are
re-resolved against the new model through rediscover_robot_ids - the loop
_recompile_preserving_state already ran, factored out so both paths share it.
The result says
Robot(s) ['so101'] found in the loaded file (same namespaced joints) and
kept registered - robot-scoped actions keep working; do NOT add_robot them
again (the names would collide).
and the json block gains carried_robots beside dropped_robots / objects /
cameras. A robot the file does not contain is dropped and named as before.
Measured after the round trip: list_robots shows so101, get_robot_state,
run_policy and render all succeed, add_robot under the same name is refused as
"already exists".
inject_robot_into_scene re-raises the attach failure after restoring the
pre-attach spec, matching the recompile-refusal path right below it and the
object path, so add_robot reports "Failed to load: <MuJoCo's reason>". When
the reason is a repeated name under this robot's namespace - a file exported
with the robot, loaded where nothing was registered - a hint names it: the
subtree is already in the scene; if it came from load_scene of such a file it
is already registered, otherwise pick a different name.
tests: test_load_scene_names_what_it_dropped.py +4 (round trip keeps the
robot registered with 6 joint / 6 actuator ids and a working
get_robot_state; add_robot under the same name refused as already exists;
exported file loaded into a FRESH sim then add_robot names the collision with
the hint; a scene without the robot still drops it);
test_scene_ops_guardrails: the unreadable-URDF inject now asserts the raised
reason and an intact spec instead of a bare False. tests/simulation/mujoco
scene/load/world/robot/inject/attach/export selection 1,505 pass; isaac
load_scene 10 pass; mypy clean.
…_scene swapped in The re-point was unpinned: replacing it with `pass` left all 53 cells green. It is load-bearing - `_attach_robot_to_mesh` sets `SimRobot._world` so the child Mesh's `_read_state` can read joint positions, so a carried robot left pointing at the world `load_scene` discarded makes its peer publish state from a dead model. An off-mesh robot keeps None, the documented value. Rewrites the conditional expression as a guarded assignment naming that reason, and adds the two cells that grade both directions.
…t it dropped A dropped object the loaded file still carries was told to add_object itself, which MuJoCo refuses with "repeated name 'cube' in body" - the same dead end this change removes for the robot. It is now reported as untracked-but-present. The "No robots registered" warning is also only given when a robot was really dropped: with the robot carried, the envelope contradicted its own first line. Uses the mj_name_to_id wrapper from .backend for the presence probe, per test_entity_name_lookup_type_safety, so a non-string name cannot crash.
|
Independent re-verification on Thor (Jetson AGX Thor, Round trip re-measured on today's main (
|
| call | before e12fe59d7 |
after 584f68f75 |
|---|---|---|
load_scene(exported.xml) |
Bodies: 8, Joints: 6, Actuators: 6 — nothing else |
+ Robot(s) ['so101'] found in the loaded file (same namespaced joints) and kept registered |
list_robots() |
[] |
['so101'] |
get_robot_state('so101') |
error — No robots in the scene; add one |
success — 6 joints, pos=0.0000, vel=0.0000 |
add_robot('so101') |
error — bare Failed to inject robot 'so101' into scene. |
error — Robot 'so101' already exists. Pick a different name, or omit name= to auto-number. |
run_policy(robot_name='so101', policy_provider='mock') |
error | success |
render() |
success | success |
run_policy is the row I would add to the body's table: before this branch the arm the file still contains is not merely unlisted, a rollout on it is refused outright.
The contract change I went looking for
inject_robot_into_scene changing return False -> raise is the one hunk a reviewer should worry about. Audited rather than assumed:
- exactly one production caller,
simulation.py:2547, and an AST walk puts it insidetry(2489-2669) withexcept Exceptionat 2654 — which is the handler that produces the new hinted message and doesself._world.robots.pop(name, None). - the early-guard
return Falsepaths in the helper survive, so theif not ok:branch at the call site is still live, not dead code. - no other production reference (
grepoverstrands_robots/); 6 test modules reference the helper and all pass.
Gates, measured here
| check | result |
|---|---|
pre-fix control (the 2 production files reverted to upstream/main, the test file's one helper-importing cell dropped so it still collects) |
8 failed / 44 passed — incl. the bare Failed to inject robot 'so101' into scene. and DID NOT RAISE ValueError |
test_load_scene_names_what_it_dropped.py + test_scene_ops_guardrails.py on this head |
57 passed |
tests/simulation/mujoco/ |
4612 passed, 2 failed |
| the 2 failures | test_stop_policy_resolves_the_only_rollout.py — reproduced with my files reverted to pure main (2 failed / 6 passed), so pre-existing and not this branch's; owned by #3798, which is APPROVED |
ruff check / format --check |
clean, 2133 files |
| mypy | 28 errors in 12 files = my recorded clean-main baseline (examples/isaac_gs owns 18), 0 in the four touched files |
| merge onto current main | conflict-free |
No objection from here — behaviour, blast radius and the raise contract all check out. Blocked only on REVIEW_REQUIRED; auto-merge (squash) is already armed.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
load_scene swaps in a fresh SimWorld, silently discarding every registered robot, object and camera; the export_xml -> load_scene round trip was a dead end (arm present in the model but unregistered, add_robot refused with the reason swallowed). This PR carries over any registered robot whose namespaced joints all resolve in the loaded model (ids re-resolved via the extracted rediscover_robot_ids, mesh back-reference re-pointed at the new world under self._lock), names everything actually dropped with the exact recovery call, distinguishes untracked-but-present objects/cameras from absent ones, and lets MuJoCo's attach-refusal reason travel instead of folding into a bare False.
What's good
- The attach-failure contract change (
return False->raiseafter spec rollback) has exactly one production caller, which converts the exception into a structured error dict with registry cleanup - consistent with the error-handling contract in AGENTS.md. - All carried-robot state mutations (
robot._worldre-point, registry insert, id rediscovery) happen inside the existingself._lockcritical section; the world swap stays atomic. - Fresh-world output is byte-identical to the historical text, and the
{"json": ...}content block follows the established result pattern (physics.py, rendering.py) - purely additive. - Tests pin every reviewed behavior, including the mutation-tested
_worldre-point, the off-meshNoneback-reference, the seeded"default"camera exclusion, and the round-trip recovery paths; guardrail test updated to the new raise contract. - Plain-ASCII user-facing strings, no host paths, changelog entry included.
Pull request was closed
load_scenereplaces the live world with a freshSimWorld, so every registered robot, object and camera was discarded — and the result said onlyScene loaded from table.xml / Bodies: 3. Through the tool the caller isRobot("so101", mode="sim"), a facade named after the very arm this drops, and the loss surfaced two calls later asNo robots registered in the simulation.The render is the model
load_scenejust loaded: the arm is standing right there, 6 joints and 6 actuators — andlist_robots()returned[].export_xml→load_scenewas a dead end, not just silentMeasured live on Thor, same call sequence in a worktree at
main(730d194) and on this branch:load_scene(exported.xml)Bodies: 8, Joints: 6, Actuators: 6Robot(s) ['so101'] found in the loaded file … kept registeredlist_robots()[]['so101']get_robot_state('so101')No robots in the scene; add one'so101' state (t=0.000s): 6 jointsadd_robot('so101')Failed to inject robot 'so101' into scene.— reason (repeated name 'so101/base_motor_holder…' in mesh) left in the logRobot 'so101' already exists.The arm was in the file, unregistered, and the one call that could recover it was refused with its reason swallowed. Now a robot whose namespaced joints all resolve in the loaded model is carried over (ids re-resolved, mesh back-reference re-pointed at the new world); anything actually discarded is named with the call that puts it back:
plus a json block of
carried_robots/dropped_*/still_in_loaded_file. A fresh world keeps the historical text byte-for-byte; the seeded free camera"default"is not reported as dropped.Two defects found while verifying, fixed here
Probing the round trip with an object registered turned up dead-end advice of the same shape this PR removes for the robot:
add_object re-adds objects→Failed to inject 'cube': repeated name 'cube' in bodyThe loaded file already carries object(s) ['cube'] under the same name: … re-adding them is refused ('repeated name') - use a different namekept registered - robot-scoped actions keep workingandrobot-scoped actions refuse with 'No robots registered'— self-contradictory in one envelopeThe presence probe goes through
mj_name_to_idfrom.backend, notmj.mj_name2id—test_entity_name_lookup_type_safetycaught the direct call, so a non-string name cannot crash the process.Verification
_world; re-point unconditionally; always adviseadd_object; always warnNo robots registeredtests/simulation/mujocomain, empty two-waycommagainst a clean worktree, already owned by #3798format --checkmainbaseline, 0 in the four touched filessimulation/base.py+isaac/recording.py, which it already conflicts with onmainLOC: +487 / −15. Additions are the carry-over path plus its pins;
load_scenepreviously had no test for what its world swap discards, and the_worldre-point was unpinned (apassmutant left all 53 cells green).The
_worldre-point matters beyond tidiness:_attach_robot_to_meshsetsSimRobot._worldso the child Mesh's_read_statecan read joint positions, so a carried robot left pointing at the discarded world would publish state from a dead model. An off-mesh robot keepsNone, the documented value.Found by the Thor harness (lab PC2-075), verified headless with
MUJOCO_GL=egl.