Uses effort_limit from USD if not specified in actuator cfg

[JuanaDd]

Description

This PR fixes a bug in actuator initialization where effort limits specified in USD assets were being incorrectly overridden with a very large default value (1.0e9) for explicit actuator models.

Fixes # (issue)

Previously, the ActuatorBase initialization logic would unconditionally fall back to _DEFAULT_MAX_EFFORT_SIM (1.0e9) for explicit actuator models when effort_limit_sim was not explicitly set in the configuration, even when the USD asset contained finite, meaningful effort limit values.

Type of change

• Bug fix (non-breaking change which fixes an issue)

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

[Mayankm96]

Modifying the code where we set effort_limit might be simpler i think

        # For explicit models, if user does not provide effort limit, we want to default to USD values.
        # For implicit models, we want to enforce the effort limit through the solver.
        if not self.is_implicit_model and self.cfg.effort_limit_sim is None:
            self.effort_limit = self._parse_joint_parameter(self.cfg.effort_limit, effort_limit)
        else:
            self.effort_limit = self._parse_joint_parameter(self.cfg.effort_limit, self.effort_limit_sim)

[ooctipus]

I think this is a bit confusing, I reasoned about several time to understand why I was confused XD

I think this is due to the fact that effort_limit from USD is unclear if author who wrote the value intended to be motor maximum or simulation solver maximum:

• I guess @JuanaDd is thinking that if effort_limit from USD has a solver limit maximum value, then we shouldn't just override it with _DEFAULT_MAX_EFFORT_SIM (1.0e9), disrespecting USD's solver maximum value.

• @Mayankm96 in your reply, I guess you are thinking effort_limit from USD may be a motor limit and thus
  # For explicit models, if user does not provide effort limit, we want to default to USD values. I think the else section (implicit part) is not needed because the later code will do exactly else logic.

I think it is more safe to assume effort_limit this is simulation solver maximum since it will end up behaving like that.

[JuanaDd]

Hi @ooctipus thanks for your comment. My understanding is that, effort_limit is used for internal clipping in Isaaclab's explicit actuators but effort_limit_sim is passed to PhysX as constraints on joint efforts.

The isssue is that the USD defaults are not set to the effort_limit when using explicit actuators. Instead it is set to 1e9 in this line

[ooctipus]

Let me clarify two different usage of effort_limit here

-1.ActuatorCfg.effort_limit is used for internal clipping in Isaaclab's explicit actuators

-2. effort_limit below is actually USD's default, (we should probably rename it XD)

    def __init__(
        self,
        cfg: ActuatorBaseCfg,
        joint_names: list[str],
        joint_ids: slice | torch.Tensor,
        num_envs: int,
        device: str,
        stiffness: torch.Tensor | float = 0.0,
        damping: torch.Tensor | float = 0.0,
        armature: torch.Tensor | float = 0.0,
        friction: torch.Tensor | float = 0.0,
        dynamic_friction: torch.Tensor | float = 0.0,
        viscous_friction: torch.Tensor | float = 0.0,
        effort_limit: torch.Tensor | float = torch.inf,
        velocity_limit: torch.Tensor | float = torch.inf,
    ):

TLDR: Treat USD’s effort_limit as the solver/physics limit (effort_limit_sim) and never silently overwrite it. Motor-level clipping stays in ActuatorCfg.effort_limit. If someone wants “effectively unbounded,” they must set ActuatorCfg.effort_limit_sim = 1e9 (or similar) explicitly in config—not via hidden defaults.

Longer:
It is unclear USD's effort_limit is meant to be motor_level limit or Simulation solver limit, it could be either depending on who provided USD.

If the USD's effort_limit is Simulation solver limit, then overriding it to _DEFAULT_MAX_EFFORT_SIM (1.0e9) for no reason is wrong. This is also what you are trying to fix if I understand correctly. (Note you are comparing USD's effort_limit with ActuatorCfg's effort_limit_sim )

If the USD's effort_limit is motor_level limit, then then overriding it to _DEFAULT_MAX_EFFORT_SIM (1.0e9) actually make sense! which sounds like this is the reason why the overriding line was designed, and also why @Mayankm96 later proposed, # For explicit models, if user does not provide effort limit, we want to default to USD values. (Note here is comparing USD's effort_limit with ActuatorCfg's effort_limit )

