Skip to content

fix(simulation): create_world's ground_plane is checked, not read by truthiness - #3553

Merged
cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/create-world-ground-plane-is-checked-not-read-by-truthiness
Sep 12, 2026
Merged

cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/create-world-ground-plane-is-checked-not-read-by-truthiness

Conversation

@cagataycali

Copy link
Copy Markdown
Member

What

create_world(ground_plane=...) on the MuJoCo and Newton backends is held to boolean_flag_error - the domain the rollout facades in the same class already apply through SimEngine._validate_posture_flags - so a non-boolean is refused by name instead of selecting a floor by truthiness.

Why

The flag selects a posture: lay a floor at z=0 or leave the world open. Both backends validate every knob beside it (timestep and gravity through the shared numeric bindings, terrain and difficulty through their own domains) and read this one by truthiness - MuJoCo compiles the plane if world.ground_plane, Newton calls builder.add_ground_plane() under the same test. Measured on 07ba807 through the MuJoCo facade, reading geom_type off the compiled model:

create_world(ground_plane=) status plane geom compiled
True / False success as asked
"false" "no" "off" "0" 1 nan success yes - the floor the word declines
None 0 0.0 "" [] {} success no - without being a spelling of "no"

Nothing raised and nothing logged on either half. What a caller sees is a scene: a floating-base robot spawned into a world it asked to be open lands on a floor, and a policy evaluated on a world it asked to have a floor falls through the one it did not get - reported by whichever predicate reads the height first, not by the argument that caused it.

Where the check sits

After the terrain / difficulty domain guards and ahead of everything that reads the flag: MuJoCo's world-exists report (which describes the world it cannot rebuild in terms of ground_plane) and the build itself. Not ahead of terrain/difficulty: the flag gates neither of those reads, and tests/simulation/test_create_world_difficulty_domain.py drives both backends' create_world on a stub carrying only the state that runs before its guard - a first draft placed the check first and that suite reported 12 parity failures, which is the suite doing its job. The settled ordering is pinned in both directions (an unusable terrain is still named first; the flag is named ahead of the world-exists report on MuJoCo and ahead of the timestep it would build with on Newton).

Four corners

tests production result
new pre-fix 41 failed / 7 passed (the 7 are the ordering and boolean controls)
new new 48 passed
pre-existing create_world / posture suites (tests/simulation -k "create_world or ground_plane or terrain or difficulty or posture", 760 tests) new 0 failures attributable - identical failing set to main in this env
whole-tree graders (139 in the roster) new 32 failed on branch, 32 on main, byte-identical node ids (all lerobot / serial absent), 0 attributable

The Newton class grades the real create_world on a __new__ engine with _rebuild stubbed, so it runs (and fails pre-fix) without newton/warp installed - the shape tests/simulation/newton/test_create_world_terrain_reject.py already uses.

Gate

ruff check + ruff format --check clean on touched files; mypy no issues on the two backend modules and the test; touched hunks are plain ASCII.

Scope

Production: +2 lines per backend (one if err := ...: return err) plus a comment and a docstring sentence each. Tests: one module. Docs: one sentence in docs/simulation/world-building.md beside the paragraph that already calls ground_plane the master floor switch. SimEngine.create_world's base docstring and overview.md are deliberately untouched - the base contract should state the domain once every backend enforces it, and test_overview_documents_the_rollout_posture_flags.py pins exactly one posture-flag paragraph on that page.

Not touching the Isaac backend, which reads the same flag by truthiness: #3343 (open) rewrites strands_robots/simulation/isaac/simulation.py, so that is a merge-order item for after it lands, alongside the four IsaacConfig flags #3548 records for the same reason. #3343 also edits both backend modules touched here, but none of its hunks fall inside either create_world (read from its diff against main: the nearest MuJoCo hunk starts at line 1635, the nearest Newton hunk ends at line 320 and create_world begins at 339), so the two compose textually clean.

Opened as a draft so the changelog fragment - which needs this number - ships before any approval can be dismissed by it.

…truthiness

The MuJoCo and Newton backends validated every knob beside ground_plane
(timestep, gravity, terrain, difficulty) and read this one by truthiness,
so "false"/"no"/"off"/"0" laid the floor the word declines and None/0/""
omitted it without being a declared spelling - every row status=success.
Bind the shared boolean_flag_error domain through the existing
SimEngine._validate_posture_flags envelope, ahead of the world-exists
report that reads the flag and ahead of the build.

Isaac's create_world reads the same flag and is left for the open Isaac
backend rewrite to absorb (merge-order item).
@cagataycali
cagataycali marked this pull request as ready for review September 12, 2026 14:58
@cagataycali
cagataycali enabled auto-merge (squash) September 12, 2026 14:58

@yinsong1986 yinsong1986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Binds create_world's ground_plane on the MuJoCo and Newton backends to the shared boolean_flag_error domain through the existing SimEngine._validate_posture_flags envelope, so a non-boolean is refused by name with a structured error instead of laying (or omitting) a floor by truthiness. The production change is two if err := ...: return err guards, placed after the terrain/difficulty domain checks (which the flag does not gate) and ahead of everything that reads the flag - MuJoCo's world-exists report and both builds - with the ordering pinned in both directions by the new test module. Verified against the head: _validate_posture_flags and boolean_flag_error(value, param, context) exist at the merge base with matching signatures; every internal caller of create_world passes defaults or literal booleans, and the tool spec already declares the parameter boolean, so no working caller is newly refused; the refused call builds nothing on either backend (pinned). The three commits main gained since the merge base touch neither backend module nor SimEngine, so the composition is clean. The Isaac backend's identical truthiness read is explicitly deferred behind the open rewrite (#3343), which is the right merge-order call.

What's good

  • Follows the AGENTS.md posture-flag convention (checked, not read by truthiness; refusal ahead of the side effect it selects; error dict, never raise) and matches the shape of the sibling fixes that just landed (#3546/#3548/#3549).
  • Guard ordering is justified against the existing difficulty-domain suite and pinned in both directions rather than asserted in prose.
  • Newton class grades the real create_world on a __new__ engine, the same shape as tests/simulation/newton/test_create_world_terrain_reject.py, so it runs (and failed pre-fix) without warp installed.
  • Changelog fragment carries the PR number; added hunks are plain ASCII; the :func: cross-reference targets resolve and stay on one line.

@cagataycali
cagataycali merged commit e865ae7 into strands-labs:main Sep 12, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants