diff --git a/source/isaaclab_contrib/changelog.d/pr5834-review-fixes.rst b/source/isaaclab_contrib/changelog.d/pr5834-review-fixes.rst index 51b2fe60d13c..26afadab11e7 100644 --- a/source/isaaclab_contrib/changelog.d/pr5834-review-fixes.rst +++ b/source/isaaclab_contrib/changelog.d/pr5834-review-fixes.rst @@ -1,9 +1,7 @@ Fixed ^^^^^ -* Fixed :class:`~isaaclab_contrib.coupling.CouplerCfg` to reject invalid - ownership, references, nested configs, and manager-only solver options at - construction instead of silently building an incomplete coupled solver. +* Fixed :class:`~isaaclab_contrib.coupling.CouplerCfg` to reject inactive + entries and unsupported nested solver configurations. -* Fixed proxy collision fallback, MPM contact and graph-capture policy, and - Proxy/ADMM config forwarding to preserve the pinned Newton solver contract. +* Fixed proxy collision fallback and MPM contact and graph-capture policy. diff --git a/source/isaaclab_contrib/isaaclab_contrib/coupling/coupler.py b/source/isaaclab_contrib/isaaclab_contrib/coupling/coupler.py index d80cc8be8746..c8508891f7b7 100644 --- a/source/isaaclab_contrib/isaaclab_contrib/coupling/coupler.py +++ b/source/isaaclab_contrib/isaaclab_contrib/coupling/coupler.py @@ -7,10 +7,7 @@ from __future__ import annotations -import logging -from collections import Counter -from dataclasses import dataclass, fields -from functools import wraps +from dataclasses import dataclass from typing import TYPE_CHECKING from isaaclab_newton.physics import ( @@ -41,9 +38,6 @@ from isaaclab.scene import InteractiveSceneCfg -logger = logging.getLogger(__name__) - - class NewtonCouplerManager(NewtonVBDManager): """Couple named Newton solver entries through proxy or ADMM interfaces.""" @@ -90,27 +84,35 @@ def _build_solver(cls, model: Model, solver_cfg: CouplerCfg) -> None: scene_cfg = solver_cfg.scene_cfg resolved_entries = [cls._resolve_entry(model, entry, scene_cfg) for entry in solver_cfg.entries] - cls._validate_resolved_entries(model, resolved_entries, solver_cfg) + proxies: list[CouplerProxyMappingCfg] = [] + active_proxy_destinations: set[str] = set() + if isinstance(solver_cfg, CouplerProxyCfg): + proxies = [cls._resolve_proxy(model, proxy, scene_cfg) for proxy in solver_cfg.proxies] + active_proxy_destinations = {proxy.destination for proxy in proxies if proxy.bodies or proxy.particles} + cls._validate_resolved_entries(model, resolved_entries, solver_cfg, active_proxy_destinations) entries = [cls._build_entry(entry) for entry in resolved_entries] if isinstance(solver_cfg, CouplerProxyCfg): - proxies = [cls._resolve_proxy(model, proxy, scene_cfg) for proxy in solver_cfg.proxies] - NewtonManager._solver = cls._build_proxy_coupled_solver(model, entries, proxies, solver_cfg) - proxy_destinations = {proxy.destination for proxy in proxies} - outer_contact_destinations = {proxy.destination for proxy in proxies if proxy.collision_pipeline is None} + solver = cls._build_proxy_coupled_solver(model, entries, proxies, solver_cfg) + directions = {(proxy.source, proxy.destination) for proxy in proxies} + proxy_destinations = {destination for _, destination in directions} + outer_contact_entries = { + entry.config.name for entry in resolved_entries if entry.config.name not in proxy_destinations + } + outer_contact_entries.update(source for source, _ in directions) + outer_contact_entries.update( + destination + for source, destination in directions + if solver.get_proxy_contacts(source, destination) is None + ) + NewtonManager._solver = solver needs_collision_pipeline = any( - (entry.config.name not in proxy_destinations or entry.config.name in outer_contact_destinations) - and cls._requires_external_contacts(entry.config.solver_cfg) + entry.config.name in outer_contact_entries and cls._requires_external_contacts(entry.config.solver_cfg) for entry in resolved_entries ) - elif isinstance(solver_cfg, CouplerAdmmCfg): + else: NewtonManager._solver = cls._build_admm_coupled_solver(model, entries, solver_cfg) needs_collision_pipeline = True - else: - raise TypeError( - f"CouplerCfg subclass {type(solver_cfg).__name__!r} is not supported; " - "use CouplerProxyCfg or CouplerAdmmCfg." - ) NewtonManager._use_single_state = False NewtonManager._supports_contact_sensors = False @@ -121,7 +123,7 @@ def _build_solver(cls, model: Model, solver_cfg: CouplerCfg) -> None: @classmethod def _validate_config(cls, solver_cfg: CouplerCfg) -> None: - """Validate references and unsupported nested-manager behavior before construction.""" + """Validate adapter-specific nested-manager constraints before construction.""" if not isinstance(solver_cfg, (CouplerProxyCfg, CouplerAdmmCfg)): raise TypeError( f"CouplerCfg subclass {type(solver_cfg).__name__!r} is not supported; " @@ -130,13 +132,8 @@ def _validate_config(cls, solver_cfg: CouplerCfg) -> None: if not solver_cfg.entries: raise ValueError("CouplerCfg.entries must contain at least one solver entry.") - names = [entry.name for entry in solver_cfg.entries] - if any(not isinstance(name, str) or not name for name in names): + if any(not isinstance(entry.name, str) or not entry.name for entry in solver_cfg.entries): raise ValueError("CouplerCfg entry names must be non-empty strings.") - duplicate_names = sorted(name for name, count in Counter(names).items() if count > 1) - if duplicate_names: - raise ValueError(f"CouplerCfg entry names must be unique; duplicates: {duplicate_names}.") - name_set = set(names) for entry in solver_cfg.entries: nested_cfg = entry.solver_cfg @@ -179,114 +176,24 @@ def _validate_config(cls, solver_cfg: CouplerCfg) -> None: "state cannot yet preserve the manager's per-world reset-mask lifecycle inside a coupled entry." ) - if isinstance(solver_cfg, CouplerProxyCfg): - if len(solver_cfg.entries) > 2: - raise ValueError("CouplerProxyCfg supports at most two solver entries.") - for proxy in solver_cfg.proxies: - cls._validate_entry_reference(name_set, proxy.source, "proxy source") - cls._validate_entry_reference(name_set, proxy.destination, "proxy destination") - if proxy.source == proxy.destination: - raise ValueError( - f"CouplerProxyMappingCfg source and destination must differ, got {proxy.source!r}." - ) - if proxy.collision_pipeline is not None and not callable(proxy.collision_pipeline): - raise TypeError("CouplerProxyMappingCfg.collision_pipeline must be callable or None.") - if proxy.collision_pipeline is None and proxy.collide_interval is not None: - raise ValueError( - "CouplerProxyMappingCfg.collide_interval requires a proxy-local collision_pipeline." - ) - else: - if solver_cfg.contact_pairs is not None: - for source, destination in solver_cfg.contact_pairs: - cls._validate_entry_reference(name_set, source, "ADMM contact-pair source") - cls._validate_entry_reference(name_set, destination, "ADMM contact-pair destination") - if source == destination: - raise ValueError(f"ADMM contact-pair entries must differ, got {source!r}.") - if solver_cfg.joint_proximal_destination_entries is not None: - for name in solver_cfg.joint_proximal_destination_entries: - cls._validate_entry_reference(name_set, name, "ADMM joint-proximal destination") - - @staticmethod - def _validate_entry_reference(entry_names: set[str], name: str, field: str) -> None: - """Raise when a coupling reference does not name a configured entry.""" - if name not in entry_names: - raise ValueError(f"CouplerCfg {field} {name!r} is not one of the configured entries {sorted(entry_names)}.") - @classmethod def _validate_resolved_entries( cls, model: Model, entries: list[_ResolvedEntry], solver_cfg: CouplerCfg, + active_proxy_destinations: set[str] | None = None, ) -> None: - """Validate resolved ownership and report intentionally unassigned model elements.""" + """Reject entries that neither own nor receive model elements.""" + active_proxy_destinations = active_proxy_destinations or set() for entry in entries: - counts = (len(entry.bodies), len(entry.particles), len(entry.joints), len(entry.shapes)) - if not any(counts): - raise ValueError(f"CouplerEntryCfg {entry.config.name!r} owns no bodies, particles, joints, or shapes.") - logger.info( - "[COUPLER] Entry %r owns %d bodies, %d particles, %d joints, and %d shapes.", - entry.config.name, - *counts, - ) - - owners = { - "bodies": cls._build_ownership_map(model.body_count, entries, "bodies"), - "particles": cls._build_ownership_map(model.particle_count, entries, "particles"), - "joints": cls._build_ownership_map(model.joint_count, entries, "joints"), - "shapes": cls._build_ownership_map(model.shape_count, entries, "shapes"), - } - cls._validate_partial_joint_ownership(model, owners["bodies"]) - - unassigned = {name: sum(owner is None for owner in values) for name, values in owners.items()} - logger.info( - "[COUPLER] Unassigned model elements: %d bodies, %d particles, %d joints, and %d shapes. " - "Unassigned elements remain outside nested solver views.", - unassigned["bodies"], - unassigned["particles"], - unassigned["joints"], - unassigned["shapes"], - ) + owns_any = any((entry.bodies, entry.particles, entry.joints, entry.shapes)) + if not owns_any and entry.config.name not in active_proxy_destinations: + raise ValueError(f"CouplerEntryCfg {entry.config.name!r} neither owns nor receives any model elements.") if isinstance(solver_cfg, CouplerProxyCfg): cls._validate_no_cross_entry_proxy_joints(model, {entry.config.name: entry for entry in entries}) - @staticmethod - def _build_ownership_map(count: int, entries: list[_ResolvedEntry], field: str) -> list[str | None]: - """Build one ownership map while validating ranges and overlap.""" - owners: list[str | None] = [None] * int(count) - for entry in entries: - for raw_index in getattr(entry, field): - index = int(raw_index) - if not 0 <= index < count: - raise ValueError( - f"CouplerEntryCfg {entry.config.name!r} {field} index {index} is out of range [0, {count})." - ) - if owners[index] is not None: - raise ValueError( - f"CouplerCfg {field} index {index} is owned by both {owners[index]!r} " - f"and {entry.config.name!r}." - ) - owners[index] = entry.config.name - return owners - - @staticmethod - def _validate_partial_joint_ownership(model: Model, body_owners: list[str | None]) -> None: - """Reject articulation joints with exactly one endpoint assigned to an entry.""" - for joint, (parent_raw, child_raw) in enumerate(zip(model.joint_parent.numpy(), model.joint_child.numpy())): - parent = int(parent_raw) - child = int(child_raw) - if parent < 0: - continue - parent_owner = body_owners[parent] - child_owner = body_owners[child] - if (parent_owner is None) != (child_owner is None): - raise ValueError( - f"CouplerCfg joint {joint} has only one owned endpoint: parent body {parent} is owned by " - f"{parent_owner!r}, child body {child} is owned by {child_owner!r}. Assign both articulation " - "bodies or leave both unassigned." - ) - @classmethod def _register_builder_attributes(cls, builder: ModelBuilder) -> None: """Register custom attributes required by nested coupled entries.""" @@ -511,33 +418,8 @@ def _build_proxy_coupled_solver( proxy_cfgs: list[CouplerProxyMappingCfg], solver_cfg: CouplerProxyCfg, ) -> SolverCoupledProxy: - proxies = [] - checked_factories: list[tuple[str, str, object, object]] = [] - for proxy_cfg in proxy_cfgs: - values = cls._checked_config_values(SolverCoupledProxy.Proxy, proxy_cfg) - factory = values.get("collision_pipeline") - if factory is not None: - checked_factory = next( - ( - checked - for source, destination, original, checked in checked_factories - if source == proxy_cfg.source and destination == proxy_cfg.destination and original is factory - ), - None, - ) - if checked_factory is None: - checked_factory = cls._checked_collision_pipeline_factory( - factory, proxy_cfg.source, proxy_cfg.destination - ) - checked_factories.append((proxy_cfg.source, proxy_cfg.destination, factory, checked_factory)) - values["collision_pipeline"] = checked_factory - proxies.append(SolverCoupledProxy.Proxy(**values)) - - coupling_values = cls._checked_config_values( - SolverCoupledProxy.Config, - solver_cfg, - handled_fields={"class_type", "solver_type", "model_cfg", "entries", "scene_cfg", "proxies"}, - ) + proxies = [SolverCoupledProxy.Proxy(**vars(proxy_cfg)) for proxy_cfg in proxy_cfgs] + coupling_values = cls._filter_solver_kwargs(SolverCoupledProxy.Config, solver_cfg) coupling_values["proxies"] = proxies coupling = SolverCoupledProxy.Config(**coupling_values) return SolverCoupledProxy(model=model, entries=entries, coupling=coupling) @@ -549,18 +431,7 @@ def _build_admm_coupled_solver( entries: list[SolverCoupled.Entry], solver_cfg: CouplerAdmmCfg, ) -> SolverCoupledADMM: - values = cls._checked_config_values( - SolverCoupledADMM.Config, - solver_cfg, - handled_fields={ - "class_type", - "solver_type", - "model_cfg", - "entries", - "scene_cfg", - "contact_pairs", - }, - ) + values = cls._filter_solver_kwargs(SolverCoupledADMM.Config, solver_cfg) if solver_cfg.contact_pairs is None: values["contact_pairs"] = SolverCoupledADMM.auto_detect_contact_pairs(entries) else: @@ -571,37 +442,6 @@ def _build_admm_coupled_solver( coupling = SolverCoupledADMM.Config(**values) return SolverCoupledADMM(model=model, entries=entries, coupling=coupling) - @staticmethod - def _checked_config_values(target_type: type, config, *, handled_fields: set[str] | None = None) -> dict: - """Forward every config field or fail loudly when the Newton schema drifts.""" - handled_fields = handled_fields or set() - target_fields = {field.name for field in fields(target_type)} - config_fields = {field.name for field in fields(config)} - unhandled = config_fields - target_fields - handled_fields - if unhandled: - raise TypeError( - f"Cannot forward {type(config).__name__} to {target_type.__qualname__}; " - f"unhandled fields: {sorted(unhandled)}." - ) - return {name: getattr(config, name) for name in config_fields & target_fields if name not in handled_fields} - - @staticmethod - def _checked_collision_pipeline_factory(factory, source: str, destination: str): - """Wrap a proxy-local collision factory so a silent outer-contact fallback is impossible.""" - - @wraps(factory) - def checked_factory(model_view): - pipeline = factory(model_view) - if pipeline is None: - raise TypeError( - "CouplerProxyMappingCfg collision_pipeline factory for " - f"{source!r}->{destination!r} returned None. Set collision_pipeline=None explicitly " - "to use the shared outer contacts." - ) - return pipeline - - return checked_factory - @staticmethod def _validate_no_cross_entry_proxy_joints(model: Model, entries: dict[str, _ResolvedEntry]) -> None: body_owner = {int(body): name for name, entry in entries.items() for body in entry.bodies} diff --git a/source/isaaclab_contrib/isaaclab_contrib/coupling/coupler_cfg.py b/source/isaaclab_contrib/isaaclab_contrib/coupling/coupler_cfg.py index 43beea42794d..a689b56dc2f9 100644 --- a/source/isaaclab_contrib/isaaclab_contrib/coupling/coupler_cfg.py +++ b/source/isaaclab_contrib/isaaclab_contrib/coupling/coupler_cfg.py @@ -66,9 +66,7 @@ class CouplerEntryCfg: """Whether fully selected child joints are owned by this entry. A joint is owned when its child body is selected and its parent is either - the world or selected by the same entry. A joint with exactly one selected - body endpoint is rejected because otherwise it would not be integrated by - any entry. + the world or selected by the same entry. """ include_body_shapes: bool = True @@ -127,15 +125,14 @@ class CouplerProxyMappingCfg: positive integers and require :attr:`collision_pipeline` to be a factory. """ - collision_pipeline: Callable[[ModelView], CollisionPipeline] | None = lambda model_view: CollisionPipeline( + collision_pipeline: Callable[[ModelView], CollisionPipeline | None] | None = lambda model_view: CollisionPipeline( model_view, broad_phase="explicit" ) """Factory for the proxy destination's collision pipeline, or ``None``. The default creates a proxy-local pipeline with ``broad_phase="explicit"``. - Set this field itself to ``None`` to pass the shared outer contacts to the - destination. A configured factory must return a collision pipeline; a - callable that returns ``None`` is rejected. + Setting the field or returning ``None`` from the factory passes shared + outer contacts to the destination. """ diff --git a/source/isaaclab_contrib/test/coupling/test_coupler.py b/source/isaaclab_contrib/test/coupling/test_coupler.py index 15a800bac5be..5dd9909fe384 100644 --- a/source/isaaclab_contrib/test/coupling/test_coupler.py +++ b/source/isaaclab_contrib/test/coupling/test_coupler.py @@ -122,6 +122,7 @@ def _entry( *, bodies: list[int] | None = None, particles: list[int] | None = None, + shapes: list[int] | None = None, ) -> NewtonCouplerManager._ResolvedEntry: """Build an already-resolved entry for validation tests.""" return NewtonCouplerManager._ResolvedEntry( @@ -129,7 +130,7 @@ def _entry( bodies=list(bodies or []), particles=list(particles or []), joints=[], - shapes=[], + shapes=list(shapes or []), ) @@ -167,17 +168,6 @@ def test_config_validation_requires_nonempty_entry_names(): NewtonCouplerManager._validate_config(cfg) -def test_config_validation_rejects_duplicate_entry_names(): - cfg = CouplerAdmmCfg( - entries=[ - CouplerEntryCfg(name="duplicate", solver_cfg=XPBDSolverCfg()), - CouplerEntryCfg(name="duplicate", solver_cfg=XPBDSolverCfg()), - ] - ) - with pytest.raises(ValueError, match="entry names must be unique"): - NewtonCouplerManager._validate_config(cfg) - - def test_config_validation_requires_newton_solver_config(): cfg = CouplerAdmmCfg( entries=[CouplerEntryCfg(name="entry", solver_cfg=object())] # type: ignore[arg-type] @@ -186,50 +176,6 @@ def test_config_validation_requires_newton_solver_config(): NewtonCouplerManager._validate_config(cfg) -@pytest.mark.parametrize( - ("cfg", "match"), - [ - ( - CouplerProxyCfg( - entries=[CouplerEntryCfg(name="a", solver_cfg=XPBDSolverCfg())], - proxies=[CouplerProxyMappingCfg(source="missing", destination="a")], - ), - "proxy source 'missing'", - ), - ( - CouplerProxyCfg( - entries=[CouplerEntryCfg(name="a", solver_cfg=XPBDSolverCfg())], - proxies=[CouplerProxyMappingCfg(source="a", destination="a")], - ), - "source and destination must differ", - ), - ( - CouplerAdmmCfg( - entries=[CouplerEntryCfg(name="a", solver_cfg=XPBDSolverCfg())], - contact_pairs=[("a", "missing")], - ), - "contact-pair destination 'missing'", - ), - ( - CouplerAdmmCfg( - entries=[CouplerEntryCfg(name="a", solver_cfg=XPBDSolverCfg())], - joint_proximal_destination_entries=["missing"], - ), - "joint-proximal destination 'missing'", - ), - ], -) -def test_config_validation_rejects_invalid_entry_references(cfg, match): - with pytest.raises(ValueError, match=match): - NewtonCouplerManager._validate_config(cfg) - - -def test_proxy_config_validation_rejects_more_than_two_entries(): - cfg = CouplerProxyCfg(entries=[CouplerEntryCfg(name=name, solver_cfg=XPBDSolverCfg()) for name in ("a", "b", "c")]) - with pytest.raises(ValueError, match="at most two"): - NewtonCouplerManager._validate_config(cfg) - - @pytest.mark.parametrize( ("solver_cfg", "entry_kwargs", "error_type", "match"), [ @@ -260,33 +206,6 @@ def test_config_validation_requires_concrete_nested_factory(): NewtonCouplerManager._validate_config(cfg) -def test_proxy_outer_contact_fallback_rejects_collision_interval(): - cfg, _, _ = _valid_proxy_setup( - proxy=CouplerProxyMappingCfg( - source="rigid", - destination="soft", - bodies=[1], - collision_pipeline=None, - collide_interval=2, - ) - ) - with pytest.raises(ValueError, match="requires a proxy-local collision_pipeline"): - NewtonCouplerManager._validate_config(cfg) - - -def test_proxy_config_validation_rejects_noncallable_collision_pipeline(): - cfg, _, _ = _valid_proxy_setup( - proxy=CouplerProxyMappingCfg( - source="rigid", - destination="soft", - bodies=[1], - collision_pipeline=1, # type: ignore[arg-type] - ) - ) - with pytest.raises(TypeError, match="must be callable or None"): - NewtonCouplerManager._validate_config(cfg) - - def test_scene_entity_and_string_selectors_resolve_full_body_labels(): """Scene selectors filter short names while strings match full labels.""" model = _FakeModel() @@ -455,8 +374,8 @@ def test_cross_entry_joint_is_left_unowned_for_admm_attachment(): assert resolved[1].joints == [] -def test_resolved_entry_validation_rejects_empty_ownership(): - with pytest.raises(ValueError, match="owns no bodies, particles, joints, or shapes"): +def test_resolved_entry_validation_rejects_inactive_entry(): + with pytest.raises(ValueError, match="neither owns nor receives any model elements"): NewtonCouplerManager._validate_resolved_entries( _FakeModel(), [_entry("empty")], @@ -464,42 +383,21 @@ def test_resolved_entry_validation_rejects_empty_ownership(): ) -@pytest.mark.parametrize("owned_body", [0, 1]) -def test_resolved_entry_validation_rejects_joint_with_one_owned_endpoint(owned_body): - model = _FakeModel() - model.joint_count = 1 - model.joint_parent = _FakeArray(np.asarray([0], dtype=np.int32)) - model.joint_child = _FakeArray(np.asarray([1], dtype=np.int32)) - with pytest.raises(ValueError, match="only one owned endpoint"): - NewtonCouplerManager._validate_resolved_entries( - model, - [_entry("partial", bodies=[owned_body])], - CouplerAdmmCfg(), - ) - - -def test_resolved_entry_validation_rejects_duplicate_ownership(): - with pytest.raises(ValueError, match="owned by both"): - NewtonCouplerManager._validate_resolved_entries( - _FakeModel(), - [_entry("a", bodies=[0]), _entry("b", bodies=[0])], - CouplerAdmmCfg(), - ) +def test_resolved_entry_validation_accepts_active_proxy_destination(): + NewtonCouplerManager._validate_resolved_entries( + _FakeModel(), + [_entry("source", bodies=[0]), _entry("destination")], + CouplerProxyCfg(), + {"destination"}, + ) -def test_resolved_entry_validation_logs_ownership_summary(caplog): - model = _FakeModel() - model.joint_count = 0 - model.joint_parent = _FakeArray(np.asarray([], dtype=np.int32)) - model.joint_child = _FakeArray(np.asarray([], dtype=np.int32)) - with caplog.at_level("INFO", logger=coupler.__name__): - NewtonCouplerManager._validate_resolved_entries( - model, - [_entry("partial", bodies=[0])], - CouplerAdmmCfg(), - ) - assert "Entry 'partial' owns 1 bodies" in caplog.text - assert "Unassigned model elements: 2 bodies" in caplog.text +def test_resolved_entry_validation_accepts_static_shape_ownership(): + NewtonCouplerManager._validate_resolved_entries( + _FakeModel(), + [_entry("ground", shapes=[3])], + CouplerAdmmCfg(), + ) def test_proxy_validation_rejects_cross_entry_joint(): @@ -594,54 +492,10 @@ def custom_pipeline(model_view): assert solver.coupling.proxies[0].mode == "staggered" assert solver.coupling.proxies[0].mass_scale == pytest.approx(0.25) assert solver.coupling.proxies[0].collide_interval == 4 - assert solver.coupling.proxies[0].collision_pipeline("custom-view") == "custom-view" + assert solver.coupling.proxies[0].collision_pipeline is custom_pipeline assert solver.coupling.proxies[1].collision_pipeline("soft-view") == ("soft-view", "explicit") -def test_proxy_collision_factory_returning_none_is_rejected(monkeypatch): - cfg, _, proxies = _valid_proxy_setup( - proxy=CouplerProxyMappingCfg( - source="rigid", - destination="soft", - bodies=[1], - collision_pipeline=lambda model_view: None, - ) - ) - monkeypatch.setattr(coupler, "SolverCoupledProxy", _RecordingProxy) - solver = NewtonCouplerManager._build_proxy_coupled_solver(_FakeModel(), [], proxies, cfg) - - with pytest.raises(TypeError, match="returned None"): - solver.coupling.proxies[0].collision_pipeline("soft-view") - - -def test_repeated_proxy_mappings_reuse_checked_collision_factory(monkeypatch): - """Newton requires one factory object for every mapping in the same direction.""" - - def collision_pipeline(model_view): - return model_view - - cfg = CouplerProxyCfg( - entries=[ - CouplerEntryCfg(name="rigid", solver_cfg=XPBDSolverCfg()), - CouplerEntryCfg(name="soft", solver_cfg=XPBDSolverCfg()), - ], - proxies=[ - CouplerProxyMappingCfg( - source="rigid", destination="soft", bodies=[0], collision_pipeline=collision_pipeline - ), - CouplerProxyMappingCfg( - source="rigid", destination="soft", particles=[0], collision_pipeline=collision_pipeline - ), - ], - ) - monkeypatch.setattr(coupler, "SolverCoupledProxy", _RecordingProxy) - - solver = NewtonCouplerManager._build_proxy_coupled_solver(_FakeModel(), [], cfg.proxies, cfg) - - first, second = solver.coupling.proxies - assert first.collision_pipeline is second.collision_pipeline - - def test_entry_build_uses_solver_config_class_type(): class _RecordingManager: @classmethod @@ -784,16 +638,20 @@ def test_contact_initialization_prepares_coupled_solver_buffers(monkeypatch): ("mjwarp_external", True, False), ("mpm", False, True), ("destination_fallback", True, False), + ("destination_factory_fallback", True, False), ], ) def test_proxy_selects_expected_outer_collision_pipeline(monkeypatch, case, expected_outer, expected_fk): """Proxy entries request shared contacts only when one of their solve paths consumes them.""" model = _FakeModel() - fallback_proxy = ( - CouplerProxyMappingCfg(source="rigid", destination="soft", bodies=[1], collision_pipeline=None) - if case == "destination_fallback" - else None - ) + if case == "destination_fallback": + fallback_proxy = CouplerProxyMappingCfg(source="rigid", destination="soft", bodies=[1], collision_pipeline=None) + elif case == "destination_factory_fallback": + fallback_proxy = CouplerProxyMappingCfg( + source="rigid", destination="soft", bodies=[1], collision_pipeline=lambda model_view: None + ) + else: + fallback_proxy = None proxy_cfg, resolved_entries, resolved_proxies = _valid_proxy_setup(proxy=fallback_proxy) proxy_cfg.scene_cfg = _FakeSceneCfg() recorded_entries: list[str] = [] @@ -842,7 +700,14 @@ def test_proxy_selects_expected_outer_collision_pipeline(monkeypatch, case, expe monkeypatch.setattr( NewtonCouplerManager, "_build_proxy_coupled_solver", - classmethod(lambda cls, model, entries, proxies, cfg: SimpleNamespace(kind="proxy")), + classmethod( + lambda cls, model, entries, proxies, cfg: SimpleNamespace( + kind="proxy", + get_proxy_contacts=lambda source, destination: ( + None if case in {"destination_fallback", "destination_factory_fallback"} else object() + ), + ) + ), ) NewtonCouplerManager._build_solver(model, proxy_cfg) @@ -980,27 +845,6 @@ def test_admm_build_forwards_multiple_pairs_matching_and_proximal_options(monkey assert solver.coupling.contact_matching_force_scale == pytest.approx(0.7) -def test_checked_forwarding_rejects_native_schema_drift(): - @dataclass - class _NativeConfigWithoutRho: - iterations: int = 1 - - cfg = CouplerAdmmCfg(entries=[CouplerEntryCfg(name="entry", solver_cfg=XPBDSolverCfg())]) - with pytest.raises(TypeError, match="unhandled fields:.*rho"): - NewtonCouplerManager._checked_config_values( - _NativeConfigWithoutRho, - cfg, - handled_fields={ - "class_type", - "solver_type", - "model_cfg", - "entries", - "scene_cfg", - "contact_pairs", - }, - ) - - def test_admm_build_auto_detects_symmetric_contact_pairs_by_default(monkeypatch): model = _FakeModel() resolved_entries = [ diff --git a/source/isaaclab_contrib/test/coupling/test_coupler_runtime.py b/source/isaaclab_contrib/test/coupling/test_coupler_runtime.py index eb62b3720fc8..0ca2a79fb515 100644 --- a/source/isaaclab_contrib/test/coupling/test_coupler_runtime.py +++ b/source/isaaclab_contrib/test/coupling/test_coupler_runtime.py @@ -77,6 +77,32 @@ def _entry_configs() -> list[CouplerEntryCfg]: ] +def test_proxy_destination_can_receive_only_proxy_bodies(isolated_newton_manager): + model = _build_overlapping_body_model() + solver_cfg = CouplerProxyCfg( + entries=[ + CouplerEntryCfg( + name="source", + solver_cfg=XPBDSolverCfg(iterations=2), + bodies=[r"/World/Source/body"], + ), + CouplerEntryCfg(name="destination", solver_cfg=XPBDSolverCfg(iterations=2)), + ], + proxies=[ + CouplerProxyMappingCfg( + source="source", + destination="destination", + bodies=[r"/World/Source/body"], + ) + ], + ) + + NewtonManager._model = model + NewtonCouplerManager._build_solver(model, solver_cfg) + + assert NewtonManager._solver._entries["destination"].proxy_body_local_indices.numpy().tolist() == [0] + + @pytest.mark.parametrize( ("algorithm", "expected_solver_type"), [ diff --git a/source/isaaclab_newton/changelog.d/feat-proxycoupledsolver.rst b/source/isaaclab_newton/changelog.d/feat-proxycoupledsolver.rst index cd8e03d951f2..585f0864c5f4 100644 --- a/source/isaaclab_newton/changelog.d/feat-proxycoupledsolver.rst +++ b/source/isaaclab_newton/changelog.d/feat-proxycoupledsolver.rst @@ -1,5 +1,10 @@ Changed ^^^^^^^ -* Made Newton solver construction reusable by manager subclasses, enabling - coupled solvers to construct nested entries from their typed solver configs. +* Extracted the repeated solver-kwargs filtering pattern from + :class:`~isaaclab_newton.physics.NewtonFeatherstoneManager`, + :class:`~isaaclab_newton.physics.NewtonMJWarpManager`, and + :class:`~isaaclab_newton.physics.NewtonXPBDManager` into a shared + :meth:`~isaaclab_newton.physics.NewtonManager._filter_solver_kwargs` helper, + so :class:`NewtonManager` subclasses can reuse it when forwarding + ``solver_cfg`` fields to a Newton solver constructor. diff --git a/source/isaaclab_tasks/changelog.d/feat-proxycoupledsolver.minor.rst b/source/isaaclab_tasks/changelog.d/feat-proxycoupledsolver.minor.rst index 8004334bf468..56774b00ffb5 100644 --- a/source/isaaclab_tasks/changelog.d/feat-proxycoupledsolver.minor.rst +++ b/source/isaaclab_tasks/changelog.d/feat-proxycoupledsolver.minor.rst @@ -1,6 +1,18 @@ Changed ^^^^^^^ +* **Breaking:** Renamed the deformable scene entity and its MDP terms in + ``lift_franka_soft`` from ``deformable`` to ``object`` to align with the + rigid lift task. Affects ``Isaac-Lift-Soft-Franka-v0`` and the cloth + variant: scene entry ``scene.deformable`` -> ``scene.object``, command + ``deformable_pose`` -> ``object_pose``, and MDP functions + ``deformable_ee_distance``, ``deformable_lifted``, + ``deformable_com_goal_distance``, ``deformable_com_in_robot_root_frame``, + ``deformable_com_below_minimum``, ``deformable_outside_table_bounds``, + ``DeformableSampledPointsInRobotRootFrame`` -> ``object_*`` / + ``ObjectSampledPointsInRobotRootFrame``. Update env configs, checkpoints, + and RL configs accordingly. + * Moved the rigid-shape contact defaults in the ``lift_franka_soft`` presets from ``NewtonModelCfg.shape_material_ke/kd/mu`` to :class:`~isaaclab_newton.physics.NewtonShapeCfg` on