Skip to content

[Newton] Implements Frame Transformer Sensor#4822

Merged
camevor merged 12 commits intoisaac-sim:developfrom
camevor:ca/newton-frame-transformer
Apr 13, 2026
Merged

[Newton] Implements Frame Transformer Sensor#4822
camevor merged 12 commits intoisaac-sim:developfrom
camevor:ca/newton-frame-transformer

Conversation

@camevor
Copy link
Copy Markdown
Contributor

@camevor camevor commented Mar 4, 2026

Description

  • Rewrites the Newton FrameTransformer sensor to use a site-injection pipeline backed by SensorFrameTransform. Source and target sites are registered before replication via
    NewtonManager.cl_register_site, so they automatically replicate per-world.
  • Replaces the old per-view body lookup and inline Warp kernels with zero-copy transform_to_vec_quat views and a flat strided sensor output.
  • Adds wildcard body matching for target frames (e.g. "Robot/finger.*" expands to one target per matched body per env).
  • Also includes a one-line fix to RigidObjectData initializing _default_root_state = None for lazy allocation, and a minor contact sensor fix.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Mar 4, 2026
@camevor camevor force-pushed the ca/newton-frame-transformer branch from d01bb4f to 3be332e Compare March 16, 2026 15:15
@camevor camevor changed the title Ca/newton frame transformer [Newton] Implements Frame Transformer Sensor Mar 16, 2026
@camevor camevor self-assigned this Mar 16, 2026
@camevor camevor marked this pull request as ready for review March 16, 2026 15:41
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 16, 2026

Greptile Summary

Implements the Newton FrameTransformer sensor by wrapping Newton's SensorFrameTransform. The PR introduces a site registration lifecycle where sensors register site requests during __init__, sites are injected into prototype builders before replication (or into the flat builder in the fallback path), and per-env site indices are resolved at PHYSICS_READY time. Key changes:

  • New FrameTransformer sensor (isaaclab_newton): replaces inline warp kernels with a site-based approach. Supports per-env source/target site registration, wildcard body matching (e.g., Robot/.*), and zero-copy transform views via the new transform_to_vec_quat utility.
  • NewtonManager site injection API: cl_register_site, _cl_inject_sites (replication path), and _cl_inject_sites_fallback (non-replication path) manage the full lifecycle. Sites are deduplicated by (body_pattern, transform).
  • newton_replicate integration: _build_newton_builder_from_mapping now returns a site index map alongside the builder and stage info, and uses NewtonManager.create_builder for consistent defaults.
  • New transform_to_vec_quat utility (isaaclab): zero-copy splitting of wp.transformf arrays into vec3f and quatf views for 1D/2D/3D arrays.
  • resolve_matching_names enhancement: new raise_when_no_match keyword parameter for graceful pattern-miss handling during prototype injection.
  • InteractiveScene: formats sensor_shape_prim_expr and filter_shape_prim_expr with env regex namespace.
  • RigidObjectData: small fix initializing _default_root_state = None for the deprecated lazy property.
  • Comprehensive test coverage: integration tests (feet-wrt-base, feet-wrt-thigh, robot-to-cube, offset frames, wildcard all-bodies, duplicate body names) and unit tests (validation, site injection fallback, index list building, transform_to_vec_quat).

Confidence Score: 4/5

  • This PR is well-structured with comprehensive test coverage and clean integration into the existing Newton physics pipeline.
  • Score of 4 reflects a solid implementation with thorough testing (both unit and integration), correct handling of both replication and fallback paths, and clean separation of concerns. Minor concern: assert statements used for runtime validation could be silently disabled with Python -O. No critical bugs found.
  • Pay attention to source/isaaclab_newton/isaaclab_newton/sensors/frame_transformer/frame_transformer.py (assert-based validation) and source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py (site injection lifecycle complexity).

Important Files Changed

