fix: correct three spatially-wrong scene-setup answers (#37, #38, #39) by whats2000 · Pull Request #45 · whats2000/isaacsim-mcp-server

whats2000 · 2026-09-02T10:51:37Z

What does this PR do?

Fixes #37
Fixes #38
Fixes #39

Three defects found in one agentic session, all of the same family: a scene-setup
surface that hands back a spatially wrong number and nothing errors. They share
code (_find_collision_floor serves both #37 and #38) and they share a live
verification sweep, so they land together.

#37 — `load_environment` + `create_physics_scene` left two collision floors

The ground-plane guard only asked whether /World/groundPlane existed, never
whether the stage already had a floor — so the setup order the server
instructions prescribe always stacked a second one. In simple_warehouse both
sit at z=0 and nothing looks wrong; on an environment whose floor is elsewhere,
objects rest at a height nothing explains, and which plane wins is PhysX's
decision rather than the caller's.

create_physics_scene now skips its plane when a collision-enabled Plane is
already present, and reports ground_plane (the floor objects will land on) and
ground_plane_created.

The floor test is deliberately narrow. The looser rule considered — any wide,
thin collider — also catches Mesh-authored floors, but a wall panel or tabletop
matches it equally well, and suppressing a floor the caller needs is worse than
the stacking. A Mesh-floored environment therefore still stacks; stated here
rather than papered over.

#38 — `bounds.floor_height` was the bounding-box minimum, not the floor

Documented as the value you place objects on, but any geometry dipping below the
floor dragged it down: simple_warehouse reported -0.009 against a collision
floor at 0.0, so placing on it embedded the object 9 mm and resolved as a
settle or jitter rather than an error.

Now two fields: floor_height derived from the environment's own collision
floor, bounds_min_z for the honest bbox value. floor_height_source says which
was used; when no collision floor can be measured it falls back to the bbox
minimum, labelled, with a floor_height_warning.

Derived through the world transform — _reference_conversion rotates and
rescales Y-up/centimetre environments immediately before bounds are read, so a
floor prim's local translate is not its height on the stage.

#39 — `transform.position` had no frame, and two different ones

It was the parent-relative pose with nothing saying so, while actual_size in
the same response was world-space. Robot links are where it bit: with an FR3 at
(2, 1, 0), fr3_hand_tcp reads local [0.1832, 0.0001, 0.8549] — a perfectly
plausible-looking end-effector position, wrong by the base offset. At the default
origin the two frames coincide, which is why the tool looked correct.

Worse than filed: on Newton the Fabric branch wrote a world position over
position, so that field was world on Newton and parent-relative everywhere else.

Breaking: there is no bare position. position_local and position_world
are both named, so a caller has to choose. Naming one frame and leaving the other
unqualified keeps the trap — the unqualified name reads as the default. This
breaks readers loudly, with a missing key rather than a wrong number.

Two further defects surfaced by that rename, neither in the issue

step_simulation(observe_prims=...) mixed frames in the same field. Its
PhysX/tensor branch returns world positions; the USD fallback beneath returned
parent-relative ones — so the frame depended on whether the physics read
happened to succeed, in the field most used to measure motion. Six sites
across both adapters. They read transform.get("position", [0, 0, 0]), which
after the rename would not have raised: it would have reported the origin as
though measured.
create_camera(target=...) aimed from the wrong frame. With no explicit
position it read the camera's own pose as the eye and passed it to a look-at
against a world target. A nested camera was aimed from a parent-relative
point and still produced a rotation, so it failed silently.

Tested Isaac Sim Version(s)

Isaac Sim 6.0.x — PhysX (isaac-sim.sh)
Isaac Sim 6.0.x — Newton (isaac-sim.newton.sh)
Isaac Sim 5.1.x
Other (please specify):

Exact builds: 6.0.1-rc.7 (engine=physx), 6.0.1-rc.7 (engine=newton),
5.1.0-rc.19 (V5 adapter, confirmed by the absence of the V6-only
engine/isaacsim_version fields). Cold-booted and run sequentially,
one instance per GPU (RTX 3090). No gaps: all three runtimes were run for
all three issues, 3 rounds each = 27 live rounds, 27 pass.

Plus scripts/smoke_test.py 19/19 on 5.1, covering both surfaces #39 touched.

Controls

Each sweep carried controls, because a probe that always answers "correct" would
pass without measuring anything:

#37

counter control, per runtime: two hand-authored collision Planes are reported
as two. The counter is not blind.
empty-stage control, per runtime: with no environment, /World/groundPlane
is still created, created=True. A fix suppressing the plane unconditionally
would pass the main check and break every scene without an environment.
negative control, 5.1: replaying what the pre-fix code did — authoring
/World/groundPlane with CollisionAPI unconditionally after
load_environment — still yields two floors. So the pass is the fix acting,
not the environment having changed.

#38

every round asserted floor_height and bounds_min_z genuinely differ
(0.0 vs -0.009); a handler still returning the bbox minimum would fail.
compared against an independent USD traversal, not the handler's own logic.
physics raycast, 5.1 and 6.0 PhysX: raycast_closest hits z=0.0 on the same
CollisionPlane — an independent physics measurement agreeing with the USD
derivation.
Newton: raycast_closest returns hit=False, confirming the issue's report
that it cannot locate the floor there. The USD derivation returns the correct
0.0 on that same stage, so [Bug] load_environment's floor_height is the environment bbox minimum, not the floor #38 is now measured on all three runtimes
rather than the two the issue could reach.
fallback path, on a real stage: a synthetic Mesh-floored environment with a
prop sunk to -0.5 reported floor_height -1.0, source bounds_min_z, warning
present. A metre wrong, and labelled — the case the label exists for.

#39

every round used the issue's own repro and asserted the two frames differ.
Newton Fabric branch, the path this change alters: a rigid-body sphere
(radius 0.1) under a parent at (1,2,0), spawned at local z=2.0, after 60 steps
read position_local [0.25, 0, 2.0] (untouched spawn pose) and
position_world [1.25, 2.0, 0.09932], source physics. X/Y = parent + local;
Z = radius resting on the floor. Both independently correct.
observe_prims on a nested non-rigid prim now reports [1.25, 2.0, 0.5],
matching its physics branch; pre-fix it reported [0.25, 0, 0].
camera aim: camera under a parent at (5,0,3), local (0,0,0), aimed at the
origin → rotation [59.04, 0, 90]. Proof the world eye was used: with the
local eye, eye and target coincide and look_at_euler returns None, so no
aim would be produced at all. Angle check: atan(3/5) = 31° below horizontal
→ pitch 90-31 = 59.04.

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guidelines? Yes.
Was this discussed/approved via a GitHub issue? Please add a link to it if that's the case. [Bug] load_environment + create_physics_scene leaves two collision floors on the stage #37, [Bug] load_environment's floor_height is the environment bbox minimum, not the floor #38, [Bug] get_prim_info returns a local position with no indication of the frame #39.
Did you make sure to update the documentation with your changes? Tool docstrings for create_physics_scene, clear_scene, load_environment, get_prim_info; plus CHANGELOG.
Did you run the linter (ruff check . && ruff format .)? Clean.
Did you write any new necessary tests? 3 new files, 22 tests; suite 304 passed, 43 skipped.
Did you manually test the behavior in a running Isaac Sim instance? (unit tests alone are not sufficient) 27 live rounds across all three runtimes, detailed above.

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@whats2000 — flagging you directly because #39 is a breaking response-shape
change (transform.position is gone), so it wants an explicit call rather than a
quiet merge.

Notes for the reviewer

[Bug] get_prim_info returns a local position with no indication of the frame #39 is a breaking change — transform.position is gone. It is under
Changed in the CHANGELOG, not Fixed.
The Mesh-floor gap is shared by [Bug] load_environment + create_physics_scene leaves two collision floors on the stage #37 and [Bug] load_environment's floor_height is the environment bbox minimum, not the floor #38 and remains open by design.
[Bug] load_environment's floor_height is the environment bbox minimum, not the floor #38 labels it; [Bug] load_environment + create_physics_scene leaves two collision floors on the stage #37 silently stacks. One widening would close both if it ever
bites in practice.
Two source-substring tests changed deliberately alongside the contract, per the
repo convention that docs and their tests move together:
test_adapter_v6.py::test_v6_get_prim_transform_labels_a_usd_fallback_on_newton
and test_prim_transform.py::test_reader_reports_rotation_and_scale_not_just_position.
test_create_physics_idempotent's fake gained Traverse. Without it those
tests passed through the new floor search's exception fallback rather than the
search itself — green for the wrong reason.

🤖 Generated with Claude Code

create_physics only ever asked whether /World/groundPlane existed, never whether the stage already had a floor. load_environment brings its own collision floor, so the setup order the server instructions prescribe -- load_environment then create_physics_scene -- always left two. Measured 9/9 on 5.1, 6.0 PhysX and 6.0 Newton before this change. In simple_warehouse both planes sit at z=0, which is why it went unnoticed: nothing looks wrong until an environment whose floor is not at the origin parks objects at a height nothing on screen explains, and which plane wins is PhysX's decision rather than the caller's. The floor test is deliberately narrow -- a prim of type Plane carrying CollisionAPI. The looser rule considered (any wide, thin collider) also catches Mesh-authored floors, but a wall panel or a tabletop matches it equally well, and suppressing a floor the caller needs is a worse failure than the stacking. A Mesh-floored environment therefore still stacks; recorded on the issue rather than papered over. The response now carries ground_plane (the floor objects will actually land on) and ground_plane_created. A bare success read as "I made you a ground plane" even when the environment's own floor was the operative one, and the path is what lets a caller query the real floor height instead of assuming z=0. The existing /World/groundPlane idempotency guard folds into the same search: a second call finds our own plane as a collision Plane and skips. test_create_physics_idempotent's fake gained Traverse so those tests exercise the search rather than passing through its exception fallback -- without it they were green for the wrong reason. Verified live, 3 cold-booted runtimes on one RTX 3090, sequentially (one instance per GPU), 3 rounds each -- 9/9, matching the 9/9 that reproduced the defect: 6.0.1-rc.7 PhysX 3/3 engine=physx 6.0.1-rc.7 Newton 3/3 engine=newton 5.1.0-rc.19 3/3 V5 adapter Each round: clear_scene -> load_environment(simple_warehouse) -> create_physics, counting collision-enabled Plane prims with world Z. One floor after each step, ground_plane_created=False, ground_plane naming the environment's own CollisionPlane. Three controls, because a probe that always answers "one floor" would pass this without measuring anything: - counter control, per runtime: two hand-authored collision Planes are reported as two. The counter is not blind. - empty-stage control, per runtime: with no environment loaded, /World/groundPlane is still created and created=True. A fix that suppressed the plane unconditionally would pass the main check and break every scene without an environment. - negative control, on 5.1: after load_environment, replaying exactly what the pre-fix code did -- author /World/groundPlane with CollisionAPI unconditionally -- still yields two floors. So the 9/9 is the fix acting, not the environment having changed underneath. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

bounds.floor_height was documented as the value that lets a caller "place objects on the ground without a second query", but it was the bounding-box minimum Z of the whole environment. Any geometry dipping below the floor -- trim, a recessed drain, a slightly sunk prop -- drags that down. It is the environment's lowest point, which is not its floor. simple_warehouse reported -0.009 against a collision floor at 0.0, so placing on the reported value embedded the object 9mm and resolved as a small settle or a jitter on the first physics step rather than as an error. The size of the error is a property of whichever environment is loaded, so it could not be corrected for once and reused. The two meanings are now two fields. bounds_min_z is the bbox minimum -- honest, and the same thing extent already means. floor_height is derived from the environment's own collision floor, scoped to the environment subtree: a /World/groundPlane left by an earlier create_physics_scene is not this environment's floor and on a dirty stage sits at a different height. floor_height_source names which was used, and floor_prim names the prim measured. Derived through the world transform, not the authored one: _reference_conversion rotates and rescales Y-up / centimetre environments immediately before bounds are read, so a floor prim's local translate is not its height on the stage. An environment whose floor is a Mesh has no collision Plane to measure -- the same narrow rule as #37. There floor_height falls back to the bbox minimum, which is the old wrong answer, so it is labelled floor_height_source=bounds_min_z and carries floor_height_warning rather than being handed back looking measured. Omitting it instead would push the caller into raw USD to find the floor, which is the round trip this field exists to remove. This is the treatment position_source and velocity_warning already give a value that may not mean what it looks like. _find_collision_floor gained an optional root; the root=None path is byte-identical, so #37's live measurements still hold. Verified live, 3 cold-booted runtimes, 3 rounds each -- 9/9: 6.0.1-rc.7 PhysX 3/3 6.0.1-rc.7 Newton 3/3 5.1.0-rc.19 3/3 Each round compared the handler against an independent USD traversal of the environment subtree. Every round also asserted the two values actually differ (0.0 vs -0.009): a handler still returning the bbox minimum would otherwise have looked correct. Controls: - physics raycast, 5.1 and 6.0 PhysX: raycast_closest from [0,0,2] hits z=0.0 on the same CollisionPlane -- an independent physics measurement agreeing with the USD derivation, 9mm from bounds_min_z. - Newton: raycast_closest returns hit=False, confirming the issue's report that it cannot locate the floor there. The USD derivation returns the correct 0.0 on that same stage, so #38 is now measured on all three runtimes rather than the two the issue could reach. - fallback path, on a real stage: a synthetic Mesh-floored environment with a prop sunk to -0.5 reported floor_height -1.0, source bounds_min_z, floor_prim None, warning present. A metre wrong, and labelled -- which is the case the label exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

get_prim_info's transform.position was the prim's parent-relative pose, and neither the response nor the tool description said so. For a prim under a transformed parent the number is not where the object is: a child at local (0.25, 0, 0) under a parent at (1, 2, 0.5) reported [0.25, 0, 0]. Reproduced 9/9 across 5.1, 6.0 PhysX and 6.0 Newton. Two things made it easy to walk into. actual_size in the same response is world-scale-aware, so one response mixed a local position with a world-space size. And robot links are addressed as /World/Franka/fr3_hand_tcp -- Isaac's FR3 is a flat hierarchy rooted at the articulation, so with the robot at the default origin the two frames coincide and the tool looks correct. Measured here with an FR3 at (2,1,0): fr3_hand_tcp reads local [0.1832, 0.0001, 0.8549], which looks like a perfectly plausible end-effector position and is wrong by the base offset. Worse than filed: on Newton the Fabric branch wrote a WORLD position over `position`, so that one field was world on Newton and parent-relative everywhere else, decided by engine and prim type. There is deliberately no bare `position`. Naming one frame and leaving the other unqualified keeps the trap -- the unqualified name reads as the default and the qualified one as a special case, which is the misreading that put a base-relative coordinate into a grasp calculation. Both frames are named, so a caller has to choose. This breaks readers of `position`, and it breaks them loudly with a missing key rather than quietly with a wrong number, which is the right way round for this failure. rotation and scale stay unqualified: they are local by definition, being the values transform_object writes back in the same convention. Two further defects surfaced while fixing this. Neither is in the issue. 1. step(observe_prims=) mixed frames in the same field. Its primary branch reads PhysX (v5) or the tensor view (v6), both of which report world positions, while the USD fallback beneath reported parent-relative ones -- so the frame depended on whether the physics read happened to succeed, invisibly, in the field most used to measure motion. Six sites across both adapters. Note these read transform.get("position", [0,0,0]), which after the rename would not have raised: it would have reported the origin as though measured. 2. create_camera(target=) aimed from the wrong frame. With no explicit position it reads the camera's own pose as the eye point and passes it to look_at_euler against a world target. A nested camera was aimed from a parent-relative point and still produced a rotation, so it failed silently. Verified live, 3 cold-booted runtimes, 3 rounds each -- 9/9: 6.0.1-rc.7 PhysX 3/3 6.0.1-rc.7 Newton 3/3 5.1.0-rc.19 3/3 Every round used the issue's own repro and asserted the two frames differ, so a tool still reporting only the local value could not pass. Controls: - Newton Fabric branch, the path this change actually alters: a rigid-body sphere (radius 0.1) under a parent at (1,2,0), spawned at local z=2.0, after 60 steps read position_local [0.25, 0, 2.0] (the untouched spawn pose) and position_world [1.25, 2.0, 0.09932] with source "physics". X/Y = parent + local; Z = radius resting on the floor. Both independently correct. - observe_prims on a nested non-rigid prim now reports [1.25, 2.0, 0.5], matching its physics branch; pre-fix it reported [0.25, 0, 0]. - camera aim: a camera under a parent at (5,0,3) with local (0,0,0), aimed at the origin, produced rotation [59.04, 0, 90]. Proof the world eye was used: with the local eye, eye and target coincide and look_at_euler returns None, so no aim would be produced at all. Angle check: atan(3/5) = 31 deg below horizontal -> pitch 90-31 = 59.04. - smoke_test.py 19/19 on 5.1, covering both touched surfaces. Two source-substring tests changed deliberately alongside the contract, per the convention that docs and their tests move together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two PRs in a row were written as a document resembling the template rather than built from it. Both times the same three things went wrong: a section dropped entirely (## Who can review?), checkbox labels reworded or trimmed ("Other (please specify):" shortened to "Other", "(unit tests alone are not sufficient)" cut), and the deviation only noticed after posting. The failure mode is composing freehand and eyeballing the result. So the rule is mechanical: start from the template file, strip the  guidance, keep every heading and checkbox line byte-identical, tick boxes and append answers after the label, and diff the composed body against the template before posting. The runtime checkboxes are a claim about what was actually run. The template already says stating a gap is fine and implying coverage you do not have is not; recorded here as a rule rather than as guidance sitting in a comment that gets stripped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🔵 Needs a closer look

It introduces breaking API changes and modifies core spatial/physics semantics across adapters and handlers, warranting final human review despite strong testing.

Pull request overview

This PR fixes three related “silent spatial wrong answer” defects in scene setup and prim transform reporting across the Isaac Sim MCP extension, including a breaking change to make position frames explicit and consistent across runtimes.

Changes:

Prevent create_physics_scene from stacking a second collision ground plane when an environment already provides a collision-enabled Plane; return ground_plane + ground_plane_created.
Split environment floor reporting into floor_height (collision-floor-derived when possible) vs bounds_min_z (bbox minimum), with explicit source/warning labeling.
Replace ambiguous transform.position with position_local and position_world, fix mixed-frame fallbacks in step_simulation(observe_prims=...), and correct create_camera(target=...) to aim from a world-space eye.

File summaries

File	Description
tests/test_prim_transform.py	Updates substring-based assertions to require explicit `position_local`/`position_world` keys.
tests/test_prim_position_frame.py	Adds unit tests covering frame split, retired key, camera aim fallback, and observe_prims frame consistency.
tests/test_ground_plane_dedup.py	Adds unit tests ensuring collision-plane dedup behavior for `create_physics_scene`.
tests/test_environment_floor_height.py	Adds unit tests for separating bbox min Z from collision-floor-derived `floor_height` plus labeling.
tests/test_create_physics_idempotent.py	Extends stage fake to support traversal so floor-search logic is exercised in idempotency tests.
tests/test_adapter_v6.py	Adds/adjusts assertions verifying Newton Fabric pose maps to `position_world` with correct source labeling.
scripts/smoke_test.py	Updates live smoke test to accept the new explicit position keys (prefer world).
isaac.sim.mcp_extension/isaac_sim_mcp_extension/handlers/sensors.py	Fixes `create_camera(target=...)` to use world-space eye when deriving look-at rotation.
isaac.sim.mcp_extension/isaac_sim_mcp_extension/handlers/scene.py	Introduces collision-floor discovery + world-Z helper; updates physics creation and environment bounds reporting.
isaac.sim.mcp_extension/isaac_sim_mcp_extension/adapters/v6.py	Fixes Newton Fabric pose to populate `position_world`; updates observe_prims fallback to world frame.
isaac.sim.mcp_extension/isaac_sim_mcp_extension/adapters/v5.py	Updates observe_prims fallback to world frame to match the physics branch semantics.
isaac.sim.mcp_extension/isaac_sim_mcp_extension/adapters/transforms.py	Adds world translation helper; returns explicit local/world positions and removes ambiguous `position`.
isaac_mcp/tools/scene.py	Updates MCP tool docstrings for the new responses/semantics (ground plane + explicit frames + floor reporting).
CHANGELOG.md	Documents breaking change for `get_prim_info` transform position fields and related fixes.

Review details

Files reviewed: 15/15 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Two findings from the review on #45, both verified before changing anything. 1. test_applies_collision_to_an_existing_uncollided_ground_plane asserted nothing about collision being applied. Confirmed empirically: with `UsdPhysics.CollisionAPI.Apply(gp)` disabled the test still passed, because the response dict is identical whether or not collision was applied, so asserting on it cannot tell the two apart. The fixture now yields the mock and the test asserts Apply was called once; re-running the same sabotage now fails it, which is the check that the assertion is real. Added the negative half too: a plane that already carries CollisionAPI must not have it applied again. 2. create_physics_scene's docstring said a loaded environment "brings its own collision floor, so this does not add a second one" with no qualification, while the implementation deliberately recognises only a collision-enabled prim of type Plane. A Mesh-authored floor is not recognised and still stacks. That limitation was recorded in the code comment, the commit message and the PR body -- none of which an agent reads. It is now in the tool docstring, which is the contract, together with what to do about it: ground_plane_created being true after loading an environment is the signal to verify the floor. load_environment's docstring already covered the same gap through floor_height_source, so it is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

whats2000 and others added 3 commits September 2, 2026 17:04

Copilot AI lite review requested due to automatic review settings September 2, 2026 10:51

Copilot started reviewing on behalf of whats2000 September 2, 2026 10:52 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread isaac_mcp/tools/scene.py Outdated

Comment thread tests/test_ground_plane_dedup.py

Navigation Menu

fix: correct three spatially-wrong scene-setup answers (#37, #38, #39) - #45

fix: correct three spatially-wrong scene-setup answers (#37, #38, #39)#45
whats2000 wants to merge 5 commits into
mainfrom
fix/spatial-truth-bundle

whats2000 commented Sep 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

whats2000 commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

#37 — load_environment + create_physics_scene left two collision floors

#38 — bounds.floor_height was the bounding-box minimum, not the floor

#39 — transform.position had no frame, and two different ones

Two further defects surfaced by that rename, neither in the issue

Tested Isaac Sim Version(s)

Controls

Before submitting

Who can review?

Notes for the reviewer

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

whats2000 commented Sep 2, 2026 •

edited

Loading

#37 — `load_environment` + `create_physics_scene` left two collision floors

#38 — `bounds.floor_height` was the bounding-box minimum, not the floor

#39 — `transform.position` had no frame, and two different ones