What I am saying is that USD's effort_limit is AMBIGUOUS, but if we have to resolve on one definition, I think treating USD's effort_limit is Simulation solver limit makes sense, because it is what eventually will behave like in simulation. That is to also say, we shouldn't do any smart auto-overriding anymore, if explicit actuator wants very very large effort_limit_sim, we should always write that value out in the ActuatorCfg(Refer my solution in later comment).

[ooctipus]

@jtigue-bdai also take a look at this discussion and let us know what you think :))

[JuanaDd]

@Mayankm96 clone is required because effort_limit would be overwritten to effort_limit_sim again here

[ooctipus]

I will propose my solution to this @Mayankm96 @JuanaDd let me know what you think.

1. Regarding effort_limit from USD as effort_limit_sim, not motor level limit,
2. Deleting below block so no more assumption about -> explicit actuator should have _DEFAULT_MAX_EFFORT_SIM if ActuatorCfg.effort_limit_sim not specified. We want to use USD default instead for that case.

        # For explicit models, we do not want to enforce the effort limit through the solver
        # (unless it is explicitly set)
        if not self.is_implicit_model and self.cfg.effort_limit_sim is None:
            self.cfg.effort_limit_sim = self._DEFAULT_MAX_EFFORT_SIM

3. for all places we want _DEFAULT_MAX_EFFORT_SIM as effort_limit_sim we mannually define them in ActuatorCfg

This we have the greatest clarity from ActuatorCfg and also minimum code path(carity) for USD-Cfg resolution

[jtigue-bdai]

@JuanaDd Thanks for looking into this. I agree with the problem. The effort limit values of the USD articulation (and the USD created from urdf importers) should be passed through as effort_limit.

I think it would be good to add more of a note to the documentation on effort_limit and effort_limit_sim explaining the pass through.

[greptile-apps]

Greptile Overview

Greptile Summary

This PR addresses a bug in actuator initialization where USD-specified effort limits were being incorrectly overridden by a very large default value (1.0e9) for explicit actuator models. The fix implements a more nuanced resolution strategy: when effort_limit is not specified in the actuator configuration, explicit actuators now preserve the meaningful effort limits from the USD asset, while implicit actuators continue to enforce limits through the solver. The change modifies the ActuatorBase initialization logic to conditionally select default values based on whether the actuator model is explicit and whether effort_limit_sim is provided in the configuration. Additionally, the articulation.py file now clones effort limit tensors before passing them to actuators, preventing unintended modifications to the original USD data. Comprehensive test coverage was added to validate the three critical scenarios: config matching USD, config overriding USD, and defaulting to USD when config is None.

PR Description Notes:

• The PR description mentions that the fix allows the ActuatorBase to "fall back" to USD values, but the implementation actually makes explicit actuators use USD values as the primary default when no config is provided, which is a semantic distinction worth noting.

Important Files Changed

Filename	Score	Overview
source/isaaclab/isaaclab/actuators/actuator_base.py	5/5	Modified effort limit initialization to use USD values as default for explicit actuators when not specified in config
source/isaaclab/isaaclab/assets/articulation/articulation.py	5/5	Added .clone() to effort limit tensor to prevent unintended modifications to original USD data
source/isaaclab/test/assets/test_articulation.py	5/5	Added comprehensive test coverage for three effort limit scenarios with proper assertions

Confidence score: 5/5

• This PR is safe to merge with minimal risk as it addresses a clear bug with a well-structured fix
• Score reflects thorough testing, clear logic improvements, and proper separation of concerns between explicit and implicit actuator models
• No files require special attention; all changes are well-tested and the logic correctly distinguishes between explicit and implicit actuator handling

Sequence Diagram