Filename Overview
source/isaaclab_newton/isaaclab_newton/sensors/frame_transformer/frame_transformer.py Major rewrite of the Newton FrameTransformer — switches from inline warp kernels to site-based registration via NewtonManager, with validation and wildcard body expansion. Well-structured with clear separation of initialization, validation, and update logic.
source/isaaclab_newton/isaaclab_newton/sensors/frame_transformer/frame_transformer_data.py Simplified data container using zero-copy views from transform_to_vec_quat. Previously returned None for several properties; now all properties return actual data.
source/isaaclab_newton/isaaclab_newton/sensors/frame_transformer/frame_transformer_kernels.py New file with two warp kernels: copy_from_newton_kernel (deinterleaves flat Newton output) and compose_target_world_kernel (computes target world transforms). Both correctly handle env_mask null checks.
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py Significant additions for site registration/injection lifecycle: cl_register_site, _cl_inject_sites, _cl_inject_sites_fallback, and add_frame_transform_sensor. Supports both replication and fallback paths with correct pending site clearing. Uses assert for runtime validation in some spots.
source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py Updated to integrate site injection into the replication pipeline. _build_newton_builder_from_mapping now returns a 3-tuple with the site index map. Proto builder creation uses NewtonManager.create_builder for consistent defaults.
source/isaaclab/isaaclab/utils/warp/math_ops.py New utility for zero-copy splitting of wp.transformf arrays into vec3f and quatf views. Handles 1D, 2D, and 3D arrays. Memory layout assumption is verified by tests.
source/isaaclab/isaaclab/utils/string.py Added raise_when_no_match keyword-only parameter to resolve_matching_names. When False, returns empty lists instead of raising ValueError on unmatched patterns. Backward-compatible (defaults to True).
source/isaaclab/isaaclab/scene/interactive_scene.py Added ENV_REGEX_NS formatting for sensor_shape_prim_expr and filter_shape_prim_expr config attributes on ContactSensorCfg. Guarded with hasattr checks for compatibility.
source/isaaclab_newton/test/sensors/test_frame_transformer.py Comprehensive integration tests covering feet-wrt-base, feet-wrt-thigh, robot-to-cube, offset frames, all-bodies wildcard, sensor print, and duplicate body name scenarios. Tests validate both world-frame and relative-frame correctness.

Sequence Diagram

sequenceDiagram
    participant FT as FrameTransformer
    participant NM as NewtonManager
    participant NR as newton_replicate
    participant SFT as SensorFrameTransform

    Note over FT,NM: __init__ phase (before simulation)
    FT->>NM: cl_register_site(None, identity) → world_origin_label
    FT->>NM: cl_register_site(prim_path, source_offset) → source_label
    FT->>NM: cl_register_site(target_path, target_offset) → target_label(s)
    NM-->>NM: Store in _cl_pending_sites

    Note over NM,NR: Replication path (newton_replicate)
    NR->>NM: _cl_inject_sites(builder, protos)
    NM-->>NR: global_sites, proto_sites
    NR->>NR: Compute per-world shape offsets
    NR->>NM: _cl_site_index_map = {...}

    Note over NM: OR Fallback path (start_simulation)
    NM->>NM: _cl_inject_sites_fallback()
    NM-->>NM: Populate _cl_site_index_map

    Note over FT,SFT: PHYSICS_READY callback
    FT->>NM: Read _cl_site_index_map
    FT->>FT: _validate_site_map()
    FT->>FT: _build_sensor_index_lists()
    FT->>NM: add_frame_transform_sensor(shapes, refs)
    NM->>SFT: Create SensorFrameTransform
    NM-->>FT: sensor_index

    Note over FT,SFT: Update loop
    NM->>SFT: sensor.update(state)
    FT->>FT: copy_from_newton_kernel
    FT->>FT: compose_target_world_kernel
Loading

Last reviewed commit: a36ead3

@camevor camevor force-pushed the ca/newton-frame-transformer branch from 61d44e7 to b7f551a Compare April 8, 2026 16:06
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

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

Review: Newton Frame Transformer Sensor

