Update documentation of ArticulationView, MuJoCoSolver#459
Conversation
Signed-off-by: Eric Heiden <eheiden@nvidia.com>
Signed-off-by: Eric Heiden <eheiden@nvidia.com>
Signed-off-by: Eric Heiden <eheiden@nvidia.com>
Signed-off-by: Eric Heiden <eheiden@nvidia.com>
📝 WalkthroughWalkthroughDocumentation was updated and expanded across several modules. The MuJoCo solver's docstring was revised to correct import paths and add a new section with debugging guidance. The selection utility received detailed docstrings for key methods, including a note about notifying the solver after modifying model attributes. A reference in a solver base method docstring was updated to point to the correct module for notification flags. Changes
Estimated code review effort1 (~5 minutes) ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
newton/solvers/mujoco/solver_mujoco.py (1)
1027-1060: Valuable debugging section, but mention Mujoco ≥ 3 requirement
mujoco.viewer.launch_passiveis only available from MuJoCo ≥ 3.0.
Add a short note to prevent confusion for users pinned to MuJoCo 2.x.@@ - import mujoco - import mujoco.viewer + import mujoco # MuJoCo ≥ 3.0 + import mujoco.viewernewton/utils/selection.py (4)
545-554: Specify concrete return type in docstring
Returns:currently saysarray, which is vague and does not surface whether callers receive a plain Python list, NumPy array, or awp.array. Using the concrete type (wp.arrayorwp.indexedarray) helps downstream users and Sphinx to hyperlink correctly.- Returns: - array: The attribute values (dtype matches the attribute). + Returns: + wp.array | wp.indexedarray: Attribute values (same dtype as the + underlying attribute). Shape equals the number of articulations + selected by this view.
558-571: Docstring: clarifymasksignature and cross-reference
- The
maskparameter accepts a 1-Dwp.array[bool]of shape(self.count,). Stating this up-front avoids misuse (e.g. passing Python lists of mismatched length).- The Sphinx cross-reference should include the module path so that the link resolves (
newton.solvers.solver.SolverBaseinstead ofnewton.solvers.SolverBase) unless an__init__.pyre-export guarantees the shorter path.- mask (array): Mask of articulations in this ArticulationView (all by default). + mask (wp.array[bool], optional): Boolean mask of length + ``self.count`` selecting which articulations to update. If + ``None`` (default) all articulations in this view are affected.Please also double-check that the re-export exists; otherwise update the note to:
:meth:`newton.solvers.solver.SolverBase.notify_model_changed`
600-603: Use fully-qualified reference foreval_fkSphinx may not resolve ``:meth:
eval_fk``` when multipleeval_fk` symbols exist.
ConsiderCall :meth:`ArticulationView.eval_fk` …to avoid ambiguity.
702-708: Add dtype / shape detail formaskargumentThe parameter description for
maskomits expected dtype and shape. Align with the pattern used elsewhere in this file.- mask (array): Mask of articulations in this ArticulationView (all by default). + mask (wp.array[bool], optional): Boolean mask of length + ``self.count`` specifying which articulations to process. If + ``None`` (default) all articulations are updated.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/generated/core/newton.Mesh.rstis excluded by!**/generated/**docs/generated/renderers/newton.utils.SimRendererOpenGL.rstis excluded by!**/generated/**docs/generated/renderers/newton.utils.SimRendererUsd.rstis excluded by!**/generated/**
📒 Files selected for processing (3)
newton/solvers/mujoco/solver_mujoco.py(1 hunks)newton/solvers/solver.py(1 hunks)newton/utils/selection.py(3 hunks)
🧠 Learnings (1)
newton/solvers/mujoco/solver_mujoco.py (1)
Learnt from: Kenny-Vilella
PR: #398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
🧰 Additional context used
🧠 Learnings (1)
newton/solvers/mujoco/solver_mujoco.py (1)
Learnt from: Kenny-Vilella
PR: #398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (2)
newton/solvers/solver.py (1)
265-275: Docstring reference fixed – looks goodThe module path for the
NOTIFY_FLAG_*constants is now correctly documented asnewton.sim.flags. No implementation changes are required.newton/solvers/mujoco/solver_mujoco.py (1)
1020-1021: Import Path VerifiedMuJoCoSolver is correctly re-exported in
newton/solvers/__init__.pyvia:from .mujoco import MuJoCoSolver __all__ = [ "FeatherstoneSolver", "MuJoCoSolver", … ]The import
newton.solvers.MuJoCoSolverwill resolve as expected. No changes required.
Allows cameras to be enabled when XR mode is active. By default, the record_demos.py script will strip envs of any cameras if xr mode is used. If `--enable_cameras` is passed as an argument to the script, externals cameras will remain and function. <!-- Thank you for your interest in sending a pull request. Please make sure to check the contribution guidelines. Link: https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html --> Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. Fixes newton-physics#2491 <!-- As a practice, it is recommended to open an issue to have discussions on the proposed pull request. This makes it easier for the community to keep track of what is being developed or added, and if a given feature is demanded by more than one party. --> <!-- As you go through the list, delete the ones that are not applicable. --> - New feature (non-breaking change which adds functionality) Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task -->
Description
Fixes #306 and #422.
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this MR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit