Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Fixed
^^^^^

* Fixed detached articulation links with Newton and Isaac RTX by falling back
from unvalidated cubric adapter versions.

* Fixed Newton clone imports creating empty MuJoCo custom-frequency rows from
ignored environment subtrees.
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@

from __future__ import annotations

import re
from collections.abc import Callable, Sequence
from dataclasses import replace
from typing import Any

import torch
Expand All @@ -18,6 +20,75 @@
from isaaclab.sim.utils.newton_model_utils import replace_newton_builder_shape_colors


def _add_global_stage_to_builder(
builder: ModelBuilder,
stage: Usd.Stage,
ignore_paths: Sequence[str],
schema_resolvers: Sequence[Any],
) -> Any:
"""Import global prims without parsing custom rows from ignored clone subtrees.

Newton's built-in USD import honors ``ignore_paths``, but its custom-frequency
traversal currently does not. MuJoCo frequencies are also registered from
inside :meth:`ModelBuilder.add_usd`, so both existing and newly registered

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.

Major: Register custom frequencies before attempting to scope them

The fresh global builders used by these call sites have no MuJoCo custom frequencies, and the repository's pinned Newton ModelBuilder.add_usd() does not call add_custom_frequency() despite this statement. Consequently original_filters is empty and the method interception never sees a registration. The newly added test_global_import_does_not_create_tendon_rows_from_ignored_envs fails with KeyError: 'mujoco:tendon', so this workaround does not establish the behavior it claims. Configure the global builder with the active solver's custom attributes before importing, preferably through the same shared builder setup used for source builders. Once registration is explicit, delete the speculative add_custom_frequency monkeypatch and duplicate callback-identity checks; the pinned importer has no path that needs them. Keep the regression assertion proving one global tendon row plus the replicated source rows.

callbacks are scoped for this import and restored afterward.
"""
ignored_patterns = tuple(re.compile(path) for path in ignore_paths)
original_filters = {
frequency_key: frequency.usd_prim_filter for frequency_key, frequency in builder.custom_frequencies.items()
}

def _scope_filter(callback):
if callback is None:
return None

def _filtered(prim: Usd.Prim, context: dict[str, Any]) -> bool:
prim_path = str(prim.GetPath())
if any(pattern.match(prim_path) for pattern in ignored_patterns):

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.

P2 Regex path matching can produce false-positive exclusions

re.compile(path) uses the raw prim-path string as a regex pattern, and pattern.match(prim_path) anchors only at the start (not the end). This means /World/envs also matches /World/envs_global/..., and /World/envs/env_1 matches /World/envs/env_10/.... Any global prim whose path begins with one of the ignore_paths strings as a substring would be silently dropped from the custom-frequency traversal, even though Newton's own add_usd excludes only the intended exact subtrees.

For a correct prefix check, replace the regex compile and match with a plain string prefix comparison that also requires a / separator or an exact match (e.g. prim_path == path or prim_path.startswith(path + "/")), or at minimum use re.compile(re.escape(path) + r"(/|$)") to prevent mid-segment matches.

return False
return callback(prim, context)

return _filtered

for frequency_key, callback in original_filters.items():
builder.custom_frequencies[frequency_key].usd_prim_filter = _scope_filter(callback)

original_add_custom_frequency = builder.add_custom_frequency
had_instance_override = "add_custom_frequency" in vars(builder)
original_instance_override = vars(builder).get("add_custom_frequency")

def _add_scoped_custom_frequency(frequency: ModelBuilder.CustomFrequency) -> None:
frequency_key = frequency.key
existing = builder.custom_frequencies.get(frequency_key)
if existing is not None:
if (
original_filters[frequency_key] is not frequency.usd_prim_filter
or existing.usd_entry_expander is not frequency.usd_entry_expander
):
raise ValueError(f"Custom frequency '{frequency_key}' is already registered with different callbacks.")
return

original_filters[frequency_key] = frequency.usd_prim_filter
original_add_custom_frequency(replace(frequency, usd_prim_filter=_scope_filter(frequency.usd_prim_filter)))

setattr(builder, "add_custom_frequency", _add_scoped_custom_frequency)
try:
return builder.add_usd(
stage,
ignore_paths=ignore_paths,
schema_resolvers=schema_resolvers,
)
finally:
if had_instance_override:
setattr(builder, "add_custom_frequency", original_instance_override)
else:
delattr(builder, "add_custom_frequency")
for frequency_key, callback in original_filters.items():
frequency = builder.custom_frequencies.get(frequency_key)
if frequency is not None:
frequency.usd_prim_filter = callback


def build_source_builders(
stage: Usd.Stage,
sources: Sequence[str],
Expand Down
8 changes: 5 additions & 3 deletions source/isaaclab_newton/isaaclab_newton/cloner/replicate.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from isaaclab.sim.utils.newton_model_utils import replace_newton_builder_shape_colors

from isaaclab_newton.cloner.newton_clone_utils import (
_add_global_stage_to_builder,
build_source_builders,
rename_builder_labels,
replicate_builder_mapping,
Expand Down Expand Up @@ -61,10 +62,11 @@ def _build_newton_builder_from_mapping(
manager_cls = PhysicsManager._sim.physics_manager

builder = manager_cls.create_builder(up_axis=up_axis)
stage_info = builder.add_usd(
stage_info = _add_global_stage_to_builder(
builder,
stage,
ignore_paths=["/World/envs", *sources],
schema_resolvers=schema_resolvers,
["/World/envs", *sources],
schema_resolvers,
)
replace_newton_builder_shape_colors(builder, stage)

Expand Down
20 changes: 4 additions & 16 deletions source/isaaclab_newton/isaaclab_newton/physics/_cubric.py

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.

I didn't know that was a thing, that looks lovely /s

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.

Changes are reasonable otherwise.

Original file line number Diff line number Diff line change
Expand Up @@ -250,10 +250,9 @@ def initialize(self) -> bool:
def _verify_iadapter_version(fw_ptr: int, ia_ptr: int) -> bool:
"""Verify the acquired IAdapter is compatible with this shim's vtable offsets.

Major mismatches and older minors return False (CPU fallback). Higher
minors are accepted under the semver compatibility contract but emit a
loud warning, so any silent ABI break — the failure mode that motivated
this verification — gets flagged early rather than miscalled.
Only the exact version whose vtable and transform behavior have been
validated is accepted. Any mismatch returns False so the caller uses
the safe CPU hierarchy fallback.
"""
get_desc_addr = _read_u64(fw_ptr + _FW_OFF_GET_INTERFACE_PLUGIN_DESC)
if get_desc_addr == 0:
Expand Down Expand Up @@ -288,7 +287,7 @@ def _verify_iadapter_version(fw_ptr: int, ia_ptr: int) -> bool:
continue
major = ctypes.c_uint32.from_address(entry_addr + 8).value
minor = ctypes.c_uint32.from_address(entry_addr + 12).value
if major != _IA_EXPECTED_MAJOR or minor < _IA_EXPECTED_MINOR:
if major != _IA_EXPECTED_MAJOR or minor != _IA_EXPECTED_MINOR:
logger.warning(
"cubric IAdapter version incompatible with this shim: plugin "
"reports v%d.%d, shim is pinned to v%d.%d. Falling back to "
Expand All @@ -299,17 +298,6 @@ def _verify_iadapter_version(fw_ptr: int, ia_ptr: int) -> bool:
_IA_EXPECTED_MINOR,
)
return False
if minor > _IA_EXPECTED_MINOR:
logger.warning(
"cubric IAdapter minor version newer than this shim was validated "
"against: plugin reports v%d.%d, shim is pinned to v%d.%d. Proceeding "
"under semver minor-compatibility — if transforms misbehave, verify "
"the vtable layout against omni/cubric/IAdapter.h.",
major,
minor,
_IA_EXPECTED_MAJOR,
_IA_EXPECTED_MINOR,
)
return True

logger.warning(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1294,7 +1294,9 @@ def instantiate_builder_from_stage(cls):
else:
# Load everything except the env subtrees (ground plane, lights, etc.)
ignore_paths = [path for _, path in env_paths]
builder.add_usd(stage, ignore_paths=ignore_paths, schema_resolvers=schema_resolvers)
from isaaclab_newton.cloner.newton_clone_utils import _add_global_stage_to_builder

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.

P2 Inline import of private cross-module function

_add_global_stage_to_builder is now used in three separate modules (replicate.py, visualization_builder.py, and here), which gives it de-facto public cross-module status. The leading underscore and this inline from import are inconsistent with that reach. Moving the import to the top-level imports of newton_manager.py (alongside the other newton_clone_utils imports already used in the file) would make the dependency explicit, and promoting the function to a non-underscore name would align its visibility with its actual API surface.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


_add_global_stage_to_builder(builder, stage, ignore_paths, schema_resolvers)
replace_newton_builder_shape_colors(builder, stage)

_, proto_path = env_paths[0]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from isaaclab.sim.utils.transforms import resolve_prim_pose

from isaaclab_newton.cloner.newton_clone_utils import (
_add_global_stage_to_builder,
build_source_builders,
rename_builder_labels,
replicate_builder_mapping,
Expand All @@ -43,7 +44,7 @@ def build_visualization_builder_from_stage_envs(
quaternions = torch.tensor([quat for _, quat in poses], dtype=torch.float32)
schema_resolvers = [SchemaResolverNewton(), SchemaResolverPhysx()]
builder = ModelBuilder(up_axis=up_axis)
builder.add_usd(stage, ignore_paths=["/World/envs", *sources], schema_resolvers=schema_resolvers)
_add_global_stage_to_builder(builder, stage, ["/World/envs", *sources], schema_resolvers)
source_builders = build_source_builders(
stage,
sources,
Expand Down

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.

I think this is also in another PRs. Worth checking out like the solver coupling PR. https://github.com/isaac-sim/IsaacLab/pull/5834/changes#diff-1b68f4d0395901f033293c2fbb2f489b98135587b9e94bbe7fb560bff647498f

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it's different context though in the same file.

Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@
from isaaclab_newton.cloner import newton_clone_utils as newton_clone_utils_module
from isaaclab_newton.cloner.newton_clone_utils import (
_BUILTIN_LABEL_TYPES,
_add_global_stage_to_builder,
build_source_builders,
rename_builder_labels,
replicate_builder_mapping,
)
from isaaclab_newton.physics import visualization_builder as visualization_builder_module
from newton.solvers import SolverMuJoCo

from pxr import Usd, UsdGeom
from pxr import Sdf, Usd, UsdGeom, UsdPhysics, Vt

from isaaclab.cloner import ClonePlan

Expand All @@ -44,6 +46,7 @@ def __init__(self, up_axis=None):
setattr(self, attr, [])
setattr(self, attr.replace("_label", "_world"), [])
self.custom_attributes = {}
self.custom_frequencies = {}
self.geometry_sources = []
self.world_slices = []
self._current_world = None
Expand All @@ -59,6 +62,9 @@ def begin_world(self):
def end_world(self):
self._current_world = None

def add_custom_frequency(self, frequency):
self.custom_frequencies[frequency.key] = frequency

def add_usd(self, stage, root_path=None, ignore_paths=None, schema_resolvers=None, **kwargs):
del stage, ignore_paths, schema_resolvers, kwargs
if root_path is None:
Expand Down Expand Up @@ -301,6 +307,85 @@ def test_inactive_source_rows_are_ignored(self):
self.assertEqual(builder.geometry_sources_for_world(1), [])


class TestIgnoredCloneCustomFrequencies(unittest.TestCase):
@staticmethod
def _define_robot(stage: Usd.Stage, root_path: str) -> None:
base = UsdGeom.Xform.Define(stage, f"{root_path}/base").GetPrim()
link = UsdGeom.Xform.Define(stage, f"{root_path}/link").GetPrim()
UsdPhysics.RigidBodyAPI.Apply(base)
UsdPhysics.RigidBodyAPI.Apply(link)
UsdPhysics.ArticulationRootAPI.Apply(base)

joint_path = f"{root_path}/joint"
joint = UsdPhysics.RevoluteJoint.Define(stage, joint_path)
joint.CreateAxisAttr().Set("Z")
joint.CreateBody0Rel().SetTargets([Sdf.Path(f"{root_path}/base")])
joint.CreateBody1Rel().SetTargets([Sdf.Path(f"{root_path}/link")])

tendon = stage.DefinePrim(f"{root_path}/tendon", "MjcTendon")
tendon.CreateAttribute("mjc:type", Sdf.ValueTypeNames.Token, True).Set("fixed")
tendon.CreateRelationship("mjc:path", True).SetTargets([Sdf.Path(joint_path)])
tendon.CreateAttribute("mjc:path:indices", Sdf.ValueTypeNames.IntArray, True).Set(Vt.IntArray([0]))
tendon.CreateAttribute("mjc:path:coef", Sdf.ValueTypeNames.DoubleArray, True).Set(Vt.DoubleArray([1.0]))

def test_global_import_does_not_create_tendon_rows_from_ignored_envs(self):
stage = Usd.Stage.CreateInMemory()
for env_id in range(2):
self._define_robot(stage, f"/World/envs/env_{env_id}/Robot")
self._define_robot(stage, "/World/global/Robot")

builder = newton.ModelBuilder()
_add_global_stage_to_builder(builder, stage, ["/World/envs"], [])
self.assertEqual(builder._custom_frequency_counts["mujoco:tendon"], 1)
self.assertEqual(builder._custom_frequency_counts["mujoco:tendon_joint"], 1)
self.assertNotIn("add_custom_frequency", vars(builder))

reference_builder = newton.ModelBuilder()
SolverMuJoCo.register_custom_attributes(reference_builder)
for frequency_key, reference_frequency in reference_builder.custom_frequencies.items():
self.assertIs(
builder.custom_frequencies[frequency_key].usd_prim_filter,
reference_frequency.usd_prim_filter,
)

source_path = "/World/envs/env_0/Robot"
source_builders = build_source_builders(
stage,
[source_path],
newton.ModelBuilder,
[],
simplify_meshes=False,
)
replicate_builder_mapping(
builder,
[source_path],
torch.ones((1, 2), dtype=torch.bool),
torch.zeros((2, 3)),
torch.tensor([[0.0, 0.0, 0.0, 1.0], [0.0, 0.0, 0.0, 1.0]]),
source_builders,
)

self.assertEqual(source_builders[source_path]._custom_frequency_counts["mujoco:tendon"], 1)
self.assertEqual(builder._custom_frequency_counts["mujoco:tendon"], 3)
self.assertEqual(builder._custom_frequency_counts["mujoco:tendon_joint"], 3)

def test_global_import_restores_custom_frequencies_when_import_fails(self):
stage = Usd.Stage.CreateInMemory()
builder = newton.ModelBuilder()
SolverMuJoCo.register_custom_attributes(builder)
original_filters = {
frequency_key: frequency.usd_prim_filter for frequency_key, frequency in builder.custom_frequencies.items()
}

with mock.patch.object(builder, "add_usd", side_effect=RuntimeError("expected import failure")):
with self.assertRaisesRegex(RuntimeError, "expected import failure"):
_add_global_stage_to_builder(builder, stage, ["/World/envs"], [])

self.assertNotIn("add_custom_frequency", vars(builder))
for frequency_key, callback in original_filters.items():
self.assertIs(builder.custom_frequencies[frequency_key].usd_prim_filter, callback)


class TestVisualizationClonePlan(unittest.TestCase):
@staticmethod
def _define_xform(stage, path, translation=None):
Expand Down
34 changes: 34 additions & 0 deletions source/isaaclab_newton/test/physics/test_cubric.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md).
# All rights reserved.
#
# SPDX-License-Identifier: BSD-3-Clause

import ctypes

from isaaclab_newton.physics._cubric import CubricBindings


def _verify_version(major: int, minor: int) -> bool:
"""Run the cubric ABI verifier against an in-memory plugin descriptor."""
interface_name = ctypes.create_string_buffer(b"omni::cubric::IAdapter\0")
interface = (ctypes.c_uint64 * 2)()
interface[0] = ctypes.addressof(interface_name)
ctypes.c_uint32.from_address(ctypes.addressof(interface) + 8).value = major
ctypes.c_uint32.from_address(ctypes.addressof(interface) + 12).value = minor

plugin_descriptor = (ctypes.c_ubyte * 56)()
ctypes.c_uint64.from_address(ctypes.addressof(plugin_descriptor) + 40).value = ctypes.addressof(interface)
ctypes.c_uint64.from_address(ctypes.addressof(plugin_descriptor) + 48).value = 1

get_descriptor_type = ctypes.CFUNCTYPE(ctypes.c_void_p, ctypes.c_void_p)
get_descriptor = get_descriptor_type(lambda _: ctypes.addressof(plugin_descriptor))
framework = (ctypes.c_uint64 * 13)()
framework[12] = ctypes.cast(get_descriptor, ctypes.c_void_p).value

return CubricBindings._verify_iadapter_version(ctypes.addressof(framework), 1)


def test_cubric_adapter_rejects_unvalidated_minor_version():
"""A newer minor ABI must use the safe CPU transform-hierarchy fallback."""
assert _verify_version(0, 1)
assert not _verify_version(0, 2)
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fixed
^^^^^

* Fixed Newton visualization marker cleanup during interpreter shutdown.
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,16 @@ def __init__(self, cfg: VisualizationMarkersCfg, visible: bool = True):
}

sim = sim_utils.SimulationContext.instance()
if sim is not None:
sim.vis_marker_registry.set_group(self.group_id, self)
self._registry = sim.vis_marker_registry if sim is not None else None
if self._registry is not None:
self._registry.set_group(self.group_id, self)

def close(self) -> None:
"""Remove marker backend from the simulation marker registry."""
sim = sim_utils.SimulationContext.instance()
if sim is not None:
sim.vis_marker_registry.remove_group(self.group_id)
registry = getattr(self, "_registry", None)
self._registry = None
if registry is not None:
registry.remove_group(self.group_id)

def infer_device(self) -> torch.device:
"""Infer the device from current marker state."""
Expand Down
Loading
Loading