This is a substantial rewrite that replaces the old per-view body lookup approach with a site-injection pipeline backed by Newton's SensorFrameTransform. The architecture is cleaner and the test coverage is thorough. A few items worth addressing:

Architecture

Strengths:

  • The site-injection pipeline (cl_register_site_cl_inject_sites → replication) is well-designed. Registering sites before replication so they automatically replicate per-world is the right approach.
  • Zero-copy transform_to_vec_quat utility is clean and well-bounded (1D/2D/3D).
  • The _validate_site_map and _build_sensor_index_lists static methods are nicely testable in isolation (and indeed tested in test_site_injection.py).

Issues

  1. Mutable class-level state on NewtonManager_cl_pending_sites, _cl_site_index_map, _newton_frame_transform_sensors are all mutable dicts/lists as class attributes. This is a pre-existing pattern in the codebase but adding more of it increases the risk of stale state between scene reloads. The clear() method resets them, but any new path that skips clear() will inherit ghost sites from a previous scene. Consider adding a defensive check at the top of cl_register_site or _cl_inject_sites if the model is already finalized.

  2. compose_target_world_kernel order — The kernel computes source_world * target_relative. In rigid-body math, if target_relative is the transform of the target in the source frame, then the world transform should be source_world ∘ target_relative, which is what wp.transform_multiply(a, b) computes (a * b). This looks correct, but the docstrings in the data class say "target-relative transforms" without explicitly specifying the convention. Worth a one-line comment confirming: T_world_target = T_world_source * T_source_target.

  3. Quaternion convention in docstrings — The base class specifies (x, y, z, w) order for quaternions and Warp's quatf is (x, y, z, w). The Newton data class docstrings say (xyzw) which is consistent — good. However, the old code had explicit split_transform_to_quat4lab converting to (w, x, y, z). The new implementation uses transform_to_vec_quat which returns raw wp.quatf views (xyzw). This is correct for the Warp-native interface, but if any downstream code expects (w, x, y, z) from source_quat_w or target_quat_w, this would be a breaking behavioral change for Newton users who relied on the old vec4f (wxyz) format. The tests use wp.to_torch() and compare against articulation data which is also xyzw, so the tests pass, but it's worth flagging this as a deliberate convention change in the changelog.

  4. resolve_matching_names raise_when_no_match=False returns ([], []) on partial match — If 2 of 3 patterns match, the current implementation returns empty lists rather than the 2 that did match. This is fine for the site injection use case (you want all-or-nothing matching per prototype), but the behavior is a bit surprising for a general-purpose utility. The docstring should clarify this returns empty on any unmatched pattern, not just when all fail.

  5. _cl_inject_sites clears _cl_pending_sites — Both _cl_inject_sites (replication path) and _cl_inject_sites_fallback (non-replication path) clear pending sites. If for some reason both are called (e.g., a code path bug), the second call would silently do nothing. This is probably fine but adding a debug log when pending is already empty would help future debugging.

  6. shape_labels access pattern in _build_sensor_index_lists — The method accesses shape_labels[site_idx] for wildcard expansion. This assumes shape_labels is indexable by int (list) or has int keys (dict). The type hint says list[str] | dict[int, str]. The builder.shape_label in Newton is typically a list, so this works, but shape_labels[site_idx] could IndexError if the site index exceeds the label list length (e.g., if sites were added after the labels snapshot). Worth a bounds check or try/except with a clear error message.

Nits

  • frame_transformer_data.py: The FrameTransformerData.__init__ sets _target_frame_names = None but target_frame_names property accesses it unconditionally. If data.target_frame_names is accessed before _initialize_impl, it returns None rather than raising — consider making it raise explicitly.
  • _default_root_state = None addition in rigid_object_data.py — this looks like an unrelated fix. Should it be in a separate PR or at least mentioned in the PR description?
  • interactive_scene.py removes a comment block about sensor template paths. If the logic is still correct, that's fine, but the comment was useful context.

