Move modular design of newton-actuators to Newton repo#2449
Move modular design of newton-actuators to Newton repo#2449jvonmuralt wants to merge 18 commits intonewton-physics:mainfrom
Conversation
Signed-off-by: jprozorova <jprozorova@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the external actuator package with an in-repo actuator subsystem: adds Actuator core, Controller implementations (PD, PID, MLP, LSTM), Clamping variants, Delay buffering, USD actuator parsing, builder integration, tests, docs, and removes the external Changes
Sequence Diagram(s)sequenceDiagram
participant User as Simulation User
participant Builder as ModelBuilder
participant Parser as USD Parser
participant Actuator as Actuator.step()
participant Delay as Delay
participant Controller as Controller
participant Clamping as Clamping
participant Warp as Warp GPU
User->>Builder: add_actuator(controller_class, index, clamping, delay)
Builder->>Builder: group shared vs per-DOF args, accumulate indices
User->>Parser: parse_actuator_prim(usd_prim)
Parser-->>User: ActuatorParsed(controller_class, controller_kwargs, component_specs)
User->>Actuator: step(sim_state, sim_control, current_state, dt)
alt Delay present and not ready
Actuator-->>Delay: update_state only
Actuator-->>User: skip compute (no forces)
else
Actuator->>Delay: get_delayed_targets(current_state)
Delay-->>Actuator: delayed_targets
Actuator->>Controller: compute(positions, velocities, targets, inputs, indices, forces_buf)
Controller->>Warp: launch kernel or run PyTorch inference
Warp-->>Controller: raw forces
Actuator->>Clamping: modify_forces(raw_forces, applied_forces, positions, velocities, indices)
Clamping->>Warp: launch clamp kernel
Warp-->>Clamping: clamped forces
Actuator->>Warp: scatter-add applied_forces into sim_control
Warp-->>Actuator: sim_control updated
Actuator->>Controller: update_state(current, next) if stateful
Actuator->>Delay: update_state(targets, inputs, input_indices, current, next)
Actuator-->>User: return (states updated)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
newton/_src/actuators/controllers/controller_net_mlp.py (1)
37-39: Consider adding a None check inState.reset().If
reset()is called beforestate()initializes the tensors, calling.zero_()onNonewill raiseAttributeError. While the current flow should prevent this, a defensive check would improve robustness.🛡️ Proposed defensive check
def reset(self) -> None: + if self.pos_error_history is not None: + self.pos_error_history.zero_() + if self.vel_history is not None: + self.vel_history.zero_() - self.pos_error_history.zero_() - self.vel_history.zero_()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_mlp.py` around lines 37 - 39, In State.reset(), guard against attributes being None before calling in-place zeroing: check that self.pos_error_history and self.vel_history are not None (or use truthy check) before calling .zero_() so reset() no-ops safely if tensors haven't been initialized; update the reset method (referenced as reset and attributes pos_error_history, vel_history) to perform these None checks.newton/_src/actuators/usd_parser.py (1)
143-146: Redundant None check:schemasis already filtered to registry keys.Line 122 filters
schemasto only include names present inSCHEMA_REGISTRY, soSCHEMA_REGISTRY.get(schema_name)will never returnNone. The check is dead code.♻️ Proposed simplification
for schema_name in schemas: - entry = SCHEMA_REGISTRY.get(schema_name) - if entry is None: - continue + entry = SCHEMA_REGISTRY[schema_name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 143 - 146, The loop contains a redundant None check: since `schemas` was previously filtered to keys present in `SCHEMA_REGISTRY`, `SCHEMA_REGISTRY.get(schema_name)` cannot return None; remove the `if entry is None: continue` branch and either use the direct lookup `entry = SCHEMA_REGISTRY[schema_name]` or keep `get` but proceed without the dead check in the loop that iterates over `schemas` (referencing `schemas`, `SCHEMA_REGISTRY.get`, and the `entry` variable).newton/_src/actuators/clamping/base.py (1)
50-73: Add SI units tomodify_forcesdocstring and consider annotating array dtypes.Per coding guidelines, physical quantities should include SI units. Also,
wp.arrayannotations should use bracket syntax to specify dtype.📝 Proposed improvements
def modify_forces( self, - src_forces: wp.array, - dst_forces: wp.array, - positions: wp.array, - velocities: wp.array, - input_indices: wp.array, + src_forces: wp.array[float], + dst_forces: wp.array[float], + positions: wp.array[float], + velocities: wp.array[float], + input_indices: wp.array[Any], num_actuators: int, ) -> None: """Read forces from src, apply clamping, write to dst. When src and dst are the same array, this is an in-place update. The Actuator uses different arrays for the first clamping (to preserve the raw controller output) and the same array for subsequent clampings. Args: - src_forces: Input force buffer to read. Shape (N,). - dst_forces: Output force buffer to write. Shape (N,). - positions: Joint positions (global array). - velocities: Joint velocities (global array). + src_forces: Input force buffer [N or N·m] to read. Shape (N,). + dst_forces: Output force buffer [N or N·m] to write. Shape (N,). + positions: Joint positions [m or rad] (global array). + velocities: Joint velocities [m/s or rad/s] (global array). input_indices: Indices into positions/velocities. num_actuators: Number of actuators N. """As per coding guidelines: "Annotate Warp arrays with bracket syntax" and "Include SI units for physical quantities in public API docstrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/clamping/base.py` around lines 50 - 73, The modify_forces method docstring is missing SI units for physical quantities and the wp.array type annotations lack dtype brackets; update the docstring for modify_forces to state SI units for forces (e.g., newtons, N), positions (meters or radians as appropriate), and velocities (m/s or rad/s), and change the type hints for src_forces, dst_forces, positions, velocities, and input_indices to use bracketed Warp array dtypes (e.g., wp.array[float32] or wp.array[int32] as appropriate) while leaving num_actuators as int; ensure the docstring units match the semantics used elsewhere in the Actuator implementation.newton/_src/actuators/clamping/clamping_position_based.py (1)
82-85: Add SI units to docstring for physical quantities.Per coding guidelines, public API docstrings should include SI units.
lookup_anglesshould specify[rad]andlookup_torquesshould specify[N·m].📝 Proposed docstring update
Args: - lookup_angles: Sorted joint angles for the torque lookup table. Shape (K,). - lookup_torques: Max output torques corresponding to lookup_angles. Shape (K,). + lookup_angles: Sorted joint angles [rad] for the torque lookup table. Shape (K,). + lookup_torques: Max output torques [N·m] corresponding to lookup_angles. Shape (K,).As per coding guidelines: "Include SI units for physical quantities in public API docstrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/clamping/clamping_position_based.py` around lines 82 - 85, Update the public docstring for the clamping position-based API to include SI units for the physical quantities: annotate lookup_angles as in radians (e.g., lookup_angles [rad]) and lookup_torques as in newton-metres (e.g., lookup_torques [N·m]) in the Args section of the constructor/function (search for lookup_angles and lookup_torques in ClampingPositionBased / clamping_position_based.py) so the docstring clearly states the units.newton/_src/actuators/controllers/controller_net_lstm.py (2)
175-183: Guard againstcurrent_statebeingNoneinupdate_state.The method checks
next_state is Nonebut doesn't checkcurrent_state. Ifcurrent_stateisNone, the method would still try to accessself._hiddenandself._cellwhich are set incompute(). This works ifcompute()was called first, but the method signature suggests both states could be optional.🛡️ Suggested improvement for clarity
def update_state( self, current_state: ControllerNetLSTM.State, next_state: ControllerNetLSTM.State, ) -> None: - if next_state is None: + if next_state is None or self._hidden is None: return next_state.hidden = self._hidden next_state.cell = self._cellThis guards against the edge case where
update_stateis called without a priorcompute()call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_lstm.py` around lines 175 - 183, The update_state method currently only guards next_state but not current_state; modify ControllerNetLSTM.update_state to also check if current_state is None (or if self._hidden/self._cell are uninitialized) and return early if so, to avoid using self._hidden and self._cell when compute() wasn't called; reference the method name update_state, parameters current_state and next_state, and the instance attributes self._hidden and self._cell when adding the guard and returning early.
132-173: Object identity check for index selection may be fragile.Line 155 uses
target_indices is self._warp_sequential_indicesto determine which torch indices to use. Object identity checks can be unreliable if the array is copied or recreated elsewhere. Consider using a more robust comparison or documenting this contract.💡 Alternative approach
If the identity check is intentional for performance (avoiding array comparison), document the contract clearly:
+ # Identity check: when delay is active, target_indices points to + # sequential_indices (local indexing). When not delayed, it points + # to input_indices (global indexing). torch_target_idx = ( self._torch_sequential_indices if target_indices is self._warp_sequential_indices else self._torch_input_indices )Or use a flag passed from the caller to indicate which indexing mode is active.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_lstm.py` around lines 132 - 173, In compute, avoid fragile object-identity selection using `target_indices is self._warp_sequential_indices`; instead determine the correct torch index set by a robust check or explicit flag: either compare index contents (e.g., element-wise/equality check between `target_indices` and `self._warp_sequential_indices` or convert to torch and use `torch.equal`) or add an explicit boolean parameter (e.g., `use_sequential_indices`) passed from the caller and branch on that; update the selection logic for `torch_target_idx` (currently choosing between `self._torch_sequential_indices` and `self._torch_input_indices`) and adjust call sites to provide the flag if you take that approach.newton/_src/actuators/actuator.py (1)
160-168: Consider makingSHARED_PARAMSa class variable instead of a property.
SHARED_PARAMSis named like a constant (uppercase) but implemented as a property that recomputes the set on every access. This is inconsistent with theClassVar[set[str]]pattern used in other classes likeDelayandController.♻️ Suggested refactor
If the composed set must be dynamic, consider caching it or documenting that this is intentional. Alternatively, compute it once during
__init__:def __init__( self, indices: wp.array, controller: Controller, delay: Delay | None = None, clamping: list[Clamping] | None = None, ... ): ... + # Compute shared params once + self._shared_params: set[str] = set() + self._shared_params |= self.controller.SHARED_PARAMS + if self.delay is not None: + self._shared_params |= self.delay.SHARED_PARAMS + for c in self.clamping: + self._shared_params |= c.SHARED_PARAMS - `@property` - def SHARED_PARAMS(self) -> set[str]: - params: set[str] = set() - params |= self.controller.SHARED_PARAMS - if self.delay is not None: - params |= self.delay.SHARED_PARAMS - for c in self.clamping: - params |= c.SHARED_PARAMS - return params + `@property` + def SHARED_PARAMS(self) -> set[str]: + return self._shared_params🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/actuator.py` around lines 160 - 168, The SHARED_PARAMS attribute is implemented as a recomputed `@property` but named like a constant; change it to a class variable or compute/cache it once in the instance to match the ClassVar pattern used by Delay/Controller. Modify SHARED_PARAMS from a property to either a ClassVar[set[str]] (if static) or compute the composed set in __init__ (using self.controller.SHARED_PARAMS, self.delay.SHARED_PARAMS when delay is not None, and each c.SHARED_PARAMS from self.clamping) and store it on the instance (e.g., self.SHARED_PARAMS) so subsequent accesses do not recompute; if dynamic updates are required, add explicit invalidation or lazy caching instead. Ensure references to SHARED_PARAMS elsewhere continue to work with the new class/instance attribute semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/actuators/clamping/clamping_dc_motor.py`:
- Around line 51-59: In resolve_arguments of ClampingDCMotor validate that
"velocity_limit" is present and strictly > 0 (raise ValueError if <= 0) and also
prevent the default saturation_effort from being math.inf (either require a
finite saturation_effort or set a safe finite default) to avoid an inf * 0 path;
update the returned dict to use the validated velocity_limit and a finite
saturation_effort and reference the "saturation_effort" and "velocity_limit"
keys in your checks.
In `@newton/_src/actuators/clamping/clamping_position_based.py`:
- Line 69: The error message uses the wrong class name "ClampPositionBased";
update the string so it matches the actual class name `ClampingPositionBased`
(in the constructor/initializer where the ValueError is raised) and search for
any other occurrences of "ClampPositionBased" in the same file to correct them
as well.
- Around line 96-105: The code may leave pre-allocated wp.array objects
(self.lookup_angles, self.lookup_torques) on the wrong device; update set_device
to ensure device consistency by checking the device of any existing wp.array and
migrating/copying it to the target device if it differs (or raising a clear
error); specifically inspect self.lookup_angles and self.lookup_torques inside
set_device (and use wp.array(..., device=...) or a device-aware copy/migrate API
when needed) so kernels launched later won’t see device mismatches.
In `@newton/_src/actuators/controllers/controller_net_mlp.py`:
- Around line 164-166: ControllerNetMLP currently ignores the documented
force_indices contract by doing wp.copy(forces, torques_wp) (see the
torques.reshape, torques_wp creation and wp.copy call); change this to write
into forces via force_indices like ControllerPD/ControllerPID: create a small
warp kernel (or reuse the existing indexed-write kernel) that takes forces,
force_indices and torques_wp and does forces[force_indices[i]] = torques_wp[i],
and call that instead of wp.copy; alternatively, if you intentionally rely on
sequential indexing, add a clear comment on ControllerNetMLP and its constructor
documenting that force_indices must be sequential and remove/validate the
force_indices parameter accordingly.
In `@newton/_src/actuators/delay.py`:
- Around line 67-72: reset() currently assumes self.buffer_pos, self.buffer_vel,
and self.buffer_act are non-None and calls .zero_() and .shape on them, which
will raise AttributeError when those attributes are None; update reset() to
first check for None (e.g. if any(b is None for b in (self.buffer_pos,
self.buffer_vel, self.buffer_act)): set self.is_filled = False, set
self.write_idx = -1 or another safe sentinel, and return) otherwise call
.zero_() and compute write_idx as before; alternatively, if the class invariant
guarantees buffers are never None after initialization, remove the | None from
the type hints for buffer_pos/buffer_vel/buffer_act to make the contract
explicit.
In `@newton/_src/actuators/usd_parser.py`:
- Around line 168-171: The error raised when controller_class is None uses
prim.GetAppliedSchemas(), which per earlier notes won't include custom schemas
and is misleading; update the ValueError in usd_parser (the block around
controller_class check) to instead report the schemas discovered from metadata
(the same metadata lookup used earlier around lines 107-109), e.g., include the
list returned by that metadata-based schema extraction or a clear statement that
only metadata-derived schemas were checked, and remove or replace the
GetAppliedSchemas() call so the message accurately reflects what was inspected.
In `@newton/_src/sim/builder.py`:
- Line 1774: The code currently appends the caller-provided list object directly
(entry.indices.append(indices)), which allows external mutation to affect stored
targets; change this to append a defensive copy of the list (e.g., a shallow
copy via list(indices) or indices.copy()) when adding to entry.indices so the
stored data cannot be mutated by the caller later—if stored elements can
themselves be mutable and require isolation, use a deep copy instead.
- Around line 1689-1711: The new add_actuator signature replaces the old
actuator_class/input_indices/output_indices API and will break callers; add a
compatibility shim inside ModelBuilder.add_actuator that detects the legacy
invocation (presence of actuator_class or input_indices/output_indices in kwargs
or positional args), issues a DeprecationWarning referencing the old symbols,
and maps the old parameters to the new ones (map actuator_class →
controller_class, combine input_indices/output_indices into indices or infer
indices as needed, and translate any old per-DOF kwargs) before delegating to
the new logic; ensure the shim preserves existing behavior (including handling
clamping and delay) and only emits the deprecation warning once per process.
- Around line 10349-10379: The loop in ModelBuilder.finalize currently iterates
self.actuator_entries.values(), which processes grouped entries by key and can
reorder actuators versus the original add (insertion) order; fix by tracking and
iterating the actual insertion sequence: add a new list attribute (e.g.,
self.actuator_add_order or self.actuator_entries_order) that append-receives a
reference/tuple for each actuator when created (update the code-path that
creates entries to append there), then in finalize replace for entry in
self.actuator_entries.values(): with a loop over that insertion list so you call
_build_index_array, _stack_args_to_arrays, construct controller/Delay/clamping
and create Actuator in the exact add order; ensure any existing code using
self.actuator_entries still works and remove reliance on dict value ordering.
In `@newton/_src/utils/import_usd.py`:
- Around line 2759-2767: The code currently maps each joint path to a single DOF
index (path_to_dof using builder.joint_qd_start), so multi-DOF joints lose their
extra DOFs; change this to map each joint path to the full list/range of DOF
indices for that joint (e.g., compute start = builder.joint_qd_start[idx] and
count = number of DOFs for that joint—obtainable from builder (e.g.,
builder.joint_qd_count) or by using the next joint start to infer the range) and
build path_to_dofs -> list(range(start, start+count)); then when processing
parsed.target_paths in parse_actuator_prim, gather and flatten all DOF indices
from path_to_dofs (instead of a single index) into dof_indices before calling
add_actuator so actuators target all DOFs of multi-DOF joints (referencing
path_to_dof/path_to_dofs, builder.joint_qd_start, parsed.target_paths,
parse_actuator_prim, add_actuator, and dof_indices).
---
Nitpick comments:
In `@newton/_src/actuators/actuator.py`:
- Around line 160-168: The SHARED_PARAMS attribute is implemented as a
recomputed `@property` but named like a constant; change it to a class variable or
compute/cache it once in the instance to match the ClassVar pattern used by
Delay/Controller. Modify SHARED_PARAMS from a property to either a
ClassVar[set[str]] (if static) or compute the composed set in __init__ (using
self.controller.SHARED_PARAMS, self.delay.SHARED_PARAMS when delay is not None,
and each c.SHARED_PARAMS from self.clamping) and store it on the instance (e.g.,
self.SHARED_PARAMS) so subsequent accesses do not recompute; if dynamic updates
are required, add explicit invalidation or lazy caching instead. Ensure
references to SHARED_PARAMS elsewhere continue to work with the new
class/instance attribute semantics.
In `@newton/_src/actuators/clamping/base.py`:
- Around line 50-73: The modify_forces method docstring is missing SI units for
physical quantities and the wp.array type annotations lack dtype brackets;
update the docstring for modify_forces to state SI units for forces (e.g.,
newtons, N), positions (meters or radians as appropriate), and velocities (m/s
or rad/s), and change the type hints for src_forces, dst_forces, positions,
velocities, and input_indices to use bracketed Warp array dtypes (e.g.,
wp.array[float32] or wp.array[int32] as appropriate) while leaving num_actuators
as int; ensure the docstring units match the semantics used elsewhere in the
Actuator implementation.
In `@newton/_src/actuators/clamping/clamping_position_based.py`:
- Around line 82-85: Update the public docstring for the clamping position-based
API to include SI units for the physical quantities: annotate lookup_angles as
in radians (e.g., lookup_angles [rad]) and lookup_torques as in newton-metres
(e.g., lookup_torques [N·m]) in the Args section of the constructor/function
(search for lookup_angles and lookup_torques in ClampingPositionBased /
clamping_position_based.py) so the docstring clearly states the units.
In `@newton/_src/actuators/controllers/controller_net_lstm.py`:
- Around line 175-183: The update_state method currently only guards next_state
but not current_state; modify ControllerNetLSTM.update_state to also check if
current_state is None (or if self._hidden/self._cell are uninitialized) and
return early if so, to avoid using self._hidden and self._cell when compute()
wasn't called; reference the method name update_state, parameters current_state
and next_state, and the instance attributes self._hidden and self._cell when
adding the guard and returning early.
- Around line 132-173: In compute, avoid fragile object-identity selection using
`target_indices is self._warp_sequential_indices`; instead determine the correct
torch index set by a robust check or explicit flag: either compare index
contents (e.g., element-wise/equality check between `target_indices` and
`self._warp_sequential_indices` or convert to torch and use `torch.equal`) or
add an explicit boolean parameter (e.g., `use_sequential_indices`) passed from
the caller and branch on that; update the selection logic for `torch_target_idx`
(currently choosing between `self._torch_sequential_indices` and
`self._torch_input_indices`) and adjust call sites to provide the flag if you
take that approach.
In `@newton/_src/actuators/controllers/controller_net_mlp.py`:
- Around line 37-39: In State.reset(), guard against attributes being None
before calling in-place zeroing: check that self.pos_error_history and
self.vel_history are not None (or use truthy check) before calling .zero_() so
reset() no-ops safely if tensors haven't been initialized; update the reset
method (referenced as reset and attributes pos_error_history, vel_history) to
perform these None checks.
In `@newton/_src/actuators/usd_parser.py`:
- Around line 143-146: The loop contains a redundant None check: since `schemas`
was previously filtered to keys present in `SCHEMA_REGISTRY`,
`SCHEMA_REGISTRY.get(schema_name)` cannot return None; remove the `if entry is
None: continue` branch and either use the direct lookup `entry =
SCHEMA_REGISTRY[schema_name]` or keep `get` but proceed without the dead check
in the loop that iterates over `schemas` (referencing `schemas`,
`SCHEMA_REGISTRY.get`, and the `entry` variable).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b0f73f95-8be9-48ca-8df9-c382b2ec21df
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
docs/guide/release.rstnewton/__init__.pynewton/_src/actuators/__init__.pynewton/_src/actuators/actuator.pynewton/_src/actuators/clamping/__init__.pynewton/_src/actuators/clamping/base.pynewton/_src/actuators/clamping/clamping_dc_motor.pynewton/_src/actuators/clamping/clamping_max_force.pynewton/_src/actuators/clamping/clamping_position_based.pynewton/_src/actuators/controllers/__init__.pynewton/_src/actuators/controllers/base.pynewton/_src/actuators/controllers/controller_net_lstm.pynewton/_src/actuators/controllers/controller_net_mlp.pynewton/_src/actuators/controllers/controller_pd.pynewton/_src/actuators/controllers/controller_pid.pynewton/_src/actuators/delay.pynewton/_src/actuators/usd_parser.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/solvers/kamino/README.mdnewton/_src/utils/import_usd.pynewton/_src/utils/selection.pynewton/actuators.pynewton/tests/assets/actuator_test.usdanewton/tests/test_actuators.pypyproject.toml
💤 Files with no reviewable changes (2)
- newton/_src/solvers/kamino/README.md
- pyproject.toml
| for entry in self.actuator_entries.values(): | ||
| indices = self._build_index_array(entry.indices, device) | ||
|
|
||
| # Build controller from stacked per-DOF arrays + shared kwargs | ||
| ctrl_arrays = self._stack_args_to_arrays( | ||
| entry.controller_args, device=device, requires_grad=requires_grad | ||
| ) | ||
| controller = entry.controller_class(**ctrl_arrays, **entry.controller_shared_kwargs) | ||
|
|
||
| # Build delay object (scalar, shared across the group) | ||
| delay_obj = Delay(entry.delay) if entry.delay is not None else None | ||
|
|
||
| # Build clamping objects from per-DOF arrays + shared kwargs | ||
| clamping_objs = [] | ||
| for i, (comp_class, shared_kw) in enumerate( | ||
| zip(entry.clamping_classes, entry.clamping_shared_kwargs, strict=True) | ||
| ): | ||
| comp_args_per_actuator = [per_act[i] for per_act in entry.clamping_args] | ||
| comp_arrays = self._stack_args_to_arrays( | ||
| comp_args_per_actuator, device=device, requires_grad=requires_grad | ||
| ) | ||
| clamping_objs.append(comp_class(**comp_arrays, **shared_kw)) | ||
|
|
||
| actuator = Actuator( | ||
| indices=indices, | ||
| controller=controller, | ||
| delay=delay_obj, | ||
| clamping=clamping_objs if clamping_objs else None, | ||
| ) | ||
| m.actuators.append(actuator) | ||
|
|
There was a problem hiding this comment.
Grouped finalize iteration can violate strict add-order semantics.
Iterating self.actuator_entries.values() emits groups by first-seen key, which can reorder actuators when calls interleave different groups (e.g., A(key1), B(key2), C(key1)).
Based on learnings: “Repo: newton-physics/newton — Actuator creation order in ModelBuilder.finalize must follow insertion order (order of adding), not a sorted order.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/sim/builder.py` around lines 10349 - 10379, The loop in
ModelBuilder.finalize currently iterates self.actuator_entries.values(), which
processes grouped entries by key and can reorder actuators versus the original
add (insertion) order; fix by tracking and iterating the actual insertion
sequence: add a new list attribute (e.g., self.actuator_add_order or
self.actuator_entries_order) that append-receives a reference/tuple for each
actuator when created (update the code-path that creates entries to append
there), then in finalize replace for entry in self.actuator_entries.values():
with a loop over that insertion list so you call _build_index_array,
_stack_args_to_arrays, construct controller/Delay/clamping and create Actuator
in the exact add order; ensure any existing code using self.actuator_entries
still works and remove reliance on dict value ordering.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
newton/_src/actuators/usd_parser.py (1)
168-171:⚠️ Potential issue | 🟡 MinorUse metadata-derived schemas in the missing-controller error message.
This message currently reports
prim.GetAppliedSchemas(), which does not reflect the custom schema tokens you actually parsed from metadata. Reportschemas(and ideally the prim path) instead for accurate debugging.In Pixar USD, does UsdPrim.GetAppliedSchemas() include unregistered custom API schema tokens that are only present in the prim's `apiSchemas` metadata list?🔧 Proposed fix
if controller_class is None: raise ValueError( - f"Actuator prim has no controller schema applied (applied schemas: {prim.GetAppliedSchemas()})" + f"Actuator prim '{prim.GetPath()}' has no controller schema applied " + f"(metadata schemas checked: {schemas})" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 168 - 171, The error raised when controller_class is None uses prim.GetAppliedSchemas(), which can omit custom API schema tokens parsed from metadata; update the ValueError to include the parsed schemas variable (e.g., schemas) and the prim path (e.g., prim.GetPath()) so the message reflects metadata-derived schemas and the prim location. Locate the check around controller_class in usd_parser.py and replace or augment the error text to reference schemas and prim.GetPath() (and keep prim.GetAppliedSchemas() only if helpful) to provide accurate debugging info.
🧹 Nitpick comments (1)
newton/_src/actuators/usd_parser.py (1)
143-146: Remove redundant registry lookup branch.
get_schemas_from_prim()already filters to keys inSCHEMA_REGISTRY, soentry is Nonehere is unreachable in normal flow.♻️ Proposed cleanup
for schema_name in schemas: - entry = SCHEMA_REGISTRY.get(schema_name) - if entry is None: - continue + entry = SCHEMA_REGISTRY[schema_name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 143 - 146, The loop over schemas contains an unreachable defensive check for entry = SCHEMA_REGISTRY.get(schema_name) followed by "if entry is None: continue"; since get_schemas_from_prim() already guarantees schema_name is in SCHEMA_REGISTRY, remove that branch and use entry directly inside the loop (or replace it with an assertion like assert entry is not None if you want a sanity check). Update the loop in usd_parser.py to delete the "if entry is None: continue" lines and proceed to use the retrieved entry variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/actuators/usd_parser.py`:
- Around line 125-130: The docstring and type hint for parse_actuator_prim are
inconsistent with the code path that raises ValueError when controller schemas
are malformed; update the contract to be explicit: either change the
exception-throwing code in the controller/schema handling (the block that
currently raises ValueError) to return None for invalid actuator definitions, or
update the parse_actuator_prim docstring and return type to indicate it may
raise ValueError on malformed schemas (and add a brief sentence about fail-fast
behavior), and adjust any callers/tests accordingly; reference the
parse_actuator_prim function and the ValueError-raising controller/schema
validation block when making the change.
---
Duplicate comments:
In `@newton/_src/actuators/usd_parser.py`:
- Around line 168-171: The error raised when controller_class is None uses
prim.GetAppliedSchemas(), which can omit custom API schema tokens parsed from
metadata; update the ValueError to include the parsed schemas variable (e.g.,
schemas) and the prim path (e.g., prim.GetPath()) so the message reflects
metadata-derived schemas and the prim location. Locate the check around
controller_class in usd_parser.py and replace or augment the error text to
reference schemas and prim.GetPath() (and keep prim.GetAppliedSchemas() only if
helpful) to provide accurate debugging info.
---
Nitpick comments:
In `@newton/_src/actuators/usd_parser.py`:
- Around line 143-146: The loop over schemas contains an unreachable defensive
check for entry = SCHEMA_REGISTRY.get(schema_name) followed by "if entry is
None: continue"; since get_schemas_from_prim() already guarantees schema_name is
in SCHEMA_REGISTRY, remove that branch and use entry directly inside the loop
(or replace it with an assertion like assert entry is not None if you want a
sanity check). Update the loop in usd_parser.py to delete the "if entry is None:
continue" lines and proceed to use the retrieved entry variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8c99773d-01be-4e70-b7b1-02abbf39a391
📒 Files selected for processing (2)
newton/_src/actuators/usd_parser.pynewton/tests/assets/actuator_test.usda
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/assets/actuator_test.usda
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
newton/_src/utils/import_usd.py (1)
2759-2768:⚠️ Potential issue | 🟠 MajorExpand joint targets to all DOFs before adding actuators.
path_to_dofcollapses each joint path to a singlejoint_qd_start, so an actuator targeting a multi-DOF joint only wires its first DOF. SinceModelBuilder.add_actuator()is single-DOF, this needs to fan out the joint path into every DOF index and add one actuator entry per DOF.Proposed fix
- path_to_dof = { - path: builder.joint_qd_start[idx] for path, idx in path_joint_map.items() if idx < len(builder.joint_qd_start) - } + path_to_dofs: dict[str, list[int]] = {} + for path, joint_idx in path_joint_map.items(): + if joint_idx >= len(builder.joint_qd_start): + continue + dof_start = builder.joint_qd_start[joint_idx] + dof_end = ( + builder.joint_qd_start[joint_idx + 1] + if joint_idx + 1 < len(builder.joint_qd_start) + else len(builder.joint_qd) + ) + path_to_dofs[path] = list(range(dof_start, dof_end)) @@ - if parsed.target_path not in path_to_dof: + dof_indices = path_to_dofs.get(parsed.target_path) + if not dof_indices: continue - dof_index = path_to_dof[parsed.target_path] @@ - builder.add_actuator( - parsed.controller_class, - index=dof_index, - clamping=clamping_specs if clamping_specs else None, - delay=delay_val, - **parsed.controller_kwargs, - ) - actuator_count += 1 + for dof_index in dof_indices: + builder.add_actuator( + parsed.controller_class, + index=dof_index, + clamping=clamping_specs if clamping_specs else None, + delay=delay_val, + **parsed.controller_kwargs, + ) + actuator_count += 1Also applies to: 2779-2786
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/utils/import_usd.py` around lines 2759 - 2768, path_to_dof currently maps each joint path to a single start index (using builder.joint_qd_start) which causes actuators to wire only the first DOF of multi-DOF joints; change the logic that builds path_to_dof (and the equivalent block at the second occurrence) to expand each joint path into all its DOF indices by using the joint's DOF count (from builder.joint_qd_start and builder.joint_qd_count or equivalent) and produce a list of indices per path, then when handling parsed = parse_actuator_prim(prim) iterate over all indices for parsed.target_path and call ModelBuilder.add_actuator (or the existing actuator-add code) once per DOF so each DOF gets its own actuator entry.newton/_src/actuators/usd_parser.py (2)
171-174:⚠️ Potential issue | 🟡 MinorReport the metadata-derived schemas in the missing-controller error.
GetAppliedSchemas()does not reflect these custom Newton API tokens, so this message points debugging at the wrong evidence. Use theschemaslist you already extracted fromapiSchemasmetadata instead.Proposed fix
- raise ValueError( - f"Actuator prim has no controller schema applied (applied schemas: {prim.GetAppliedSchemas()})" - ) + raise ValueError( + f"Actuator prim '{prim.GetPath()}' has no controller schema applied " + f"(found schemas: {schemas})" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 171 - 174, The ValueError raised when controller_class is None shows prim.GetAppliedSchemas() which omits metadata-derived Newton API tokens; replace that evidence with the already-extracted schemas list (the variable named schemas from apiSchemas metadata) in the error message so the raised ValueError uses schemas (and include prim.path or similar context if desired) instead of prim.GetAppliedSchemas() in the raise inside usd_parser.py where controller_class is checked.
128-133:⚠️ Potential issue | 🟡 MinorDocument that malformed actuator prims raise, not
None.The current docstring reads like every invalid actuator case returns
None, but this function fail-fast raises on malformed controller schemas. Please make that contract explicit so callers know they must handle exceptions separately from “not an actuator” cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 128 - 133, Update the parse_actuator_prim docstring to state that the function returns a ParsedActuator or None (when the prim is not an actuator) but will raise on malformed actuator/controller schemas rather than returning None; mention parse_actuator_prim by name and note callers must handle exceptions raised during schema parsing (e.g., malformed controller schemas) separately from the None "not an actuator" case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/actuators/controllers/controller_net_lstm.py`:
- Around line 141-153: The code incorrectly tests array identity with
"target_indices is input_indices" and can pick the wrong indices; instead
convert the actual target_indices contents to a torch index tensor and use that
for indexing. Modify the block that sets torch_target_idx to compute a torch
tensor from target_indices (e.g., torch.tensor(target_indices.numpy(),
dtype=torch.long, device=self._torch_device)) when target_indices does not match
the cached self._torch_input_indices, falling back to cached
self._torch_input_indices or self._torch_sequential_indices only as an
optimization; then compute pos_error and vel using that computed
torch_target_idx (use target[torch_target_idx] and
current_pos[self._torch_input_indices] / current_vel[self._torch_input_indices]
as appropriate).
---
Duplicate comments:
In `@newton/_src/actuators/usd_parser.py`:
- Around line 171-174: The ValueError raised when controller_class is None shows
prim.GetAppliedSchemas() which omits metadata-derived Newton API tokens; replace
that evidence with the already-extracted schemas list (the variable named
schemas from apiSchemas metadata) in the error message so the raised ValueError
uses schemas (and include prim.path or similar context if desired) instead of
prim.GetAppliedSchemas() in the raise inside usd_parser.py where
controller_class is checked.
- Around line 128-133: Update the parse_actuator_prim docstring to state that
the function returns a ParsedActuator or None (when the prim is not an actuator)
but will raise on malformed actuator/controller schemas rather than returning
None; mention parse_actuator_prim by name and note callers must handle
exceptions raised during schema parsing (e.g., malformed controller schemas)
separately from the None "not an actuator" case.
In `@newton/_src/utils/import_usd.py`:
- Around line 2759-2768: path_to_dof currently maps each joint path to a single
start index (using builder.joint_qd_start) which causes actuators to wire only
the first DOF of multi-DOF joints; change the logic that builds path_to_dof (and
the equivalent block at the second occurrence) to expand each joint path into
all its DOF indices by using the joint's DOF count (from builder.joint_qd_start
and builder.joint_qd_count or equivalent) and produce a list of indices per
path, then when handling parsed = parse_actuator_prim(prim) iterate over all
indices for parsed.target_path and call ModelBuilder.add_actuator (or the
existing actuator-add code) once per DOF so each DOF gets its own actuator
entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c8ef74a3-60d9-4c8a-81fb-1651a08729c5
📒 Files selected for processing (14)
newton/_src/actuators/actuator.pynewton/_src/actuators/clamping/base.pynewton/_src/actuators/clamping/clamping_dc_motor.pynewton/_src/actuators/clamping/clamping_position_based.pynewton/_src/actuators/controllers/base.pynewton/_src/actuators/controllers/controller_net_lstm.pynewton/_src/actuators/controllers/controller_net_mlp.pynewton/_src/actuators/controllers/controller_pd.pynewton/_src/actuators/controllers/controller_pid.pynewton/_src/actuators/delay.pynewton/_src/actuators/usd_parser.pynewton/_src/sim/builder.pynewton/_src/utils/import_usd.pynewton/tests/test_actuators.py
🚧 Files skipped from review as they are similar to previous changes (8)
- newton/_src/actuators/clamping/base.py
- newton/_src/actuators/clamping/clamping_dc_motor.py
- newton/_src/actuators/clamping/clamping_position_based.py
- newton/_src/actuators/controllers/base.py
- newton/_src/actuators/delay.py
- newton/_src/actuators/actuator.py
- newton/_src/actuators/controllers/controller_net_mlp.py
- newton/_src/sim/builder.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
newton/_src/actuators/clamping/clamping_position_based.py (1)
84-94: Consider validating that lookup torques are positive.The kernel applies symmetric clamping
[-limit, limit]wherelimitcomes fromlookup_torques. If a user provides negative torque values, the clamping behavior would be inverted (e.g.,-limit>+limit). Consider adding validation that alllookup_torquesvalues are non-negative.🛡️ Proposed validation
if len(lookup_angles) != len(lookup_torques): raise ValueError( f"lookup_angles length ({len(lookup_angles)}) must match lookup_torques length ({len(lookup_torques)})" ) + # Validate positive torque limits for symmetric clamping + if any(t < 0 for t in lookup_torques) if not isinstance(lookup_torques, wp.array) else False: + raise ValueError("lookup_torques must contain non-negative values for symmetric clamping") self.lookup_size = len(lookup_angles)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/clamping/clamping_position_based.py` around lines 84 - 94, The code does not validate sign of lookup_torques, which are used as symmetric limits in the clamping logic; convert lookup_torques to a wp.array (as done currently) and then check that all entries in lookup_torques are >= 0 (e.g., using numpy or wp array comparison) and raise a ValueError with a clear message if any value is negative; update the constructor in the ClampingPositionBased class (symbols: lookup_torques, lookup_angles, lookup_size, self.lookup_torques) to perform this non-negative check after conversion and before assigning to self.lookup_torques.newton/_src/actuators/usd_parser.py (1)
96-104: Consider warning when relationship has unexpected target count.
get_relationship_targetsilently returnsNonefor both missing relationships and relationships with multiple targets. This could hide USD authoring errors where someone accidentally added multiple targets. A debug-level warning might help diagnose configuration issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 96 - 104, get_relationship_target currently returns None silently when the relationship is missing or has !=1 targets; add a debug-level warning so unexpected multi-target relationships are surfaced. Modify get_relationship_target to obtain a module-level logger (logging.getLogger(__name__)) and when rel is present but len(rel.GetTargets()) != 1, log a debug message including the prim identity (e.g. str(prim.GetPath()) or prim.GetName()) and the relationship name and actual target count before returning None; leave behavior unchanged for missing relationship but you may also log a debug message for that case if desired. Use the existing calls prim.GetRelationship and rel.GetTargets to locate where to insert the logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@newton/_src/actuators/clamping/clamping_position_based.py`:
- Around line 84-94: The code does not validate sign of lookup_torques, which
are used as symmetric limits in the clamping logic; convert lookup_torques to a
wp.array (as done currently) and then check that all entries in lookup_torques
are >= 0 (e.g., using numpy or wp array comparison) and raise a ValueError with
a clear message if any value is negative; update the constructor in the
ClampingPositionBased class (symbols: lookup_torques, lookup_angles,
lookup_size, self.lookup_torques) to perform this non-negative check after
conversion and before assigning to self.lookup_torques.
In `@newton/_src/actuators/usd_parser.py`:
- Around line 96-104: get_relationship_target currently returns None silently
when the relationship is missing or has !=1 targets; add a debug-level warning
so unexpected multi-target relationships are surfaced. Modify
get_relationship_target to obtain a module-level logger
(logging.getLogger(__name__)) and when rel is present but len(rel.GetTargets())
!= 1, log a debug message including the prim identity (e.g. str(prim.GetPath())
or prim.GetName()) and the relationship name and actual target count before
returning None; leave behavior unchanged for missing relationship but you may
also log a debug message for that case if desired. Use the existing calls
prim.GetRelationship and rel.GetTargets to locate where to insert the logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2ce5c866-5b1a-44e9-a3e2-7e4ecf768fa0
📒 Files selected for processing (8)
newton/_src/actuators/__init__.pynewton/_src/actuators/actuator.pynewton/_src/actuators/clamping/base.pynewton/_src/actuators/clamping/clamping_max_force.pynewton/_src/actuators/clamping/clamping_position_based.pynewton/_src/actuators/usd_parser.pynewton/actuators.pynewton/tests/test_actuators.py
✅ Files skipped from review due to trivial changes (1)
- newton/_src/actuators/init.py
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/actuators.py
- newton/_src/actuators/clamping/clamping_max_force.py
- newton/tests/test_actuators.py
|
|
||
| controller.finalize(device, self.num_actuators) | ||
|
|
||
| def get_param(self, name: str) -> wp.array | None: |
There was a problem hiding this comment.
Why do we need this method? It seems like a potential source of problems if the controller and clamping object share the same parameter name. The user should know from where to get the parameter, right?
| @@ -0,0 +1,158 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2025 The Newton Developers | |||
There was a problem hiding this comment.
All these new files should have 2026 as copyright year.
| as input and is expected to return torques in physical units. Scaling is left to the user. | ||
| """ | ||
|
|
||
| SHARED_PARAMS: ClassVar[set[str]] = {"network_path"} |
There was a problem hiding this comment.
🔴 SHARED_PARAMS is missing input_order and input_idx, which causes a RuntimeError during finalize().
resolve_arguments() returns input_order (a string like 'pos_vel') and input_idx (None) as non-shared params. Because they are not in SHARED_PARAMS, ModelBuilder treats them as per-DOF numeric data and passes them through _stack_args_to_arrays() with wp.float32. That blows up with:
RuntimeError: could not convert string to float: 'pos_vel'
Both values are configuration shared across all DOFs, not per-DOF arrays.
| SHARED_PARAMS: ClassVar[set[str]] = {"network_path"} | |
| SHARED_PARAMS: ClassVar[set[str]] = {"network_path", "input_order", "input_idx"} |
| raise ValueError("Delay requires 'delay' argument") | ||
| return {"delay": args["delay"]} | ||
|
|
||
| def __init__(self, delay: int): |
There was a problem hiding this comment.
🟡 delay=0 is accepted here but causes a ZeroDivisionError on the first actuator step.
update_state() computes (current_state.write_idx + 1) % self.delay, which divides by zero when self.delay == 0.
Either reject it early or treat zero as a pass-through:
| def __init__(self, delay: int): | |
| def __init__(self, delay: int): | |
| """Initialize delay. | |
| Args: |
Option A -- fail fast:
def __init__(self, delay: int):
if delay < 1:
raise ValueError(f"delay must be >= 1, got {delay}")Option B -- treat delay=0 as identity (skip the ring buffer entirely in update_state).
Either way, the current code will crash at runtime for any user who passes delay=0.
| component_specs (delay, clamping, etc.). | ||
| """ | ||
|
|
||
| controller_class: type |
There was a problem hiding this comment.
⚪ Nit: bare type loses useful type information here and on line 84.
Since controller_class is always a Controller subclass and component_specs entries are always Clamping or Delay subclasses, tighter annotations would give better IDE autocompletion and catch misuse earlier:
| controller_class: type | |
| controller_class: type[Controller] |
And on line 84:
component_specs: list[tuple[type[Clamping] | type[Delay], dict[str, Any]]] = field(default_factory=list)
| """ | ||
| raise NotImplementedError(f"{cls.__name__} must implement resolve_arguments") | ||
|
|
||
| def modify_forces( |
There was a problem hiding this comment.
🟡 modify_forces() silently does nothing when a subclass forgets to override it.
The pass body means dst_forces stays zeroed while src_forces is ignored, so a controller might compute the correct torque but the actuator applies zero force. This is inconsistent with resolve_arguments() on line 35, which already raises NotImplementedError.
Replace pass with raise NotImplementedError to match:
def modify_forces(self, src_forces, dst_forces, positions, velocities):
raise NotImplementedError(f"{type(self).__name__} must implement modify_forces")
|
🟡 Bare AGENTS.md requires bracket-syntax Warp array annotations ( Affected locations include:
Please update these to match the dtypes used in the corresponding kernel signatures. |
| class State: | ||
| """Circular buffer state for delayed targets.""" | ||
|
|
||
| buffer_pos: wp.array2d[float] = None |
There was a problem hiding this comment.
⚪ buffer_pos, buffer_vel, and buffer_act default to None but are annotated without | None.
Per PEP 604 and AGENTS.md, these should include the union type. This also matches the pattern already used in ControllerPID.State.integral and ControllerNetLSTM.State.
| buffer_pos: wp.array2d[float] = None | |
| buffer_pos: wp.array2d[float] | None = None |
Same change needed for buffer_vel on line 62 and buffer_act on line 63.
|
|
||
| def __init__( | ||
| self, | ||
| indices: wp.array, |
There was a problem hiding this comment.
In the original version of the library, we had both "input_indices" and "output_indices". I think we got rid of this because in the up-coming integration with Lab/Sim, we are:
- limiting ourselves to the case where we have one actuator per DOF of the robot, and
- we expect that the input array (list of joint measurements) will have the same structure as the output array (list of joint force outputs)
However, isn't collapsing those two lists into just "indices" a bit limiting on the library beyond Sim/Lab? That requires that every consumer of this library matches the structure of their input and output arrays, which may not always be the case.
For example, what if:
- the input array contains passive and active joint measurements, and
- the output array contains only actuated joint forces?
That is a perfectly reasonable way for a user to organize their data, but if the user only has indices then they are being blocked from doing things that way.
Thoughts @jvonmuralt @eric-heiden @preist-nvidia ?
There was a problem hiding this comment.
I was thinking to start it minimal for now and introduce output_indices as an optional parameter at a later iteration.
There was a problem hiding this comment.
I also have a concern if we have some D6 joint in the robot, then joint_q and joint_qd indexing differs. should it be a users responsibility to construct every step joint_q_per_dof so that library reads correct values. Or should we then accept pos_indices, vel_indices, output_indices in actuator and then newton state and control can be passed as they are (it would also imply changing control.joint_target_pos to be consistent with state.joint_q)?
There was a problem hiding this comment.
I see. So I guess we could say we have three options:
- Keep just
indices: simplest (and what we already have) but most restrictive on the user data. - Put back
input_indicesandoutput_indices: I think this will work for all 1DOF acuators, including those with tranmission, but is slightly more complex. - Every array gets its own index array: most flexible and seems needed for D6 joints, adds the most indexing complexity.
There was a problem hiding this comment.
I'm confused - D6 joints have the same number of q and qd coordinates, maybe @jvonmuralt means free and ball joints which have differing coordinate counts?
I also think we need some separation of q and qd indices because for floating-base articulations (or articulations which have ball joints somewhere) the q indices will differ from qd/"output" indices.
This isn't really a question of input vs. output indices, right? It is more about joint_q and joint_qd indices because an actuator controller could read inputs from both joint_q and joint_qd. state_pos_attr and state_vel_attr are the raw joint_q and joint_qd arrays in Newton so there are going to be indexing problems unless we provide q/qd indexing information.
So maybe we actually need 3 index arrays?
| Axis | What it indexes | Example where collapsing fails |
|---|---|---|
A. joint_q read index |
positions for controller feedback | free/ball joint shifts coord vs. DOF numbering |
B. joint_qd read index |
velocities for controller feedback | same joints — q and qd diverge |
C. joint_f write index |
where clamped force gets scattered | passive vs. actuated DOFs, tendon/coupled transmissions |
If an actuator actuates exactly the joints that it reads from then joint_qd and joint_f indexing are the same.
There was a problem hiding this comment.
yes, I meant free and ball joints:) and exactly the problem which you describe.
It looks like we would need 3 index arrays indeed.
I also think we would need to change control.joint_target_pos to be the same shape as joint_q, so that index array A can be used for reading both position state and target.
There was a problem hiding this comment.
The analysis table is great! It's a very good summary of cases where having just one index array will fail.
@jvonmuralt and @preist-nvidia , is the preference still to keep maximally simple on the first release and then add the other index arrays later?
Nitpick: I don't think we use the word "joint" except for on the output, where we are in fact always output joint forces.
On inputs, some actuators will depend on positions/velocities which are "actuator space" values, no always "joint-space" (i.e. length or speed of a retracting tendon).
| Args: | ||
| indices: DOF indices for reading state/targets and writing forces. Shape (N,). | ||
| controller: Controller that computes raw forces. | ||
| delay: Optional Delay instance for input delay. | ||
| clamping: List of Clamping objects (post-controller force bounds). | ||
| state_pos_attr: Attribute on sim_state for positions. | ||
| state_vel_attr: Attribute on sim_state for velocities. | ||
| control_target_pos_attr: Attribute on sim_control for target positions. | ||
| control_target_vel_attr: Attribute on sim_control for target velocities. | ||
| control_feedforward_attr: Attribute on sim_control for control input. None to skip. | ||
| control_output_attr: Attribute on sim_control for clamped output forces. | ||
| control_computed_output_attr: Attribute on sim_control for raw (pre-clamp) | ||
| forces. None to skip writing computed forces. |
There was a problem hiding this comment.
I would move these docstrings for constructor args to the constructor's docstring. That's at least what we have done in most other Newton classes.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
newton/_src/actuators/controllers/controller_net_lstm.py (1)
149-151:⚠️ Potential issue | 🟠 MajorUse
target_indicesvalues directly instead of array identity check.
target_indices is input_indicesonly works when the caller passes the exact same Warp array object. If arrays are equal-but-distinct, this will use_torch_sequential_indicesincorrectly.♻️ Proposed fix
- torch_target_idx = ( - self._torch_input_indices if target_indices is input_indices else self._torch_sequential_indices - ) + # Convert target_indices to torch tensor for correct indexing + torch_target_idx = torch.tensor(target_indices.numpy(), dtype=torch.long, device=self._torch_device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_lstm.py` around lines 149 - 151, The current selection of torch_target_idx uses an identity check (target_indices is input_indices) which fails for equal-but-distinct arrays; change the branch in controller_net_lstm.py to compare the values/content of target_indices and input_indices (e.g., element-wise equality or numpy.array_equal/torch.equal) rather than object identity, and then assign torch_target_idx to self._torch_input_indices when the arrays are equal by value, otherwise use self._torch_sequential_indices; update the logic around target_indices, input_indices, _torch_input_indices and _torch_sequential_indices accordingly.newton/_src/actuators/usd_parser.py (1)
176-179:⚠️ Potential issue | 🟡 MinorError message shows
GetAppliedSchemas()which won't include custom schemas.Per lines 110-112, custom schemas like
NewtonControllerPDAPIare not returned byGetAppliedSchemas()since they're not registered USD schema types. The error message is misleading for debugging.if controller_class is None: raise ValueError( - f"Actuator prim has no controller schema applied (applied schemas: {prim.GetAppliedSchemas()})" + f"Actuator prim '{prim.GetPath()}' has no controller schema applied (found schemas from metadata: {schemas})" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/usd_parser.py` around lines 176 - 179, The error message is misleading because prim.GetAppliedSchemas() does not include custom/unregistered API schemas; update the ValueError to include both prim.GetAppliedSchemas() and the prim-level metadata that lists applied API schemas (e.g., prim.GetMetadata('apiSchemas') or equivalent) and also include the resolved controller_class name (controller_class) so the message shows both standard applied schemas and any custom API schemas for easier debugging.newton/_src/actuators/controllers/controller_net_mlp.py (1)
137-139:⚠️ Potential issue | 🟠 MajorUse
target_indicesvalues directly instead of array identity check.Same issue as in
ControllerNetLSTM:target_indices is input_indicesonly works when the caller passes the exact same Warp array object.♻️ Proposed fix
- torch_target_idx = ( - self._torch_input_indices if target_indices is input_indices else self._torch_sequential_indices - ) + torch_target_idx = torch.tensor(target_indices.numpy(), dtype=torch.long, device=self._torch_device)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/controllers/controller_net_mlp.py` around lines 137 - 139, The code currently selects torch_target_idx using an identity check (target_indices is input_indices) which fails when callers pass equivalent but distinct arrays; change the selection to compare values instead of object identity by checking content equality (e.g., using an element-wise equality helper such as torch.equal/torch.allclose or numpy.array_equal depending on the tensor/array type) between target_indices and input_indices, and then set torch_target_idx to self._torch_input_indices or self._torch_sequential_indices accordingly; update the logic in controller_net_mlp.py where torch_target_idx is computed to use this value-equality check.
🧹 Nitpick comments (1)
newton/_src/actuators/delay.py (1)
89-89: Use bracketed Warp array annotation for_indices.Per coding guidelines, Warp arrays should use bracket syntax with dtype. This also aligns with the reviewer's request for typed array annotations.
- self._indices: wp.array | None = None + self._indices: wp.array[wp.uint32] | None = NoneAs per coding guidelines: "Annotate Warp arrays with bracket syntax (
wp.array[wp.vec3],wp.array2d[float],wp.array[Any]), not the parenthesized form."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/actuators/delay.py` at line 89, The instance attribute annotation self._indices currently uses the parenthesized form "wp.array" — update it to the bracketed Warp array syntax with an explicit dtype, e.g. change "self._indices: wp.array | None" to "self._indices: wp.array[Any] | None" (or a more specific dtype such as wp.array[int32] | None if these are integer indices) inside the class where _indices is defined so the annotation follows the required wp.array[...] form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/actuators/actuator.py`:
- Line 200: The current check (has_states = current_act_state is not None and
next_act_state is not None) allows stepping a stateful actuator without both
state objects, causing None dereferences in ControllerPID and stalled delay
buffers; modify step() so that for any actuator with a controller state or
nonzero delay (e.g., check self.controller is not None or self.delay > 0 or
whatever stateful flags your Actuator exposes) the code asserts or raises
immediately if either current_act_state or next_act_state is None, and apply the
same fail-fast check in the other state-handling blocks (the sections around the
existing has_states use and the ranges you mentioned) so the compute path never
executes when required state objects are missing.
- Line 125: Update the get_param method signature to use bracketed Warp typing
by changing its return annotation from "wp.array | None" to "wp.array[Any] |
None" in the get_param function; also ensure Any is imported (e.g., from typing
import Any) at the top of the module so the annotation resolves correctly.
- Around line 17-31: The kernel _scatter_add_kernel currently does non-atomic
accumulation (output[idx] = output[idx] + forces[i]) which causes race
conditions; replace both non-atomic writes with wp.atomic_add calls: use
wp.atomic_add(output, idx, forces[i]) for the primary accumulation and, inside
the existing computed_output guard, use wp.atomic_add(computed_output, idx,
computed_forces[i]); ensure you preserve the existing conditional on
computed_output and keep the same argument types (forces, computed_forces,
indices) and kernel signature.
In `@newton/_src/actuators/clamping/clamping_position_based.py`:
- Around line 93-98: The lookup tables self.lookup_angles and
self.lookup_torques are created on Warp's current default device, which can
mismatch the device used later in modify_forces and cause kernel failures;
update modify_forces (or add a set_device(self, device) helper) to check the
target device parameter and migrate these arrays when needed (e.g., compare
array.device to the passed device and recreate or call Warp's device-migration
API to move lookup_angles and lookup_torques onto that device before launching
kernels) so the arrays are always on the same device as the kernel.
In `@newton/_src/actuators/controllers/controller_net_lstm.py`:
- Around line 44-46: The reset method on the State class calls
self.hidden.new_zeros(...) and self.cell.new_zeros(...) unconditionally which
will raise AttributeError when hidden or cell is None; update State.reset to
guard these attributes (e.g., if self.hidden is not None: self.hidden =
self.hidden.new_zeros(self.hidden.shape) and similarly for self.cell) or
alternatively remove None from the type hints if state() guarantees non-None;
modify the reset implementation (referencing the reset method and attributes
hidden and cell) to perform these None-checks so it no longer dereferences None.
In `@newton/_src/actuators/controllers/controller_net_mlp.py`:
- Around line 37-39: State.reset() unconditionally calls
self.pos_error_history.zero_() and self.vel_history.zero_() but the attributes
are annotated as optional, causing AttributeError if they are None; update
reset() in the State class to guard these calls (e.g., if self.pos_error_history
is not None: self.pos_error_history.zero_() and similarly for self.vel_history)
or, if invariants guarantee they are never None after state() initialization,
remove the "| None" from the pos_error_history and vel_history type hints and
add an assertion in state()/constructor to enforce non-None.
In `@newton/_src/actuators/controllers/controller_pid.py`:
- Around line 76-83: The resolve_arguments method currently allows negative
integral_max which will cause the integral anti-windup clamp (used by the PID
controller, referenced by wp.clamp in this class) to receive an invalid range;
update resolve_arguments to validate that args.get("integral_max") is not
negative (reject negatives by raising ValueError with a clear message) and keep
the same default math.inf, and add the same validation in the controller's
__init__ (e.g., in ControllerPID.__init__) to guard against callers bypassing
resolve_arguments; ensure both places either coerce invalid values or raise so
integral_max is always >= 0.0.
---
Duplicate comments:
In `@newton/_src/actuators/controllers/controller_net_lstm.py`:
- Around line 149-151: The current selection of torch_target_idx uses an
identity check (target_indices is input_indices) which fails for
equal-but-distinct arrays; change the branch in controller_net_lstm.py to
compare the values/content of target_indices and input_indices (e.g.,
element-wise equality or numpy.array_equal/torch.equal) rather than object
identity, and then assign torch_target_idx to self._torch_input_indices when the
arrays are equal by value, otherwise use self._torch_sequential_indices; update
the logic around target_indices, input_indices, _torch_input_indices and
_torch_sequential_indices accordingly.
In `@newton/_src/actuators/controllers/controller_net_mlp.py`:
- Around line 137-139: The code currently selects torch_target_idx using an
identity check (target_indices is input_indices) which fails when callers pass
equivalent but distinct arrays; change the selection to compare values instead
of object identity by checking content equality (e.g., using an element-wise
equality helper such as torch.equal/torch.allclose or numpy.array_equal
depending on the tensor/array type) between target_indices and input_indices,
and then set torch_target_idx to self._torch_input_indices or
self._torch_sequential_indices accordingly; update the logic in
controller_net_mlp.py where torch_target_idx is computed to use this
value-equality check.
In `@newton/_src/actuators/usd_parser.py`:
- Around line 176-179: The error message is misleading because
prim.GetAppliedSchemas() does not include custom/unregistered API schemas;
update the ValueError to include both prim.GetAppliedSchemas() and the
prim-level metadata that lists applied API schemas (e.g.,
prim.GetMetadata('apiSchemas') or equivalent) and also include the resolved
controller_class name (controller_class) so the message shows both standard
applied schemas and any custom API schemas for easier debugging.
---
Nitpick comments:
In `@newton/_src/actuators/delay.py`:
- Line 89: The instance attribute annotation self._indices currently uses the
parenthesized form "wp.array" — update it to the bracketed Warp array syntax
with an explicit dtype, e.g. change "self._indices: wp.array | None" to
"self._indices: wp.array[Any] | None" (or a more specific dtype such as
wp.array[int32] | None if these are integer indices) inside the class where
_indices is defined so the annotation follows the required wp.array[...] form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 85b2ef70-1a33-4243-972c-6282d85f1e4a
📒 Files selected for processing (18)
docs/concepts/actuators.rstdocs/index.rstnewton/_src/actuators/__init__.pynewton/_src/actuators/actuator.pynewton/_src/actuators/clamping/__init__.pynewton/_src/actuators/clamping/base.pynewton/_src/actuators/clamping/clamping_dc_motor.pynewton/_src/actuators/clamping/clamping_max_force.pynewton/_src/actuators/clamping/clamping_position_based.pynewton/_src/actuators/controllers/__init__.pynewton/_src/actuators/controllers/base.pynewton/_src/actuators/controllers/controller_net_lstm.pynewton/_src/actuators/controllers/controller_net_mlp.pynewton/_src/actuators/controllers/controller_pd.pynewton/_src/actuators/controllers/controller_pid.pynewton/_src/actuators/delay.pynewton/_src/actuators/usd_parser.pynewton/tests/test_actuators.py
✅ Files skipped from review due to trivial changes (6)
- docs/index.rst
- newton/_src/actuators/controllers/init.py
- newton/_src/actuators/clamping/init.py
- newton/_src/actuators/init.py
- docs/concepts/actuators.rst
- newton/_src/actuators/controllers/controller_pd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/actuators/clamping/clamping_max_force.py
|
Schemas for this are ready for review here. Once that merges the parsing here should be updated to align. Or we can merge this first & align in a followup PR. |
| state_vel_attr: str = "joint_qd", | ||
| control_target_pos_attr: str = "joint_target_pos", | ||
| control_target_vel_attr: str = "joint_target_vel", | ||
| control_feedforward_attr: str | None = "joint_act", |
There was a problem hiding this comment.
@jvonmuralt question:
Is the feedforward intended to be "post-transmission" (once we have transmission)? I.e., forces forwarded directly to the joints?
In the future, should we also have a "pre-transmission" feed-forward? This would cover the case we discussed yesterday in slack:
- A user has a single tendon which is routed such that it transmits forces 3 joints
- They just want to apply a constant force through the tendon
I.e., more generally they could run a full control-law externally, and then just forward the actuator-space commands to their actuators.
There was a problem hiding this comment.
We can discuss it further when we are working on adding transmission.
But I would say we should not need to have 2 feedforward terms and in the general case it is going to be pre-transmission. It is just joint actuators where we dont model transmission and assume everything to be in the joint space.
| Args: | ||
| actuator: Actuator instance (used for DOF index mapping). | ||
| component: The component that owns the parameter — a | ||
| :class:`~newton._src.actuators.controllers.base.Controller`, |
There was a problem hiding this comment.
AGENTS.md: "Sphinx cross-refs… Prefer public API paths; never use newton._src." These new docstrings reference :class:~newton._src.actuators.controllers.base.Controller`` etc. The public paths are newton.actuators.Controller / `newton.actuators.Clamping` / `newton.actuators.Delay`. Can we route through those instead? (Same check worth running over all the new files — I spot-checked the actuator module itself and it already uses `newton.actuators.*`, so it's just this pair of selection docstrings I noticed.)
| self.pos_error_history[:, t] = 0.0 | ||
| self.vel_history[:, t] = 0.0 | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
resolve_arguments() returns only network_path, but the __init__ signature has five more user-tunable params (input_order, input_idx, pos_scale, vel_scale, torque_scale). Because the builder does
resolved_ctrl = controller_class.resolve_arguments(controller_kwargs)
...
controller = entry.controller_class(**ctrl_arrays, **entry.controller_shared_kwargs)any of those kwargs passed through ModelBuilder.add_actuator(ControllerNetMLP, ...) (or via USD) will be silently dropped and the controller will use the defaults — including pos_scale=vel_scale=torque_scale=1.0, which is often not what a trained network needs.
Two options I can see:
- Include them in
resolve_arguments()and markinput_idx/input_orderasSHARED_PARAMS(they don't vary per-DOF anyway). - Document explicitly that programmatic construction is required and raise if these kwargs are passed through
add_actuator.
Either way, silent dropping of scale factors feels like a footgun. Same comment applies in spirit to ControllerNetLSTM.resolve_arguments, though that one has fewer tunables.
Related: the comment in usd_parser.py:67-68 ("input_order / input_idx are intentionally left out of the schema; they should be set programmatically") acknowledges the issue for USD but the programmatic add_actuator path has the same hole.
| for clamp in self.clamping: | ||
| clamp.finalize(self.device, self.num_actuators) | ||
|
|
||
| @property |
There was a problem hiding this comment.
Actuator.SHARED_PARAMS is an instance property, but the component-level Controller.SHARED_PARAMS / Clamping.SHARED_PARAMS are ClassVar[set[str]]. The builder relies on reading it as a classvar (getattr(controller_class, "SHARED_PARAMS", set())). Reading Actuator.SHARED_PARAMS on the class returns the property object, not a set — so this method would silently break the same grouping logic if someone ever reached for it as a group key.
grep shows nothing else in the repo actually calls Actuator.SHARED_PARAMS. Is this still needed, or can it be dropped? (Noting @eric-heiden raised a related question in the existing thread — this is asking the same thing from the "consistency with the rest of the system" angle.)
| self.buffer_act.zero_() | ||
| self.num_pushes.zero_() | ||
| self.write_idx = self.buffer_pos.shape[0] - 1 | ||
| else: |
There was a problem hiding this comment.
Two small things on masked reset:
a. The loop launches _delay_masked_reset_kernel three times (one per buffer), and each launch zeros num_pushes[i] where mask is true. That's redundant — the kernel takes num_pushes as output. Either pass num_pushes on only one launch, or have one kernel that zeros all three buffers and num_pushes once.
b. Related, Delay.__init__ accepts max_delay: int with no validation. ModelBuilder.add_actuator's delay path enforces delay >= 1, but Delay(max_delay=0) constructed directly would later divide by zero in update_state's (write_idx + 1) % self.buf_depth. The builder-side validation in resolve_arguments should probably move (or be mirrored) into Delay.__init__ so direct construction (the exact use case the concept doc advertises) doesn't have that trap.
| def __init__(self, gain: wp.array): | ||
| self.gain = gain | ||
|
|
||
| def compute(self, positions, velocities, target_pos, target_vel, |
There was a problem hiding this comment.
The example signature here is stale vs. Controller.compute in base.py:
def compute(self, positions, velocities, target_pos, target_vel,
feedforward, input_indices, target_indices, forces,
state, dt, device=None):The real signature uses pos_indices, vel_indices, target_pos_indices, target_vel_indices (four index arrays, not two). Anyone following this example to write a custom controller will get a TypeError on kernel launch. Could we update the example to match the 4-index layout?
| num_worlds = 3 | ||
| builder = newton.ModelBuilder() | ||
| builder.replicate(template, num_worlds) | ||
| class TestDelay(unittest.TestCase): |
There was a problem hiding this comment.
The test file is thorough on the "happy path" (PD/PID/DC-motor/position-based/MLP/LSTM compute + full pipeline integration + selection API), which is great. Two gaps that match promises the code or docs make:
State.reset(mask=...)—Delay.State.reset,ControllerPID.State.reset,ControllerNetMLP.State.reset,ControllerNetLSTM.State.reset, andActuator.State.resetare all part of the public API but not exercised by a single test. The masked path is the most interesting (e.g. resetting one env in a vectorized batch) and is the exact kind of logic that silently does the wrong thing.- CUDA graph capture —
Actuator.is_graphable()is documented but there's nowp.ScopedCapture/wp.capture_launchtest confirming PD+clamping actually graph-captures end-to-end. Given that this is explicitly called out as a design goal in the concept doc, a smoke test would be valuable.
At minimum, a reset() test for Delay.State + ControllerPID.State with a mask feels low-cost and catches the kind of regression that would otherwise silently corrupt a training run.
Follow-up for a later PR (not blocking this one): a requires_grad=True end-to-end test validating that a differentiable chain produces non-None grads after backward(). The gradient-enabled scratch-buffer path in Actuator.__init__(requires_grad=...) has no coverage today, but testing it well requires thinking through what a differentiable-actuator API contract should look like — bigger than this PR.
|
|
||
| controller_class: type[Controller] | ||
| controller_kwargs: dict[str, Any] = field(default_factory=dict) | ||
| component_specs: list[tuple[type[Clamping | Delay], dict[str, Any]]] = field(default_factory=list) |
There was a problem hiding this comment.
The dataclass keeps target_paths: list[str] "for future multi-target support" (comment in # Currently only a single target is supported.) and then parse_actuator_prim raises if len != 1. Meanwhile import_usd.py:2769 also re-validates len == 1. Two questions:
- Should this just be
target_path: struntil multi-target is actually a thing? The list shape leaks into callers (seeimport_usd.py) and is easy to misuse. - If we keep it as list for forward-compat, is it worth making
SCHEMA_REGISTRYmutable/extensible via a public hook? Right now adding a custom controller schema requires monkey-patching a module-level dict; aregister_schema()function would be a cleaner extension point.
| @@ -0,0 +1,237 @@ | |||
| .. SPDX-FileCopyrightText: Copyright (c) 2026 The Newton Developers | |||
There was a problem hiding this comment.
Per AGENTS.md, user-facing changes (especially Changed/Removed) require a CHANGELOG entry with migration guidance. This PR (a) deletes the newton-actuators dependency, (b) replaces the old ModelBuilder.add_actuator(actuator_class=..., input_indices=..., output_indices=...) signature with a very different add_actuator(controller_class, index, clamping=[...], delay=..., pos_index=..., **ctrl_kwargs) API, and (c) changes ArticulationView.get_actuator_parameter/set_actuator_parameter to require a component argument.
All three are breaking for any existing user. Could we add entries under Changed/Removed with explicit before/after snippets? e.g.
- Remove `ActuatorPD`/`ActuatorPID` etc. in favor of composed
`ControllerPD` + `ClampingMaxForce` etc. Migrate:
builder.add_actuator(ActuatorPD, input_indices=[i], kp=..., kd=..., max_force=...)
→
builder.add_actuator(ControllerPD, index=i, kp=..., kd=...,
clamping=[(ClampingMaxForce, {"max_force": ...})])
| Custom Attributes <concepts/custom_attributes> | ||
| Extended Attributes <concepts/extended_attributes> | ||
| Collisions and Contacts <concepts/collisions> | ||
| Actuators <concepts/actuators> |
There was a problem hiding this comment.
Don't forget to add api/newton_actuators to the API Reference toctree (further down in this file) and run docs/generate_api.py to generate the page. Per AGENTS.md — and without it, the :mod:newton.actuators / `:class:`ControllerPD / :meth:Actuator.state`` cross-refs in the new concept doc won't resolve.
| The per-step pipeline is: | ||
|
|
||
| .. code-block:: text | ||
|
|
There was a problem hiding this comment.
Per Newton docs convention (articulations.rst, collisions.rst), runnable Python examples in concept pages use .. testcode:: so they're exercised by the doctest builder and can't silently rot. This file uses .. code-block:: python for every Python example (lines 82, 104, 134, 146, 206) — none are executed.
Could we convert the first four (builder-level usage, manual construction, USD loading, selection API) to .. testcode:: with appropriate named groups so setup state carries across? The custom-controller example at line 206 has a stub body (...) that won't execute cleanly — either give it a minimal concrete toy body so testcode works, or keep that one as .. code-block:: with a note explaining why.
| history so the most recent data is returned. | ||
|
|
||
| **Controller** | ||
| Computes raw forces or torques from the current simulator state and control |
There was a problem hiding this comment.
Given my comments on the USD PR and having looked at PhysX perf envelope and lab API docs, should we standardize to effort to mean force/torque in this PR also?
Not going to comment further in this PR we can decide first then make a single refactor.
There was a problem hiding this comment.
yes. I can use the word effort
| └── ClampingPositionBased (angle-dependent lookup table) | ||
|
|
||
| **Delay** | ||
| Optionally delays the control targets (e.g. position or velocity) by *N* |
There was a problem hiding this comment.
The question I had in the USD PR applies here too, does delay apply also to FF terms and should we therefore prefer the control inputs instead of specific target language?
I'm aware I initially suggested the more narrow control targets, sorry.
There was a problem hiding this comment.
yes, delay is applied to all the inputs.
|
|
||
| The delay always produces output. When the buffer is empty | ||
| (e.g. right after reset), the current targets are returned | ||
| directly. When underfilled, the lag is clamped to the |
There was a problem hiding this comment.
Not sure the underfill behavior matches lab where the docstring says: https://github.com/isaac-sim/IsaacLab/blob/4df6560e187f2cc66685b41b21b259f4485d0c22/source/isaaclab/isaaclab/utils/buffers/delay_buffer.py#L164
There was a problem hiding this comment.
Behavior is consistent, it is implemented differently ( lab first writes then reads) and we do the other way around but the resulting targets used in controller are the same.
| torques.append(float(parts[1])) | ||
| if not angles: | ||
| raise ValueError(f"Lookup table file is empty: {path}") | ||
| if any(v < 0 for v in torques): |
There was a problem hiding this comment.
Should there be an additional check on monotonicity?
| parsed = parse_actuator_prim(prim) | ||
| if parsed is None: | ||
| continue | ||
| if len(parsed.target_paths) != 1 or parsed.target_paths[0] not in path_to_dof: |
There was a problem hiding this comment.
In the schema we say: " Currently only the first target is supported" so we don't need to fail for != 1, just ignore additional ones.
The dof detection failure should raise I think.
0 targets may be a valid disable mechanism and should just ignore, but depends on whether we add an enable: https://github.com/newton-physics/newton-usd-schemas/pull/45/changes#r3100480607
| """Initialize DC motor saturation. | ||
|
|
||
| Args: | ||
| saturation_effort: Peak motor torque at stall [N·m]. Shape (N,). |
There was a problem hiding this comment.
Should this define allowable ranges? And then input-check as well?
| Args: | ||
| max_force: Per-actuator force limits [N or N·m]. Shape (N,). | ||
| """ | ||
| self.max_force = max_force |
There was a problem hiding this comment.
A goal was for this library to be usable standalone, e.g. outside Newton with PhysX. In that usage, I would not expect the builder -> resolve_arguments() -> constructor path to be followed, so some of the current input validation/defaulting would be bypassed.
Should we make the standalone Python construction path validated as well, e.g. via a public factory that applies resolve_arguments(), or by moving the relevant checks into the component construction path?
| Note: | ||
| For selection we assume that input_indices is 1D (one input per actuator), | ||
| For selection we assume that indices is 1D (one input per actuator), | ||
| not the general 2D case (multiple inputs per actuator) which is supported |
There was a problem hiding this comment.
What is the general 2D case mentioned here? We are limiting to SISO first right?
There was a problem hiding this comment.
cleaned this up, thanks!
There was a problem hiding this comment.
Are these float arrays an issue for int delay gather/scatter?
| def _get_actuator_attribute_array(self, actuator: "Actuator", name: str) -> wp.array: | ||
| """Get actuator parameter array shaped (world_count, dofs_per_world), zeros for non-actuated DOFs.""" | ||
| def get_actuator_parameter(self, actuator: "Actuator", component: Any, name: str) -> wp.array: | ||
| """Get actuator parameter values for actuators corresponding to this view's DOFs. |
There was a problem hiding this comment.
Can you expand on this docstring a bit? I don’t understand what “this view’s DOFs” means here.
Is this returning parameter values for all DOFs in the view, with non-actuated ones ignored/defaulted, or only for the subset of DOFs that this actuator instance actually applies effort to? If the latter, it would help to say that explicitly.
6 participants
Description
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Bug fix
Steps to reproduce:
Minimal reproduction:
New feature / API change
Summary by CodeRabbit
New Features
Chores