sequenceDiagram
    participant User
    participant Articulation
    participant ActuatorBase
    participant PhysX

    User->>Articulation: Initialize Articulation(cfg)
    Articulation->>Articulation: _process_actuators_cfg()
    
    Note over Articulation: For each actuator in cfg.actuators
    
    Articulation->>PhysX: get joint properties from USD<br/>(effort_limit, velocity_limit, etc.)
    PhysX-->>Articulation: USD joint properties
    
    Articulation->>ActuatorBase: __init__(cfg, joint_names, joint_ids,<br/>effort_limit=usd_effort_limit, ...)
    
    alt cfg.effort_limit_sim is None AND not implicit model
        ActuatorBase->>ActuatorBase: Set effort_limit_sim = _DEFAULT_MAX_EFFORT_SIM
    end
    
    ActuatorBase->>ActuatorBase: _parse_joint_parameter(cfg.effort_limit_sim, usd_effort_limit)
    Note over ActuatorBase: Stores as self.effort_limit_sim
    
    alt cfg.effort_limit is not None
        ActuatorBase->>ActuatorBase: _parse_joint_parameter(cfg.effort_limit, effort_limit_sim)
    else cfg.effort_limit is None
        ActuatorBase->>ActuatorBase: _parse_joint_parameter(None, usd_effort_limit)
    end
    Note over ActuatorBase: Stores as self.effort_limit
    
    ActuatorBase-->>Articulation: Actuator initialized
    
    Articulation->>PhysX: write_joint_effort_limit_to_sim(actuator.effort_limit_sim)
    Note over PhysX: Sets physics simulation limits

Loading

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Overview

Greptile Summary

This review covers only the changes made since the last review, not the entire PR. The latest change is a minor refactoring in the test file that improves code readability. Specifically, the variable name usd_default has been renamed to usd_default_effort_limit to make its purpose more explicit when testing effort limit initialization behavior. This rename aligns with the improved comment on line 1453 that clarifies "usd default effort limit is set to 80", making the test more self-documenting. The test logic, assertions, and behavior remain completely unchanged - this is purely a cosmetic improvement to variable naming that makes the code easier to understand at a glance.

Important Files Changed

Filename	Score	Overview
source/isaaclab/test/assets/test_articulation.py	5/5	Variable renamed from usd_default to usd_default_effort_limit for clarity

Confidence score: 5/5

• This change is completely safe to merge with zero risk of introducing bugs or breaking existing functionality.
• Score reflects that this is a simple variable rename in test code with no behavioral changes - the test assertions remain identical and the renamed variable is only used locally within the test method.
• No files require special attention as this is a straightforward refactoring change.

_{1 file reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Overview

Greptile Summary

Note: This review covers only the changes made since the last review, not the entire PR.

The latest changes implement the previously suggested logic fix for effort limit initialization and correct a corresponding variable name typo in the test file. In actuator_base.py, the conditional logic for effort_default assignment has been corrected to properly use USD-provided effort limits when cfg.effort_limit is None, preventing the erroneous fallback to the inflated 1.0e9 default for explicit actuator models. In the test file, the variable reference was corrected from usd_default to usd_default_effort_limit to match the variable declaration on line 1454. These changes ensure that physically meaningful effort limits from USD assets are preserved in the actuator initialization pipeline, which is critical for realistic physics simulation and matches the established pattern for velocity_limit handling.

Important Files Changed

Filename	Score	Overview
source/isaaclab/isaaclab/actuators/actuator_base.py	5/5	Fixed conditional logic to use USD effort limits when config value is None
source/isaaclab/test/assets/test_articulation.py	5/5	Corrected variable name typo from usd_default to usd_default_effort_limit

Confidence score: 5/5

• This PR is safe to merge with minimal risk
• Score reflects straightforward logic corrections that directly address the identified bug, with clear test coverage validating the fix
• No files require special attention

_{2 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Overview

Greptile Summary

This review covers only the changes made since the last review, not the entire PR. The latest changes simplify the effort limit initialization logic in the ActuatorBase class based on previous feedback. The implementation now distinguishes between implicit and explicit actuator models: for explicit models, when cfg.effort_limit_sim is not specified, the effort limit defaults to the USD-provided values (passed via constructor); for implicit models or when cfg.effort_limit_sim is specified, it uses effort_limit_sim. This approach ensures that meaningful effort limits from USD assets are preserved for explicit models while maintaining appropriate behavior for implicit models that enforce limits through the solver.

Important Files Changed

Filename	Score	Overview
source/isaaclab/isaaclab/actuators/actuator_base.py	5/5	Simplified effort limit initialization logic to properly handle USD defaults for explicit actuator models

Confidence score: 5/5

• This PR is safe to merge with minimal risk
• The simplified conditional logic correctly addresses the bug where USD effort limits were being overridden; the implementation properly differentiates between implicit and explicit models and handles the None case for cfg.effort_limit_sim as intended
• No files require special attention

_{1 file reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[JuanaDd]

Hi @jtigue-bdai Could you review my latest commit and check if the documentation is clear enough?

[jtigue-bdai]

Looks good. Thanks @JuanaDd