Tests

Excellent coverage: feet w.r.t. base, feet w.r.t. thigh, robot-to-external-cube, offset frames, all-bodies wildcard, duplicate body names, and isolated unit tests for site injection/validation. The parametrized duplicate-body-name test with source_robot × path_prefix is particularly good.

Overall this is solid work. The site injection pipeline is the right abstraction for Newton's model-building approach.

@camevor
Copy link
Copy Markdown
Contributor Author

camevor commented Apr 9, 2026

@isaaclab-review-bot

  1. Accurate but not actionable for this PR. As stated, this is a pre-existing pattern in the code base. No change needed.
  2. Re-stating the convention for every composition of two transforms adds noise. No change needed.
  3. This convention change was made across the board and documented and is not related to this PR. No change needed.
  4. This is existing behavior. This PR simply uses it. No change needed.
  5. Technically accurate but not a real risk. Both _cl_inject_sites and _cl_inject_sites_fallback clear _cl_pending_sites. Adding a debug log for an impossible condition is unnecessary noise. No change needed.
  6. Tightened type hint to list[str], matching Newton ModelBuilder.shape_labels. Sites are shapes and thus, site_idx is always a valid shape index by construction. The existing assert on site_map membership is the right validation layer and raising with an error message would be no more helpful than a bare IndexError.

Nits:

  1. _target_frame_names is set to a real list in FrameTransformer.__init__, before any external code can access data.target_frame_names. The None window is internal to the constructor. No change needed.
  2. Added to the PR description.
  3. The comment was removed because it described a workaround for sensor template path resolution that is no longer the only concern in that block. No change needed.

@camevor
Copy link
Copy Markdown
Contributor Author

camevor commented Apr 10, 2026

@greptile-apps

Score of 4 reflects a solid implementation with thorough testing (both unit and integration), correct handling of both replication and fallback paths, and clean separation of concerns. Minor concern: assert statements used for runtime validation could be silently disabled with Python -O. No critical bugs found.

Assertions are used in this PR to verify internal invariants, failures of which are unrecoverable. User input and configuration validation does not use assertions but raises errors instead.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 10, 2026

That's a valid distinction. Using assertions for internal invariants (which indicate programming errors) while raising explicit errors for user-facing validation is a common and acceptable pattern. Since these are checking preconditions that should never fail in correct usage, the risk of -O disabling them is acceptable.

Tip: You can give me more context with .greptile/rules so I get this right next time.

Copy link
Copy Markdown
Collaborator

@AntoineRichard AntoineRichard left a comment

Choose a reason for hiding this comment

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

It looks ok, it's quite a substantial change with the site injection, the frame transformer does gets a nice update. @ooctipus I think some of the changes in this PR will require to take a fresh look at the cloner when you get back.

Comment on lines -739 to -741
# Sensor may be nested under a proto_asset_N prim (e.g. a camera on a robot
# link). Search for the actual template location so spawning succeeds even
# though the parent asset lives at template_root/<Asset>/proto_asset_0/...
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this no longer true?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks. Restored it.

# -- default root pose and velocity
self._default_root_pose = wp.zeros((self._num_instances,), dtype=wp.transformf, device=self.device)
self._default_root_vel = wp.zeros((self._num_instances,), dtype=wp.spatial_vectorf, device=self.device)
self._default_root_state = None # lazily allocated by deprecated default_root_state property
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Was that missing var not caught by the tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question, I'm not sure why it wasn't caught.

if not env_mask[env]:
return

target_transforms_w[env, tgt] = wp.transform_multiply(source_transforms[env], target_transforms[env, tgt])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is that better than a plain source_transforms[env] * target_transforms[env, tgt]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, this is exactly the same.

