fix(sim): strip robot-asset ground plane on attach to stop floor z-fighting

[cagataycali]

What

Fixes the broken floor render (flickering checkerboard/triangle mess) that appears when a robot whose asset scene ships its own ground plane is added to a world with ground_plane=True.

Root cause

Two coplanar infinite ground planes at z=0 -> depth-buffer z-fighting:

• ground -- added by SpecBuilder.build (world's ground_plane=True)
• arm/floor -- from the robot's own menagerie scene (e.g. franka_emika_panda/scene.xml ships <geom name="floor" type="plane"/>)

Fix

SpecBuilder.attach_robot now deletes any PLANE geom from the robot spec before attaching, leaving exactly one world-owned ground plane.

Verification

• Before: 2 planes at z=0, floor renders as a flickering mess.
• After: exactly 1 ground plane, floor renders cleanly (verified via offscreen render of a Panda + cube scene).
• New regression test tests/simulation/mujoco/test_spec_builder.py::test_attach_robot_strips_robot_scene_ground_plane.
• Full tests/simulation/mujoco/ suite unaffected (the one pre-existing so101 action-controller failure is unrelated and reproduces on clean main).
• ruff check + ruff format --check clean.

Discovered while rendering a Cosmos 3 policy rollout (#317).

Closes #320

---

Review changelog

Round	Concern	Fix commit	Pin test
R1	Doc-drift: inline comment referenced #319 (Qwen-VLA) instead of #320 (ground-plane issue)	668b8f4	N/A (comment-only)
R2	Doc-drift: test docstring referenced #319 instead of #320 (same class as R1, symmetric fix)	395dbf1	tests/simulation/mujoco/test_spec_builder.py::test_attach_robot_strips_robot_scene_ground_plane
R3	Doc-semantics: attach_robot docstring did not surface the silent mjGEOM_PLANE strip (per AGENTS.md PR #86 "Match docstrings to semantics")	d29c71a	N/A (doc-only)

Deferred to v0.4.1 (reviewer-confirmed non-blocking)

• Conditional strip when world.ground_plane=False (robot whose scene ships the only floor would silently lose it)
• Tighten plane filter from type-only (mjGEOM_PLANE) to name-allowlist or pose-check
• Expanded regression test matrix: ground_plane=False, multi-plane scenes, non-floor planes
• CHANGELOG one-liner under v0.4.0 notes flagging the silent-strip behavior for users feeding custom MJCF with deliberate non-floor <geom type="plane">

[yinsong1986]

Summary

Fixes the visible z-fighting between the world-owned ground plane and a robot scene's own floor (e.g. franka_emika_panda/scene.xml ships <geom name="floor" type="plane"/>) by stripping every PLANE geom from the robot spec before scene_spec.attach(...) in SpecBuilder.attach_robot. Diff is small (+12 prod / +36 test), localised to one internal staticmethod, no public API surface touched, no persisted-schema or wire-format implications. The accompanying regression test compiles a minimal robot MJCF with a floor plane against a ground_plane=True world and asserts exactly one ground plane survives.

What's good

• Scope is tight and matches the PR title; no drive-by edits.
• The inline code comment explains why (z-fighting on coplanar infinite planes) before what, which matches the AGENTS.md preference for documenting non-obvious fixes.
• Regression test pins the fix and would fail on pre-fix code (would compile to two planes) — AGENTS.md PR #86 "Pin every reviewed fix with a regression test" satisfied.
• No public API change, no __all__ mutation, no env var or default flip — not a one-way door, safe to ship in v0.4.0 / Summit.

Must fix before merge

(none — PR is ready to merge once any follow-ups are tracked)

Follow-up in v0.4.1

• Unconditional strip ignores world.ground_plane=False (spec_builder.py:424). When a user opts out of the world ground (SimWorld(ground_plane=False)) and attaches a robot whose scene ships its own floor, this PR silently strips that floor too — the robot ends up over an empty world with no ground. Real config: there are call sites today (tests/simulation/mujoco/test_simulation.py:127, test_spec_builder.py:195) that exercise ground_plane=False. Fix is straightforward (gate the strip on a flag plumbed in from world.ground_plane, or only strip when the world's ground plane will exist). Not blocking because the reported regression is the more common case; track as a v0.4.1 issue.
• Filter strips all planes, not only z=0 floors (spec_builder.py:424). g.type == mjGEOM_PLANE removes walls, dividers, and any geometric plane the robot scene uses for non-floor purposes. Today's menagerie scenes use planes only as floors so the bug isn't observable, but tightening to a name allowlist ({floor, ground}) or a pos[2] ≈ 0 + axis-aligned check would prevent silent removal of future non-floor planes. Track as a v0.4.1 hardening issue.
• Issue-number drift in the inline comment (spec_builder.py:423). Comment says "See #319." but the commit message and PR description say "Closes #320". One of the two refs is wrong; align before this comment ossifies as documentation.
• Test coverage gap on the new behaviour (tests/simulation/mujoco/test_spec_builder.py:380). Only the ground_plane=True happy path is covered. Worth adding: (a) ground_plane=False + robot-with-floor (pins whatever decision is made on the gating point above), (b) robot scene with multiple planes (the for _g in _plane_geoms loop is untested with len > 1), (c) plane at non-zero z (would tighten the filter discussion above). AGENTS.md PR #86 > "Pin every reviewed fix with a regression test" — test debt, not blocking.

Verification suggestions

hatch run test -- tests/simulation/mujoco/test_spec_builder.py -k ground_plane -v
# Manual smoke (needs the [sim-mujoco] extra and a menagerie checkout):
python -c "from strands_robots import Robot; r = Robot('panda', mode='sim'); r.simulation.render('/tmp/panda.png')"
# Inspect /tmp/panda.png — floor should render as a clean grid, no flicker/triangles.

[yinsong1986]

[FOLLOW-UP] This strips every robot-scene plane unconditionally, even when the world doesn't own a ground plane (SimWorld(ground_plane=False)). In that configuration a robot whose menagerie scene ships its own floor — which is the only ground in the resulting scene — silently loses it, and the robot ends up over an empty world. There are call sites today exercising ground_plane=False (tests/simulation/mujoco/test_simulation.py:127, test_spec_builder.py:195), so the regression is reachable.

Suggest gating the strip on the world flag, e.g. plumb world.ground_plane into attach_robot and only strip when True, or guard on scene_spec already containing a geom named ground. Not a merge blocker — the reported z-fighting is the common case and the misconfiguration is narrow — but worth tracking as a v0.4.1 issue.

[yinsong1986]

[FOLLOW-UP] The filter is g.type == mjGEOM_PLANE, which matches any plane, not just floor planes at z=0. Today's menagerie scenes only use planes as floors so the bug isn't observable, but a robot MJCF that uses a plane as a wall, divider, or virtual sensor surface would have it silently removed too — and the inline comment promises "ground/floor PLANE geom", which is narrower than what the code does.

Tighten to either a name allowlist (g.name in {"floor", "ground"}) or a position/orientation check (pos[2] ≈ 0 and z-axis aligned). Follow-up because nothing in robot_descriptions ships a non-floor plane today; fixing it now just prevents a future silent-strip surprise.

[yinsong1986]

[FOLLOW-UP] Single happy-path assertion. To pin the fix's full surface (per AGENTS.md PR #86 > "Pin every reviewed fix with a regression test"), worth adding:

1. ground_plane=False + robot-with-floor — locks in whatever decision is made on whether the strip should be conditional (see the related comment on spec_builder.py:424).
2. Robot scene with multiple planes — the for _g in _plane_geoms loop currently runs with len == 1 only.
3. Plane at non-zero z (e.g. a wall) — would either pass (current code strips it, which the suggested filter tightening would change) or fail, making the contract explicit.

Not blocking; track as v0.4.1 test debt.

[yinsong1986]

Summary

Small, focused fix: SpecBuilder.attach_robot now deletes any mjGEOM_PLANE geom from the robot spec before attaching, eliminating the coplanar-z=0 z-fighting that appeared when a robot scene (e.g. franka_emika_panda/scene.xml) shipped its own floor plane on top of the world's ground plane. +12/-0 in spec_builder.py, +36/-0 regression test. Behavior matches the description.

What's good

• Root-cause analysis in the PR description is correct and well-documented; the inline rationale comment in spec_builder.py:416-423 will save the next reader an afternoon.
• Regression test pins the contract structurally ("exactly one plane named ground survives compile") rather than asserting on internal state.
• Scope discipline: behavioral concerns surfaced in round 1 (conditional strip on ground_plane=False, name-allowlist tightening, expanded matrix coverage) are explicitly deferred to v0.4.1 rather than scope-creeping this PR. The single round-1 fold (#319 → #320 doc-drift in spec_builder.py:423) was the right call.
• AGENTS.md compliance: no host paths, no emojis in user-facing strings, no public-API surface changes, no new env vars.

Must fix before merge

(none — PR is ready to merge once the v0.4.1 follow-ups are tracked as issues)

Follow-up in v0.4.1

• Stale issue ref in the new test docstring (tests/simulation/mujoco/test_spec_builder.py:381). The test references (#319) — the same drift the round-1 fold corrected in spec_builder.py. Should be (#320). Pure doc-fix, anchors the next reader on the right tracked issue.
• Conditional strip when world.ground_plane=False (already flagged R1). A robot whose own scene ships the only floor (e.g. someone using a robot scene MJCF directly without world ground) silently loses its floor. Track as if not world.ground_plane: skip the strip.
• Tighten plane filter from type-only to name-allowlist or pose check (already flagged R1). g.type == mjGEOM_PLANE strips walls/dividers/non-floor planes too. A {floor, ground} name allowlist or a z≈0 pose check is a tighter contract. No robot_descriptions asset hits this today, but it's a latent footgun.
• Expand regression matrix (already flagged R1): ground_plane=False, multi-plane scenes, non-floor planes (walls). Pin the contract once the conditional-strip decision lands.
• Renderer-level regression test (gap, not a R1 thread). The new test asserts model.ngeom-based structure but the user-visible bug was a rendering artifact (z-fighting on the depth buffer). A future integ test that does an offscreen render of the Panda scene and asserts against a reference image / pixel-diff would catch the next variant of this class of bug. Low priority — structural assertion is a reasonable proxy.

Verification suggestions

• hatch run test tests/simulation/mujoco/test_spec_builder.py::test_attach_robot_strips_robot_scene_ground_plane to confirm the new test passes.
• Manual offscreen-render spot-check (optional, matches the PR's reproduction): load a Panda + world with ground_plane=True, render once, confirm the floor is uniform (no flicker/triangle pattern). The PR description claims this was verified — worth a 30-second reproduction before merging.
• Standard CI (hatch run lint && hatch run test) is otherwise sufficient; no new deps, no public-API changes, no wire-format touches.

[yinsong1986]

[FOLLOW-UP] Two related v0.4.1 concerns ride on this filter — both already flagged in round 1, leaving an anchor here so the v0.4.1 issue has a code-pointer:

1. world.ground_plane=False case: this strips unconditionally, so a robot whose scene ships the only floor silently loses it. Conditional strip (if scene_has_world_ground: ...) closes the gap.
2. Type-only filter is too broad: a robot using a <geom type="plane"> as a wall/divider gets silently stripped too. Name allowlist ({floor, ground}) or a pose check (z≈0, normal≈+z) is the tighter contract.

Neither blocks v0.4.0 — no robot_descriptions asset in the registry today hits case (2), and case (1) requires the ground_plane=False flag which has thin coverage. Track as v0.4.1 issues with a regression test added when the decision lands.

[yinsong1986]

Summary

Small, well-scoped fix: SpecBuilder.attach_robot now deletes any mjGEOM_PLANE geom from the robot spec before attaching, eliminating the depth-buffer Z-fight between the world's ground plane and a robot scene's floor plane (e.g. franka_emika_panda's scene.xml). Comes with a focused regression test that compiles the resulting model and asserts exactly one plane named ground survives. Diff is +48/-0, no public API or wire-format changes.

What's good

• Root cause is correctly identified and the in-source comment captures it well (#320 referenced, behaviour rationale explicit).
• Regression test is end-to-end (compiles the spec, walks model.geom_type, checks the surviving plane's name) — this is exactly the "Pin regression tests for reviewed fixes" pattern from AGENTS.md.
• Mutation pattern collects geoms into a list before deleting, so we're not iterating-and-mutating.
• Scope discipline: the diff touches only the two relevant files; no drive-by edits.
• The two reviewer-confirmed deferrals (conditional-strip when world.ground_plane=False, tighter plane filter, expanded matrix) are correctly classified as v0.4.1 — none of them are one-way doors and none change persisted state or public APIs.

Must fix before merge

(none — PR is ready to merge once the docstring follow-up is tracked as a v0.4.1 issue)

Follow-up in v0.4.1

• spec_builder.py:392-411 — attach_robot's docstring still describes the function as a thin wrapper over spec.attach(...) and does not mention that it now silently mutates the loaded robot_spec by deleting all PLANE geoms before attach. Per AGENTS.md > Review Learnings (PR #86) > "Match docstrings to semantics", add a one-line note (e.g. an Args / Notes line: "Any mjGEOM_PLANE geoms in the robot spec are stripped before attach to avoid coplanar z-fight with the world's ground plane; pass ground_plane=False to SimWorld if the robot's scene is the only floor.").
• Already in the PR's "Deferred to v0.4.1" list and reviewer-confirmed non-blocking, restated here so they're easy to copy into tracker issues:
  • Conditional strip when world.ground_plane=False (robot whose scene ships the only floor would silently lose it).
  • Tighten the plane filter from type-only (mjtGeom.mjGEOM_PLANE) to a name-allowlist (floor, ground) or a pose check (normal ≈ +z, z ≈ 0). The current filter also strips wall / ramp / vertical planes if a robot scene ever ships one.
  • Expand the regression matrix: ground_plane=False, multi-plane robot scenes, non-floor planes, real menagerie scene (Panda) integration test.
• CHANGELOG: this is a user-visible behaviour change for anyone who feeds a custom MJCF with a deliberate <geom type="plane"> (other than a floor) into attach_robot. Worth a one-liner under v0.4.0 notes so the silent-strip isn't a surprise.

Verification suggestions

• Sanity-check on a real menagerie scene the PR was motivated by: python -c "from strands_robots.simulation.mujoco.spec_builder import SpecBuilder; from strands_robots.simulation.world import SimWorld; from strands_robots.simulation.robot import SimRobot; import mujoco; spec = SpecBuilder.build(SimWorld(ground_plane=True)); SpecBuilder.attach_robot(spec, SimRobot(name='panda', urdf_path='<menagerie>/franka_emika_panda/scene.xml', position=[0,0,0], orientation=[1,0,0,0]), '<menagerie>/franka_emika_panda/scene.xml'); m = spec.compile(); print(sum(1 for g in range(m.ngeom) if m.geom_type[g] == mujoco.mjtGeom.mjGEOM_PLANE))" — expect 1.
• Run only the new test for fast feedback: hatch run test tests/simulation/mujoco/test_spec_builder.py::test_attach_robot_strips_robot_scene_ground_plane.
• Visual smoke (optional): offscreen-render the resulting model and confirm the floor texture is uniform (no flicker / triangulation artefacts).

[yinsong1986]

[FOLLOW-UP] Type-only filter (g.type == mujoco.mjtGeom.mjGEOM_PLANE) strips every plane geom — not just floors. A robot MJCF that ships a wall, a ramp, or a +x-facing plane as part of its workspace would lose those too. Reviewer-confirmed deferred per the PR description ("Tighten plane filter from type-only to a name-allowlist or pose-check"); restating inline so the tracker issue captures the exact filter line. Suggested tightening for v0.4.1: keep only planes whose normal is ≈ +z AND whose pos[2] ≈ 0, or restrict to a name allowlist ({"floor", "ground"}) — both are one-line changes.

[yinsong1986]

[FOLLOW-UP] Test asserts len(plane_ids) == 1 and name == "ground" only for the ground_plane=True path. The symmetric case — SimWorld(ground_plane=False) + a robot whose MJCF ships its own floor — is the silent-floor-loss scenario flagged in the PR's deferred list, and is the higher-risk one (visible scene with no floor at all, robot falls through). Worth a paired test in v0.4.1 that asserts the current (deliberately permissive) behaviour now, so when the v0.4.1 conditional-strip lands the test flips to assert the kept plane. Not blocking — pinning the regression for the current fix is sufficient for v0.4.0.

[yinsong1986]

Summary

Small, well-scoped fix: deletes any mjGEOM_PLANE geom from a robot's loaded MjSpec before calling scene_spec.attach(...), eliminating the coplanar-z=0 z-fight against the world's ground plane. The accompanying regression test compiles a real MjModel and asserts (a) exactly one surviving plane and (b) it is named ground — that's a strong post-condition, not a schema-only check.

Docstring on attach_robot (per AGENTS.md PR #86 "Match docstrings to semantics") explicitly surfaces the silent strip and points users with floor-only-in-robot-MJCF at the v0.4.1 conditional-strip work.

What's good

• Compile-then-assert regression test (round-trips through spec.compile() rather than poking the spec tree). Pins the fix per AGENTS.md PR #85 "Pin regression tests for reviewed fixes".
• Docstring discloses the silent-strip behaviour up front rather than burying it in code comments only.
• Inline comment cites the exact menagerie example (franka_emika_panda/scene.xml) — future reader doesn't have to re-derive the failure case.
• Scope discipline: zero changes outside the two files in scope. PR #86 "Reject silently-dropped kwargs" / scope-creep guidance respected.
• Review-changelog table in the PR body is exemplary — every reviewer round has fix-commit + pin-test references.

Must fix before merge

(none — PR is ready to merge once any follow-ups are tracked)

No security, crash, data-corruption, or one-way-door findings. The behavioural change is silent but pre-1.0, internal-API only (SpecBuilder.attach_robot is not in __all__-exported user surface), explicitly documented in the docstring, and the user-facing escape hatch (SimWorld(ground_plane=False) + v0.4.1 conditional strip) is already on the roadmap. CHANGELOG-on-v0.4.0 is a doc gap, not a one-way door.

Follow-up in v0.4.1

• robot_spec.geoms traverses the entire spec, not just worldbody (spec_builder.py:424). The current filter strips ANY mjGEOM_PLANE anywhere in the robot's spec tree, including planes nested inside bodies. Today's robot_descriptions assets only use planes as worldbody floors so this is benign, but it's a wider net than the docstring ("robot scene's plane(s)") implies. Folds naturally into the already-tracked "tighten plane filter from type-only to name-allowlist or pose-check" v0.4.1 item — worth adding pose-on-z=0 plane and worldbody-direct only as candidate predicates when that work lands.
• Test matrix expansion (already tracked): add ground_plane=False + robot-with-plane (asserts the single surviving plane is the robot's, not the world's, once conditional strip lands), multi-plane robot scenes, and a non-floor <geom type="plane"> (e.g. wall) case to pin the desired post-conditional-strip semantics.
• CHANGELOG entry under v0.4.0 (already tracked): one-liner flagging the silent strip for users feeding custom MJCF with deliberate non-floor <geom type="plane">. Keep-a-Changelog "Changed" section, since this is behavioural rather than additive.
• Conditional strip when world.ground_plane=False (already tracked): the docstring currently has to tell users "build the world with SimWorld(ground_plane=False) — note that the strip is unconditional in v0.4.0". That's a wart the v0.4.1 work erases; once it lands, prune that paragraph.

Verification suggestions

hatch run test tests/simulation/mujoco/test_spec_builder.py::test_attach_robot_strips_robot_scene_ground_plane -v

For an end-to-end smoke (offscreen render, no display required) — useful given the bug was visual-only and CI doesn't render:

import numpy as np, mujoco, os
os.environ["MUJOCO_GL"] = "egl"
from strands_robots.simulation.models import SimWorld, SimRobot
from strands_robots.simulation.mujoco.spec_builder import SpecBuilder

# Use a menagerie scene that ships its own floor (panda is the canonical case).
world = SimWorld(ground_plane=True)
spec = SpecBuilder.build(world)
robot = SimRobot(name="arm", urdf_path="<path/to/franka_emika_panda/scene.xml>",
                 position=[0,0,0], orientation=[1,0,0,0])
SpecBuilder.attach_robot(spec, robot, robot.urdf_path)
model = spec.compile()
planes = [i for i in range(model.ngeom) if model.geom_type[i] == mujoco.mjtGeom.mjGEOM_PLANE]
assert len(planes) == 1 and mujoco.mj_id2name(model, mujoco.mjtObj.mjOBJ_GEOM, planes[0]) == "ground"

[yinsong1986]

[FOLLOW-UP] robot_spec.geoms returns every plane geom in the spec tree, not just the worldbody-direct floor. The docstring (line 416-426) says "robot scene's plane(s)" which reads like the worldbody floor, but the filter would also strip a <geom type="plane"> nested inside a body (e.g. an end-effector pad, or a wall under a robot's mobile base). No current robot_descriptions asset hits this case, so this is non-blocking — but when the already-tracked v0.4.1 "tighten plane filter from type-only to name-allowlist or pose-check" work lands, please also constrain the traversal to robot_spec.worldbody.geoms (direct children) plus a pose-on-z=0 predicate. That makes the silent-strip scope match what the docstring promises.

[yinsong1986]

[FOLLOW-UP] Strong post-condition (compile + assert exactly one plane named ground) — good. To pin the v0.4.1 conditional-strip behaviour when it lands, please add a sibling test for SimWorld(ground_plane=False) + robot-with-plane that asserts the surviving plane is the robot's (not stripped). Today on main that test would fail (unconditional strip ⇒ zero planes ⇒ no floor), which is exactly what makes it a useful pin for the v0.4.1 fix. Also worth a geom type="plane" not-at-z=0 (a vertical wall) case so the future pose-check predicate has regression coverage.