Comment on lines +9 to +34
def transform_to_vec_quat(
t: wp.array,
) -> tuple[wp.array, wp.array]:
"""Split a wp.transformf array into position (vec3f) and quaternion (quatf) arrays.

Zero-copy: returns views into the same underlying memory.

Args:
t: Array of transforms (dtype=wp.transformf). Shape ``(N,)``, ``(N, M)``, or ``(N, M, K)``.

Returns:
Tuple of (positions, quaternions) as warp array views with matching dimensionality.

Raises:
TypeError: If *t* does not have dtype ``wp.transformf``.
"""
if t.dtype != wp.transformf:
raise TypeError(f"Expected wp.transformf array, got dtype={t.dtype}")
floats = t.view(wp.float32)
if t.ndim == 1:
return floats[:, :3].view(wp.vec3f), floats[:, 3:].view(wp.quatf)
if t.ndim == 2:
return floats[:, :, :3].view(wp.vec3f), floats[:, :, 3:].view(wp.quatf)
if t.ndim == 3:
return floats[:, :, :, :3].view(wp.vec3f), floats[:, :, :, 3:].view(wp.quatf)
raise ValueError(f"Expected 1D, 2D, or 3D transform array, got ndim={t.ndim}")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can do slicing now? Might be interesting to keep this in mind when we'll review the asset code. There we use some quite violent code to generate this sort of things.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this could simplify code in a few places.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we have a quick benchmark on the initialization time of this sensor? (To avoid a contact sensor init situation)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll check.

Copy link
Copy Markdown
Contributor Author

@camevor camevor Apr 10, 2026

Choose a reason for hiding this comment

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

A brief checks on 128 and 2048 worlds looks good:

Setup: NVIDIA L40, Python 3.11, Newton 1.1.0.dev0, kitless. Scene: ANYmal C + cube + FrameTransformer (4 feet targets). 200 steps, 20 warmup, 5 runs each.

Initialization (ms, first run discarded)

Phase develop 128 this PR 128 develop 2048 this PR 2048
scene_construction 5237 ±783 3878 ±415 27285 ±29592 23722 ±8969
sim_reset 3805 ±143 3650 ±65 8480 ±521 7666 ±176
total 9042 ±896 7528 ±394 35764 ±29403 31388 ±9018

scene_construction has very large run-to-run variance. It dominates the init measurement and makes the two branches indistinguishable on init time. sim_reset is more stable and shows this PR ~1s faster at 2048 (9036 vs 8005), likely from the slimmer _initialize_impl.

No init regression from the new implementation.

Step Breakdown (ms, median, first run discarded)

Phase develop 128 this PR 128 develop 2048 this PR 2048
write_data_to_sim 1.042 1.042 1.237 1.229
sim.step 0.781 0.792 1.188 1.169
scene.update 0.180 0.166 1.861 0.160
total 2.003 1.999 4.289 2.560

Step-time std across 5 runs is ≤0.03 ms for all phases.
Step time scales much better with this implementation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for commenting the mods to the cloner here. @kellyguo11 we need to start considering another refactor of this class. I think after milestone 2 we need to look into this. That class is getting out of hand.

Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

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

Follow-up Review (3 commits since b7f551a61c83b1)

Changes Since Last Bot Review

  1. "Simplify transform composition"wp.transform_multiply(a, b)a * b in compose_target_world_kernel. Addressed AntoineRichard's inline comment. ✅
  2. "Restore deleted comment" — Comment block in interactive_scene.py restored per AntoineRichard's catch. ✅
  3. "Tighten type hint" — Minor typing improvement. ✅
  4. Merge of develop — No conflicts with PR files.

Previous Review Items — Status

From the initial bot review (4077361704):

# Item Status
1 Mutable class-level state on NewtonManager Acknowledged as pre-existing pattern. clear() resets all new attributes.
2 compose_target_world_kernel convention comment Simplified to * operator. Convention is clear from context (T_w_target = T_w_source * T_source_target).
3 Quaternion convention (xyzw vs wxyz) Tests verify xyzw throughout. Deliberate convention change from old split_transform_to_quat4lab (wxyz). Not called out in changelog — see below.
4 resolve_matching_names partial-match behavior Unchanged. Docstring updated.
5 Double-clear of _cl_pending_sites Acknowledged. Both paths clear, second is benign no-op.
6 shape_labels bounds check Unchanged.

Remaining Items Worth Noting

Quaternion convention change should be documented in the changelog. The old Newton FrameTransformerData returned wp.vec4f in (w,x,y,z) order from split_transform_to_quat4lab. The new implementation returns wp.quatf views in (x,y,z,w) order (Warp native). Any downstream code that indexed source_quat_w or target_quat_w expecting [0]=w will silently get wrong values. The tests pass because they compare against articulation data which is also xyzw, but existing user code migrating from the old Newton FrameTransformer would break. A one-liner in the changelog "Changed" section noting this convention shift would save debugging time.

FrameTransformerData.__init__ sets _target_frame_names = None but the target_frame_names property returns it unconditionally. Accessing data.target_frame_names before _initialize_impl returns None instead of raising. Minor, but an explicit guard would be cleaner.

Newton changelog date mismatch: 0.5.11 (2026-04-07) but the actual commits are from 2026-04-08 through 2026-04-10. Minor nit.

Overall

All reviewer feedback from AntoineRichard has been addressed. The architecture is clean, test coverage is excellent (813 lines of integration tests + 320 lines of unit tests), and the benchmark numbers camevor posted show a 42% step-time improvement at 2048 envs with comparable init time. The site injection pipeline is the right approach for Newton's builder-based replication model.

No blocking issues. The quaternion convention documentation is the most important remaining item — it's a behavioral change that could bite migrating users.

Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

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

🤖 Isaac Lab Review Bot

Summary

This PR rewrites the Newton FrameTransformer sensor from an inline per-view body lookup + Warp kernel approach to a clean site-injection pipeline backed by SensorFrameTransform. The design is solid — registering sites before replication means they automatically replicate per-world without post-hoc index patching. The implementation is well-structured with good separation of concerns (site registration → injection → validation → sensor creation), and ships with thorough test coverage (1100+ lines across integration and unit tests).

Design Assessment

Design is sound. The site-injection approach via NewtonManager.cl_register_site is a clean pattern that lets sensors declare their needs at __init__ time and have the replication machinery handle per-world duplication transparently. The zero-copy transform_to_vec_quat views are an elegant way to expose position/quaternion components without buffer duplication. The strided flat sensor layout ([source₀, target₀₀..target₀ₘ, source₁, target₁₀..target₁ₘ, ...]) is efficient for GPU reads.

The approach correctly handles three code paths:

  1. Replication path (newton_replicate) — sites injected into prototypes before add_builder
  2. USD stage path (instantiate_builder_from_stage) — sites injected into proto, then replicated per-env
  3. Fallback path (_cl_inject_sites_fallback) — sites injected into flat builder (no replication)

Findings

🟡 Warning: assert used for runtime validation — stripped by python -Osource/isaaclab_newton/isaaclab_newton/sensors/frame_transformer/frame_transformer.py:129,201,222

Three assert statements are used to validate that site labels exist in the site map during _initialize_impl and _validate_site_map. These checks guard against real misconfiguration (e.g., a sensor registering a site that wasn't injected) but would silently disappear if Python is run with optimizations enabled (-O or -OO), leading to confusing KeyError or TypeError further downstream instead of a clear diagnostic message.

The PhysX counterpart doesn't use assert for this kind of validation.

# Line 129 — replace:
#   assert self._world_origin_label in site_map
# with:
if self._world_origin_label not in site_map:
    raise RuntimeError(
        f"World-origin site '{self._world_origin_label}' not found in site map. "
        "Ensure NewtonManager site injection ran before initialization."
    )

# Lines 201-205 — replace assert with:
if source_label not in site_map:
    raise ValueError(
        f"FrameTransformer source '{source_prim_path}' (site label '{source_label}') "
        "not found in NewtonManager._cl_site_index_map."
    )

# Lines 222-225 — replace assert with:
if label not in site_map:
    raise ValueError(
        f"FrameTransformer target '{target_prim_paths[tgt_idx]}' (site label '{label}') "
        "not found in NewtonManager._cl_site_index_map."
    )

🟡 Warning: No length validation in add_frame_transform_sensorsource/isaaclab_newton/isaaclab_newton/physics/newton_manager.py:1155

add_frame_transform_sensor passes shapes and reference_sites directly to SensorFrameTransform without validating that they have the same length. A mismatch would likely cause a confusing error inside Newton rather than a clear diagnostic at the Isaac Lab level.

# Suggested guard at the top of add_frame_transform_sensor:
if len(shapes) != len(reference_sites):
    raise ValueError(
        f"shapes and reference_sites must have the same length, "
        f"got {len(shapes)} and {len(reference_sites)}"
    )

🔵 Suggestion: _build_sensor_index_lists accesses shape_labels by integer indexsource/isaaclab_newton/isaaclab_newton/sensors/frame_transformer/frame_transformer.py:271

In the wildcard expansion path, shape_labels[site_idx] indexes builder.shape_label using the site's shape index. This works correctly because add_site returns a valid shape index, but accessing a list by an external integer without bounds checking means a bug in site injection could produce an opaque IndexError. Consider adding a guard or a comment documenting the invariant:

if n_bodies > 1:
    site_idx = per_world[0][k]
    # site_idx is guaranteed valid by _cl_inject_sites / add_site
    expanded_names.append(shape_labels[site_idx].rsplit("/", 2)[-2])

Test Coverage

Coverage is excellent for a new feature PR:

  • Integration tests (test_frame_transformer.py, 813 lines): Covers feet-wrt-base, feet-wrt-thigh, robot-to-external-cube, offset frames, wildcard all-bodies matching, duplicate body names across robots, and sensor print. Tests run with 2 envs, include periodic resets, and verify both world-frame and source-relative transforms against articulation ground truth.
  • Unit tests (test_site_injection.py, 320 lines): Covers transform_to_vec_quat (1D/2D split, zero-copy, dtype errors), fallback site injection (global, local, wildcard, no-match), _validate_site_map (source/target validation, edge cases), and _build_sensor_index_lists (zero targets, single target, multi-env, wildcard expansion).
  • Gap: No explicit test for the _cl_inject_sites replication path (only the fallback path is unit-tested), though the integration tests exercise it indirectly via InteractiveScene with num_envs=2.

CI Status

  • pre-commit: ✅ pass
  • license-check: ⏳ pending
  • Build Latest Docs: ⏳ pending
  • Check for Broken Links: ✅ pass

Verdict

COMMENT

Well-structured rewrite with strong test coverage. The two warnings above are minor hardening improvements — the assertraise change is the most actionable since it affects error reporting in optimized Python runtimes. Overall this is clean, well-tested code ready for merge once the CI checks complete.

site_map = NewtonManager._cl_site_index_map

# Resolve and validate per-env site indices
assert self._world_origin_label in site_map
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Warning: assert is stripped by python -O, turning this into a no-op. Consider:

Suggested change
assert self._world_origin_label in site_map
if self._world_origin_label not in site_map:
raise RuntimeError(
f"World-origin site '{self._world_origin_label}' not found in site map. "
"Ensure NewtonManager site injection ran before initialization."
)

Same applies to the assert statements in _validate_site_map at lines 201 and 222.

"""
sensor = SensorFrameTransform(
cls._model,
shapes=shapes,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Warning: No validation that shapes and reference_sites have the same length. A mismatch would produce a confusing error inside Newton rather than a clear diagnostic.

Suggested change
shapes=shapes,
sensor = SensorFrameTransform(
cls._model,
shapes=shapes,
reference_sites=reference_sites,
)

Consider adding a guard:

if len(shapes) != len(reference_sites):
    raise ValueError(
        f"shapes and reference_sites must have the same length, "
        f"got {len(shapes)} and {len(reference_sites)}"
    )

@camevor camevor merged commit 3d2c67b into isaac-sim:develop Apr 13, 2026
8 checks passed
xiangdonglai pushed a commit to xiangdonglai/IsaacLab that referenced this pull request Apr 15, 2026
# Description

- Rewrites the Newton `FrameTransformer` sensor to use a site-injection
pipeline backed by `SensorFrameTransform`. Source and target sites are
registered before replication via
`NewtonManager.cl_register_site`, so they automatically replicate
per-world.
- Replaces the old per-view body lookup and inline Warp kernels with
zero-copy `transform_to_vec_quat` views and a flat strided sensor
output.
- Adds wildcard body matching for target frames (e.g. `"Robot/finger.*"`
expands to one target per matched body per env).
- Also includes a one-line fix to `RigidObjectData` initializing
`_default_root_state = None` for lazy allocation, and a minor contact
sensor fix.

## Checklist

- [ ] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [ ] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./isaaclab.sh --format`
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [ ] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there
AntoineRichard added a commit that referenced this pull request Apr 17, 2026
## Summary

Fix incorrect attribute name `contact_margin` → `gap` on Newton
`ShapeConfig` in `NewtonManager.create_builder()`.

Newton PR #1732 renamed `contact_margin` to `gap` (broad-phase AABB
expansion). The IsaacLab code was never updated, creating a dead
attribute that Python silently accepted. The intended 1 cm default shape
gap was never applied.

## Newton PR #1732 rename mapping

| Old field | New field | Semantics |
|-----------|-----------|-----------|
| `thickness` | `margin` | Shape surface extension (affects contact
forces) |
| `contact_margin` | **`gap`** | Broad-phase AABB expansion (detection
range only) |
| `rigid_contact_margin` | `rigid_gap` | Builder-level default gap
(already 0.1) |

## Timeline

| Date | Newton | IsaacLab | `contact_margin` valid? |
|------|--------|----------|------------------------|
| Feb 19 | pin: `51ce35e` (has `contact_margin`) | #4646 adds
`contact_margin = 0.01` | Yes |
| Feb 24 | **PR #1732 renames `contact_margin` → `gap`** | — | — |
| Mar 2 | pin updated to `v0.2.3` (after rename) | #4761 keeps
`contact_margin` | **No — broken** |
| Mar 9 | — | #4883 removes the line | Removed |
| Apr 13 | — | #4822 re-adds `contact_margin` | **No — still broken** |

## Ablation: gap vs margin

We conducted an ablation study to understand the impact. Note: `margin`
(shape surface
extension) is a different field from `gap` (broad-phase range). The
original code intended
to set `gap`, but setting `margin` also has a significant positive
effect on training for
rough terrain locomotion.

| Setting | `gap` | `margin` | Go1 Reward (300 iter) | Effect |
|---------|-------|----------|----------------------|--------|
| Dead field (baseline) | 0.1 (default) | 0.0 | ~1.0 | — |
| `gap=0.01` only | 0.01 | 0.0 | 0.66 | No training improvement |
| `margin=0.01` only | 0.1 (default) | 0.01 | 18.77 | Major improvement
|
| `gap=0.01` + `margin=0.01` | 0.01 | 0.01 | 16.54 | Similar to
margin-only |

**This PR fixes the correct field migration** (`contact_margin` →
`gap`). The `margin` setting
for rough terrain contact quality will be addressed separately in the
rough terrain env PR
(#5248) via a new `default_shape_margin` config field on `NewtonCfg`.

## Test plan

- [x] Verified `contact_margin` is not a field on `ShapeConfig` (Newton
1.1.0.dev0)
- [x] Verified `gap` is the correct replacement field per Newton PR
#1732
- [x] Confirmed by camevor (Newton developer)
- [x] Ablation study: gap alone doesn't change training, so existing
tests should pass

Co-authored-by: Antoine RICHARD <antoiner@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants