Add Viser viewer, tutorial notebook #1298
Conversation
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
📝 WalkthroughWalkthroughAdds a new Viser-based viewer backend (ViewerViser) to the public API, Jupyter/Sphinx environment detection helpers, Viser web client static assets, notebook tutorials (nbsphinx), a CORS-aware local docs server, and related docs, CI, packaging, and example wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Sphinx
participant nbsphinx
participant Notebook
participant ViewerViser
participant Viser
participant DocsServer
Sphinx->>nbsphinx: Build + execute notebooks
nbsphinx->>Notebook: Run tutorial cells
Notebook->>ViewerViser: Instantiate & log geometry/frames
ViewerViser->>Viser: Initialize/open browser server (live) or create .viser recording
ViewerViser->>ViewerViser: Detect environment (Jupyter vs Sphinx)
alt Static playback (Sphinx)
ViewerViser->>ViewerViser: save_recording() -> .viser file
ViewerViser->>DocsServer: request serving of .viser + client assets
DocsServer-->>nbsphinx: Serve static playback HTML with CORS/MIME
else Live Jupyter
ViewerViser->>Notebook: Embed iframe pointing to live Viser server
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
🧹 Nitpick comments (7)
newton/_src/viewer/viewer.py (1)
1321-1328: Narrow the exception handler and consider logging.Catching bare
Exceptionsilently swallows all errors including unexpected ones. Consider catching specific exceptions or at minimum logging the failure for debugging purposes.🔎 Suggested fix
# Check call stack for sphinx-related frames try: import traceback for frame_info in traceback.extract_stack(): if "sphinx" in frame_info.filename.lower() or "nbsphinx" in frame_info.filename.lower(): return True - except Exception: - pass + except (OSError, AttributeError): + # Stack inspection may fail in some edge cases; continue with False + pass return Falsedocs/serve.py (1)
47-68: LGTM with optional style improvement.The handler correctly provides MIME types needed for WebAssembly and JavaScript modules. The static analysis suggests adding
ClassVarannotation for the mutable class attribute, but this is a minor style concern for this internal development tool.If you want to address the linter warning:
🔎 Optional ClassVar annotation
+from typing import ClassVar + class CORSHTTPRequestHandler(http.server.SimpleHTTPRequestHandler): """HTTP handler with proper MIME type support and CORS headers.""" # Explicit extensions map for strict MIME type checking - extensions_map = { + extensions_map: ClassVar[dict[str, str]] = { ".wasm": "application/wasm", # ... rest of map }docs/tutorials/00_introduction.ipynb (2)
443-466: Rename unused loop variable.The loop variable
frameis not used within the loop body. Convention is to prefix with underscore to indicate it's intentionally unused.Suggested fix
-for frame in range(num_frames): +for _frame in range(num_frames):
38-53: Consider adding installation guidance for the pxr (OpenUSD) dependency.The notebook imports
from pxr import Usd, UsdGeomwhich requires the OpenUSD Python bindings. Users may encounter import errors if this isn't installed. Consider adding a note about this dependency in the setup section.newton/_src/viewer/viewer_viser.py (3)
194-199: Consider catching a more specific exception.The bare
except Exception: passcould mask unexpected errors during scene handle removal. Consider catching a more specific viser exception if one exists, or at minimum logging at debug level.Example with debug logging
if name in self._scene_handles: try: self._scene_handles[name].remove() - except Exception: - pass + except Exception as e: + # Scene handle may already be removed or invalid + import logging + logging.debug(f"Failed to remove scene handle {name}: {e}")
300-319: Move return statement outside the try block.The
returnon line 311 is inside thetryblock. If the updates succeed, the return should be in anelseblock to clearly separate the success path from exception handling.Suggested restructure
else: # Update transforms in-place try: handle.batched_positions = positions handle.batched_wxyzs = quats_wxyz handle.batched_scales = batched_scales # Only update colors if they were explicitly provided if batched_colors is not None: handle.batched_colors = batched_colors # Cache the colors for future reference self._instances[name]["colors"] = batched_colors - return except Exception: # If update fails, recreate the mesh try: handle.remove() except Exception: pass del self._scene_handles[name] del self._instances[name] + else: + return
407-407: Remove redundant local import.
Pathis already imported at module level (line 16). The local import is unnecessary.Fix
- from pathlib import Path # noqa: PLC0415 - data = self._serializer.serialize()
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/_static/viser/Inter-VariableFont_slnt,wght.ttfis excluded by!**/*.ttfdocs/_static/viser/assets/Sorter-Df0J3ZWJ.wasmis excluded by!**/*.wasmdocs/_static/viser/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
docs/_static/viser/assets/SplatSortWorker-DiSpcAPr.jsdocs/_static/viser/assets/WebsocketServerWorker-C6PJJ7Dx.jsdocs/_static/viser/assets/__vite-browser-external-BIHI7g3E.jsdocs/_static/viser/assets/index-BVvA0mmR.cssdocs/_static/viser/assets/index-H4DT9vxj.jsdocs/_static/viser/index.htmldocs/_static/viser/manifest.jsondocs/_static/viser/robots.txtdocs/api/newton_viewer.rstdocs/conf.pydocs/guide/development.rstdocs/guide/tutorials.rstdocs/serve.pydocs/tutorials/00_introduction.ipynbnewton/_src/viewer/__init__.pynewton/_src/viewer/viewer.pynewton/_src/viewer/viewer_rerun.pynewton/_src/viewer/viewer_viser.pynewton/viewer.pypyproject.toml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-25T21:44:17.047Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: docs/concepts/sensors.rst:12-12
Timestamp: 2025-12-25T21:44:17.047Z
Learning: Future API standardization: The sensor.update() method should be standardized to a single-argument form update(state) across all sensor types. The redundant model parameter will be removed from sensors like SensorFrameTransform. In code reviews, check for API changes that move toward a single-argument update, ensure all sensor implementations conform, and update user-facing docs to reflect the change. If reviewing related code, verify that calls using the old two-argument form are refactored and that any dependent serialization or tests are updated accordingly.
Applied to files:
docs/api/newton_viewer.rstdocs/guide/development.rstdocs/guide/tutorials.rst
📚 Learning: 2025-08-14T17:38:36.106Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/__init__.py:25-29
Timestamp: 2025-08-14T17:38:36.106Z
Learning: The Newton project prefers incremental __all__ building using __all__ += [...] pattern to group exports with their related imports, rather than a single consolidated __all__ at the end of the file.
Applied to files:
newton/viewer.py
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynbdocs/guide/tutorials.rst
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The STYLE3D solver in Newton is a new solver that did not exist in warp.sim, so it does not require any migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
🧬 Code graph analysis (2)
newton/viewer.py (1)
newton/_src/viewer/viewer_viser.py (1)
ViewerViser(28-764)
newton/_src/viewer/__init__.py (1)
newton/_src/viewer/viewer_viser.py (1)
ViewerViser(28-764)
🪛 Biome (2.1.2)
docs/_static/viser/assets/WebsocketServerWorker-C6PJJ7Dx.js
[error] 1-1: Do not use the e variable name as a label
The variable is declared here
Creating a label with the same name as an in-scope variable leads to confusion.
(lint/suspicious/noLabelVar)
docs/_static/viser/assets/SplatSortWorker-DiSpcAPr.js
[error] 1-1: $$ is assigned to itself.
This is where is assigned.
Self assignments have no effect and can be removed.
(lint/correctness/noSelfAssign)
[error] 8-8: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 11-11: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'v'. Consider to delete it or rename it.
'v' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'f'. Consider to delete it or rename it.
'f' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Actions: Pull Request
docs/tutorials/00_introduction.ipynb
[error] 1-1: PandocMissing in tutorials/00_introduction.ipynb: Pandoc wasn't found. Please install pandoc. Command: uv run --extra docs --extra sim sphinx-build -W -b html docs docs/_build/html
newton/_src/viewer/viewer_rerun.py
[error] 1-1: AttributeError in tests: module 'newton._src.viewer.viewer_rerun' does not have the attribute '_is_jupyter_notebook'. Tests in test_viewer_rerun_init_args expect this helper; potential API change or removal of _is_jupyter_notebook.
🪛 Ruff (0.14.10)
newton/_src/viewer/viewer.py
1327-1328: try-except-pass detected, consider logging the exception
(S110)
1327-1327: Do not catch blind exception: Exception
(BLE001)
docs/serve.py
51-68: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
docs/tutorials/00_introduction.ipynb
82-82: f-string without any placeholders
Remove extraneous f prefix
(F541)
116-116: f-string without any placeholders
Remove extraneous f prefix
(F541)
170-170: Loop control variable frame not used within loop body
Rename unused frame to _frame
(B007)
188-188: Found useless expression. Either assign it to a variable or remove it.
(B018)
newton/_src/viewer/viewer_viser.py
50-50: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
160-160: Unused method argument: normals
(ARG002)
161-161: Unused method argument: uvs
(ARG002)
198-199: try-except-pass detected, consider logging the exception
(S110)
198-198: Do not catch blind exception: Exception
(BLE001)
216-216: Unused method argument: materials
(ARG002)
234-234: Avoid specifying long messages outside the exception class
(TRY003)
245-246: try-except-pass detected, consider logging the exception
(S110)
245-245: Do not catch blind exception: Exception
(BLE001)
296-297: try-except-pass detected, consider logging the exception
(S110)
296-296: Do not catch blind exception: Exception
(BLE001)
311-311: Consider moving this statement to an else block
(TRY300)
312-312: Do not catch blind exception: Exception
(BLE001)
316-317: try-except-pass detected, consider logging the exception
(S110)
316-316: Do not catch blind exception: Exception
(BLE001)
386-387: try-except-pass detected, consider logging the exception
(S110)
386-386: Do not catch blind exception: Exception
(BLE001)
405-405: Avoid specifying long messages outside the exception class
(TRY003)
407-407: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
434-435: try-except-pass detected, consider logging the exception
(S110)
434-434: Do not catch blind exception: Exception
(BLE001)
530-531: try-except-pass detected, consider logging the exception
(S110)
530-530: Do not catch blind exception: Exception
(BLE001)
601-601: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
603-603: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
626-628: Avoid specifying long messages outside the exception class
(TRY003)
672-672: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
673-673: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
674-674: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
678-678: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
682-682: Avoid specifying long messages outside the exception class
(TRY003)
688-691: Avoid specifying long messages outside the exception class
(TRY003)
708-724: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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 Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (25)
pyproject.toml (1)
89-93: LGTM!The documentation dependencies are well-organized with clear comments explaining each package's purpose. The combination of
pypandocandpypandoc_binaryensures pandoc is available without requiring a system installation.newton/_src/viewer/viewer.py (2)
1279-1294: LGTM!The
is_jupyter_notebook()function correctly detects Jupyter environments by inspecting the IPython shell class name. TheNameErrorhandling forget_ipythonis appropriate.
1304-1304: This import is necessary and should be kept.The
osmodule is not imported at module level in this file. The local import at line 1304 is the only import ofosin the entire file and is required for theis_sphinx_build()function to work, as it usesos.environ.get()three times. Removing it would cause aNameErrorat runtime.Likely an incorrect or invalid review comment.
newton/_src/viewer/viewer_rerun.py (2)
120-120: LGTM!Good refactor to use the centralized
is_jupyter_notebook()utility. This improves maintainability by having a single source of truth for environment detection.
542-545: LGTM!The
legacy_notebook_showpath is correctly gated by checking both the flag and the Jupyter environment detection.docs/serve.py (2)
81-93: LGTM!The CORS headers including
Cross-Origin-Opener-PolicyandCross-Origin-Embedder-Policyare correctly configured for enablingSharedArrayBufferand other WebAssembly features required by Viser visualizations.
120-124: Good user experience with helpful error message.The check for missing build directory with guidance on how to build the docs is a nice touch for developer experience.
docs/guide/development.rst (1)
227-261: LGTM!Excellent documentation addition. The section clearly explains why the custom server is needed (MIME types and CORS for Viser), provides examples for both
uvandvenvworkflows consistent with the rest of the document, and includes a helpful note about why simpler alternatives won't work.newton/_src/viewer/__init__.py (1)
46-54: LGTM!The
ViewerViserimport and export follow the established pattern for other viewer backends in this module.docs/api/newton_viewer.rst (1)
17-17: LGTM!The addition of ViewerViser to the API documentation is correct and consistent with the existing viewer classes.
docs/_static/viser/robots.txt (1)
1-3: LGTM!Standard robots.txt configuration appropriate for public documentation static assets.
newton/viewer.py (1)
17-17: LGTM!The import and export of ViewerViser follow the established pattern for viewer classes in this module. The changes correctly expose the new viewer backend in the public API.
Also applies to: 25-25
docs/_static/viser/manifest.json (1)
1-15: Verify the manifest works correctly with the Viser static site.The manifest structure looks standard otherwise. Ensure that the favicon.svg file exists at the expected location and that the PWA behavior integrates correctly with the static Viser assets introduced in this PR.
docs/guide/tutorials.rst (1)
4-11: LGTM!The updated tutorial introduction and toctree directive properly integrate the new Jupyter notebook tutorial. The structure follows standard Sphinx documentation patterns.
docs/_static/viser/assets/__vite-browser-external-BIHI7g3E.js (1)
1-1: LGTM!Standard Vite-generated browser external shim. This placeholder module is expected in Vite builds to handle Node.js externals in browser contexts.
docs/_static/viser/index.html (1)
1-22: LGTM!Well-structured HTML5 shell for the Viser client with proper meta tags, manifest link, and module script loading. The relative paths align with the static asset structure.
docs/_static/viser/assets/SplatSortWorker-DiSpcAPr.js (1)
1-3: Vendored build artifact - no review required.This is minified/bundled JavaScript with WASM bindings generated from the Viser library build. The static analysis warnings (self-assignment, variable redeclarations) are artifacts of the minification process and are false positives for generated code.
docs/_static/viser/assets/WebsocketServerWorker-C6PJJ7Dx.js (1)
1-1: Vendored build artifact - no review required.This is minified/bundled JavaScript implementing the WebSocket client worker, generated from the Viser library build. The static analysis warning about label variable naming is an artifact of minification.
newton/_src/viewer/viewer_viser.py (7)
45-55: LGTM - Good lazy import pattern for optional dependency.The lazy import with clear error message guides users on how to install viser if missing.
66-129: LGTM!Well-structured initialization with clear documentation, optional share URL support, and conditional recording setup.
262-272: LGTM - Correct quaternion format conversion.The conversion from Warp's
[x, y, z, w]format to viser's[w, x, y, z]format is implemented correctly.
480-512: LGTM!Good handling of plane geometry with dynamic extent calculation and proper fallback to parent implementation for other geometry types.
696-700: LGTM - Correct ephemeral port allocation.The
find_free_port()function correctly uses a context manager to allocate an ephemeral port. The port is released when the socket closes and immediately reused by the HTTP server.
751-757: Background server uses daemon thread - will stop with kernel.The HTTP server runs in a daemon thread, which means it will automatically stop when the Jupyter kernel is stopped or restarted. This is the intended behavior for ephemeral notebook playback servers.
154-176: Unused parametersnormalsanduvsare intentional for interface compliance.The docstring correctly documents that these parameters are unused in the viser implementation. This maintains API compatibility with the
ViewerBaseinterface while the viser backend doesn't utilize vertex normals or UV coordinates.
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/conf.py (1)
95-96: Clarify the "auto" execution behavior in the comment.The comment "Configure notebook execution mode for nbsphinx" doesn't explain what
nbsphinx_execute = "auto"actually does. The "auto" mode executes notebooks only if they lack pre-existing outputs, which means:
- Build behavior varies depending on notebook state
- Notebooks without outputs will be executed in CI, adding to build time
- Requires all notebook dependencies to be available in the CI environment
Consider either:
- Updating the comment to explain the "auto" behavior explicitly
- Using
nbsphinx_execute = "never"if you prefer pre-executed notebooks for consistent, faster buildsVerify that all dependencies required by the tutorial notebooks are installed in the CI environment (check the
--extra docsand--extra simflags in the Sphinx build step cover everything needed).🔎 Suggested comment clarification
-# Configure notebook execution mode for nbsphinx -nbsphinx_execute = "auto" +# Execute notebooks during build only if they don't already have outputs +# This means notebooks should ideally be pre-executed and committed with outputs +nbsphinx_execute = "auto"Or if you prefer to never execute notebooks during build for reproducibility:
-# Configure notebook execution mode for nbsphinx -nbsphinx_execute = "auto" +# Don't execute notebooks during build (notebooks should be pre-executed with outputs) +nbsphinx_execute = "never"
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/sphinx.ymldocs/conf.py
⏰ 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 Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (4)
.github/workflows/sphinx.yml (1)
30-31: LGTM! Pandoc installation properly integrated.The pandoc installation step is correctly positioned before the Sphinx build and uses standard Ubuntu package management. This is necessary for nbsphinx to process Jupyter notebooks in the documentation.
docs/conf.py (3)
28-30: Environment variable setting is appropriate for build detection.Setting
NEWTON_SPHINX_BUILDat module import allows viewer components to detect the Sphinx build context, which is useful for adjusting behavior during documentation generation (e.g., static HTML exports vs. interactive viewers).
75-75: Appropriate extension addition for notebook support.Adding nbsphinx to the extensions list enables Jupyter notebook rendering in the documentation, aligning with the PR's objective to add tutorial notebooks.
98-102: Notebook timeout and error handling configured appropriately.The 600-second timeout provides adequate time for tutorial notebook execution, and
nbsphinx_allow_errors = Falsecorrectly ensures the build fails if notebooks contain errors, maintaining documentation quality.
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/tutorials/00_introduction.ipynb (1)
1-1: Verify Pandoc installation in CI.Based on past review comments, the Sphinx build previously failed due to missing Pandoc. While the PR appears to add Pandoc installation (referenced in commit messages), ensure the CI workflow properly installs Pandoc before running sphinx-build.
#!/bin/bash # Check if Pandoc installation is present in CI workflow files fd -e yml -e yaml . .github/workflows --exec cat {} \; | rg -i 'pandoc'
🧹 Nitpick comments (7)
newton/_src/viewer/viewer.py (1)
1299-1329: Remove unusednoqadirective on Line 1321.The
# noqa: PLC0415comment is unnecessary since PLC0415 is not enabled in your linter configuration.🔎 Proposed fix
try: - import traceback # noqa: PLC0415 + import traceback for frame_info in traceback.extract_stack():Note: The broad
Exceptioncatch andtry-except-passat lines 1326-1327 are acceptable here since this is an optional fallback check for Sphinx detection, and failures should be silently ignored.newton/_src/viewer/viewer_viser.py (6)
67-131: Consider validating the recording path parent directory.If
record_to_viseris provided but its parent directory doesn't exist and can't be created, the error won't surface untilsave_recording()is called much later. Consider validating this early in__init__.🔎 Proposed validation
# Recording state self._frame_dt = 0.0 self._record_to_viser = record_to_viser self._serializer = self._server.get_scene_serializer() if record_to_viser else None + + # Validate recording path early + if record_to_viser: + recording_path = Path(record_to_viser) + try: + recording_path.parent.mkdir(parents=True, exist_ok=True) + except OSError as e: + raise ValueError(f"Cannot create recording directory: {recording_path.parent}") from e
196-214: Overly broad exception handling.Catching bare
Exceptionat line 199 can mask unexpected errors. Consider catching specific exceptions (e.g.,RuntimeError,ValueError) that viser'sremove()method might raise.🔎 Proposed fix
# Remove existing mesh if present if name in self._scene_handles: try: self._scene_handles[name].remove() - except Exception: + except (RuntimeError, AttributeError): + # Handle removal failures gracefully (node may already be removed) pass
217-343: Multiple broad exception handlers mask errors.The
log_instancesmethod catches bareExceptionin multiple places (lines 246, 297, 313, 317). This can hide bugs and make debugging difficult. Consider:
- Catching specific exception types that viser methods raise
- Logging exceptions instead of silently suppressing them
- At minimum, document why broad catching is necessary
Note: The unused
materialsparameter (line 217) is required by the@overridecontract from the base class and cannot be removed.
625-649: Path manipulation logic is fragile.The string-based path parsing (finding
"_static/"in the path) could break if:
- Paths use inconsistent separators (though you normalize with
replace("\\", "/"))- Recording files are stored in nested directories containing
"_static/"in parent names- The relative path calculation assumes a fixed directory structure
Consider using
pathlib.Path.relative_to()oros.path.relpath()for more robust relative path computation:🔎 More robust approach
from pathlib import Path # Instead of string manipulation: recording_path = Path(self._record_to_viser).resolve() viser_index = Path("docs/_static/viser/index.html").resolve() try: # Calculate relative path from viser index to recording rel_path = recording_path.relative_to(viser_index.parent) playback_path = str(rel_path).replace("\\", "/") except ValueError: # If paths don't share a common base, fall back to your current logic ...
51-51: Remove unusednoqadirectives.Multiple lines have
# noqa: PLC0415comments that are unnecessary since PLC0415 (import outside top-level) is not enabled in your ruff configuration.Affected lines: 51, 408, 602, 604, 673, 674, 675, 679
🔎 Proposed cleanup
try: - import viser # noqa: PLC0415 + import viser cls._viser_module = viserApply similar changes to all flagged lines.
Also applies to: 408-408, 602-602, 604-604, 673-675, 679-679
698-751: Server thread lifecycle management concern.The HTTP server runs in a daemon thread (line 758) with no explicit shutdown mechanism. When the ViewerViser is closed or garbage collected, the daemon thread will be forcibly terminated, but the socket may not be cleanly released.
Consider:
- Storing the server reference so it can be shut down explicitly
- Using a non-daemon thread with proper shutdown in
close()- Adding context manager support for proper cleanup
🔎 Suggested improvement
# Store server for later cleanup self._recording_server = server self._recording_thread = server_thread # In close() method: def close(self): self._running = False try: self._server.stop() if self._serializer is not None: self.save_recording() # Shutdown recording playback server if active if hasattr(self, '_recording_server'): self._recording_server.shutdown() self._recording_thread.join(timeout=1.0) except Exception: pass
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/sphinx.yml.gitignore.pre-commit-config.yamldocs/serve.pydocs/tutorials/00_introduction.ipynbnewton/_src/viewer/viewer.pynewton/_src/viewer/viewer_viser.pypyproject.toml
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/serve.py
- .github/workflows/sphinx.yml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T23:32:48.670Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 660
File: pyproject.toml:54-59
Timestamp: 2025-08-27T23:32:48.670Z
Learning: In the newton project's pyproject.toml, self-referential extras (e.g., `examples = ["newton[sim]", "newton[importers]", ...]`) are normal and encouraged for dependency management, rather than inlining the dependencies.
Applied to files:
pyproject.toml
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
🪛 GitHub Actions: Pull Request
docs/tutorials/00_introduction.ipynb
[error] 1-1: Sphinx notebook build failed. NotebookError: AttributeError: module 'newton' has no attribute 'examples' in tutorials/00_introduction.ipynb during 'uv run --extra docs --extra sim sphinx-build -W -b html docs docs/_build/html'. Process completed with exit code 2.
🪛 Ruff (0.14.10)
newton/_src/viewer/viewer.py
1321-1321: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
1326-1327: try-except-pass detected, consider logging the exception
(S110)
1326-1326: Do not catch blind exception: Exception
(BLE001)
newton/_src/viewer/viewer_viser.py
51-51: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
161-161: Unused method argument: normals
(ARG002)
162-162: Unused method argument: uvs
(ARG002)
199-200: try-except-pass detected, consider logging the exception
(S110)
199-199: Do not catch blind exception: Exception
(BLE001)
217-217: Unused method argument: materials
(ARG002)
235-235: Avoid specifying long messages outside the exception class
(TRY003)
246-247: try-except-pass detected, consider logging the exception
(S110)
246-246: Do not catch blind exception: Exception
(BLE001)
297-298: try-except-pass detected, consider logging the exception
(S110)
297-297: Do not catch blind exception: Exception
(BLE001)
312-312: Consider moving this statement to an else block
(TRY300)
313-313: Do not catch blind exception: Exception
(BLE001)
317-318: try-except-pass detected, consider logging the exception
(S110)
317-317: Do not catch blind exception: Exception
(BLE001)
387-388: try-except-pass detected, consider logging the exception
(S110)
387-387: Do not catch blind exception: Exception
(BLE001)
406-406: Avoid specifying long messages outside the exception class
(TRY003)
408-408: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
435-436: try-except-pass detected, consider logging the exception
(S110)
435-435: Do not catch blind exception: Exception
(BLE001)
531-532: try-except-pass detected, consider logging the exception
(S110)
531-531: Do not catch blind exception: Exception
(BLE001)
602-602: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
604-604: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
627-629: Avoid specifying long messages outside the exception class
(TRY003)
673-673: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
674-674: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
675-675: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
679-679: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
683-683: Avoid specifying long messages outside the exception class
(TRY003)
689-692: Avoid specifying long messages outside the exception class
(TRY003)
docs/tutorials/00_introduction.ipynb
167-167: Found useless expression. Either assign it to a variable or remove it.
(B018)
⏰ 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 Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (4)
newton/_src/viewer/viewer.py (1)
1281-1296: LGTM! Clean environment detection.The Jupyter notebook detection logic correctly distinguishes between Jupyter/JupyterLab (
ZMQInteractiveShell) and IPython terminal environments (TerminalInteractiveShell).newton/_src/viewer/viewer_viser.py (1)
46-56: LGTM! Clean lazy import pattern.The classmethod lazy-loading of the viser module is a good practice to avoid requiring viser as a hard dependency.
pyproject.toml (2)
131-131: LGTM!Excluding JavaScript files from Ruff linting is appropriate given the new JS assets added for the Viser viewer under docs/_static/viser/assets.
219-219: LGTM!Excluding JavaScript files from typo checking is appropriate, especially for third-party or generated JS assets like those added for the Viser viewer.
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/_src/viewer/viewer_viser.py (2)
16-17: Redundant import.
Pathis imported at line 16 but also locally imported at line 401. The top-level import is sufficient.♻️ Remove redundant local import
- from pathlib import Path # noqa: PLC0415 - - data = self._serializer.serialize() + data = self._serializer.serialize()
186-190: Consider narrowing exception handling or adding logging.The broad
except Exception: passpattern is used consistently for cleanup operations. While acceptable for robustness, consider either:
- Catching more specific exceptions (e.g.,
AttributeError,KeyError)- Adding debug-level logging to aid troubleshooting
This applies to similar patterns throughout the file (lines 236, 287, 307, 377, 428, 524).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
newton/_src/viewer/viser/static/Inter-VariableFont_slnt,wght.ttfis excluded by!**/*.ttfnewton/_src/viewer/viser/static/assets/Sorter-Df0J3ZWJ.wasmis excluded by!**/*.wasmnewton/_src/viewer/viser/static/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
docs/conf.pydocs/guide/development.rstdocs/tutorials/00_introduction.ipynbnewton/_src/viewer/viewer_rerun.pynewton/_src/viewer/viewer_viser.pynewton/_src/viewer/viser/static/assets/SplatSortWorker-DiSpcAPr.jsnewton/_src/viewer/viser/static/assets/WebsocketServerWorker-C6PJJ7Dx.jsnewton/_src/viewer/viser/static/assets/__vite-browser-external-BIHI7g3E.jsnewton/_src/viewer/viser/static/assets/index-BVvA0mmR.cssnewton/_src/viewer/viser/static/assets/index-H4DT9vxj.jsnewton/_src/viewer/viser/static/index.htmlnewton/_src/viewer/viser/static/manifest.jsonnewton/_src/viewer/viser/static/robots.txt
✅ Files skipped from review due to trivial changes (3)
- newton/_src/viewer/viser/static/index.html
- newton/_src/viewer/viser/static/robots.txt
- newton/_src/viewer/viser/static/manifest.json
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/conf.py
- docs/guide/development.rst
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The STYLE3D solver in Newton is a new solver that did not exist in warp.sim, so it does not require any migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-07-15T19:00:39.991Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 394
File: newton/examples/example_ik_benchmark.py:132-146
Timestamp: 2025-07-15T19:00:39.991Z
Learning: In the Newton library, `newton.utils.download_asset()` already handles internal caching and can work with both local and remote assets, so conditional logic to check for local paths is unnecessary. The standard practice is to use `download_asset` directly rather than implementing custom path handling logic.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-12-03T09:33:27.083Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_cartpole.py:388-392
Timestamp: 2025-12-03T09:33:27.083Z
Learning: In the Kamino ModelBuilder class located at newton/_src/solvers/kamino/core/builder.py, the attributes `gravity`, `bodies`, `joints`, `collision_geoms`, and `physical_geoms` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.gravity`) rather than called as methods (e.g., `builder.gravity()`).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-27T23:32:48.670Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 660
File: pyproject.toml:54-59
Timestamp: 2025-08-27T23:32:48.670Z
Learning: In the newton project's pyproject.toml, self-referential extras (e.g., `examples = ["newton[sim]", "newton[importers]", ...]`) are normal and encouraged for dependency management, rather than inlining the dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T17:58:16.929Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/tests/test_ik.py:25-26
Timestamp: 2025-08-12T17:58:16.929Z
Learning: In the Newton physics engine codebase, it's acceptable for tests to import and use private `_src` internal modules and functions when needed for testing purposes, even though this breaks the public API boundary. This is an intentional project decision for test code.
Applied to files:
newton/_src/viewer/viewer_rerun.py
🪛 Biome (2.1.2)
newton/_src/viewer/viser/static/assets/SplatSortWorker-DiSpcAPr.js
[error] 1-1: $$ is assigned to itself.
This is where is assigned.
Self assignments have no effect and can be removed.
(lint/correctness/noSelfAssign)
[error] 8-8: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 11-11: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'v'. Consider to delete it or rename it.
'v' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'l'. Consider to delete it or rename it.
'l' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'f'. Consider to delete it or rename it.
'f' is defined here:
(lint/suspicious/noRedeclare)
newton/_src/viewer/viser/static/assets/WebsocketServerWorker-C6PJJ7Dx.js
[error] 1-1: Do not use the e variable name as a label
The variable is declared here
Creating a label with the same name as an in-scope variable leads to confusion.
(lint/suspicious/noLabelVar)
🪛 Ruff (0.14.10)
docs/tutorials/00_introduction.ipynb
165-165: Found useless expression. Either assign it to a variable or remove it.
(B018)
newton/_src/viewer/viewer_viser.py
51-51: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Unused method argument: normals
(ARG002)
152-152: Unused method argument: uvs
(ARG002)
189-190: try-except-pass detected, consider logging the exception
(S110)
189-189: Do not catch blind exception: Exception
(BLE001)
207-207: Unused method argument: materials
(ARG002)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
236-237: try-except-pass detected, consider logging the exception
(S110)
236-236: Do not catch blind exception: Exception
(BLE001)
287-288: try-except-pass detected, consider logging the exception
(S110)
287-287: Do not catch blind exception: Exception
(BLE001)
302-302: Consider moving this statement to an else block
(TRY300)
303-303: Do not catch blind exception: Exception
(BLE001)
307-308: try-except-pass detected, consider logging the exception
(S110)
307-307: Do not catch blind exception: Exception
(BLE001)
377-378: try-except-pass detected, consider logging the exception
(S110)
377-377: Do not catch blind exception: Exception
(BLE001)
399-399: Avoid specifying long messages outside the exception class
(TRY003)
401-401: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
428-429: try-except-pass detected, consider logging the exception
(S110)
428-428: Do not catch blind exception: Exception
(BLE001)
524-525: try-except-pass detected, consider logging the exception
(S110)
524-524: Do not catch blind exception: Exception
(BLE001)
598-598: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
600-600: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
623-625: Avoid specifying long messages outside the exception class
(TRY003)
669-669: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
670-670: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
671-671: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
676-676: Avoid specifying long messages outside the exception class
(TRY003)
681-684: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (3)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (20)
newton/_src/viewer/viser/static/assets/__vite-browser-external-BIHI7g3E.js (1)
1-1: LGTM - Standard Vite browser external shim.This is a standard Vite-generated module that exports an empty object to mock Node.js built-in modules in browser environments.
newton/_src/viewer/viewer_rerun.py (4)
25-25: LGTM - Shared utility import.Good refactoring to use the shared
is_jupyter_notebookutility from the viewer module, promoting consistency across viewer implementations.
120-120: LGTM - Consistent Jupyter detection.Using the shared
is_jupyter_notebook()function ensures consistent behavior across all viewer backends.
472-472: LGTM - Bug fix for hidden plane meshes.Good fix - the
hiddenparameter is now correctly forwarded tolog_mesh, ensuring plane meshes respect the visibility flag.
542-545: LGTM - Notebook show logic.The condition correctly uses
self.is_jupyter_notebook(set at init time) to determine the appropriate notebook display method.newton/_src/viewer/viser/static/assets/SplatSortWorker-DiSpcAPr.js (1)
1-20: Bundled viser Web Worker asset.This is a minified Web Worker module from the viser library for GPU-accelerated splat sorting. The static analysis warnings (self-assignment, variable redeclarations) are false positives caused by JavaScript minification and should be ignored for bundled third-party assets.
newton/_src/viewer/viser/static/assets/WebsocketServerWorker-C6PJJ7Dx.js (1)
1-1: Bundled viser WebSocket worker asset.This is a minified WebSocket client worker from the viser library handling real-time communication with the viser server. The static analysis warning about label naming is a false positive in minified code. This bundled asset should not be modified.
newton/_src/viewer/viewer_viser.py (7)
254-263: LGTM - Quaternion format conversion.The quaternion conversion from Warp's
(x, y, z, w)format to viser's(w, x, y, z)format is correctly implemented.
746-751: Daemon server thread for notebook playback.The HTTP server is correctly configured as a daemon thread, ensuring cleanup when the main process exits. Note that repeated calls to
_serve_viser_recordingwill spawn new servers on different ports - this is acceptable for notebook usage but could accumulate if called frequently.
574-649: LGTM - Comprehensive notebook integration.The
show_notebookmethod handles multiple contexts well:
- Live IFrame for non-recording mode
- Static HTML playback for recordings
- Proper path handling for Sphinx documentation builds
The Sphinx detection via
is_sphinx_build()and_static/path parsing is appropriate for the documentation workflow.
44-56: LGTM - Lazy viser import with helpful error message.Good pattern for optional dependencies - lazy loading with a clear installation instruction on ImportError.
67-131: LGTM - Well-documented constructor.The constructor properly initializes all components with sensible defaults and good documentation of parameters. The recording setup with
get_scene_serializer()is correctly conditional.
145-204: LGTM - Mesh logging implementation.The mesh logging correctly:
- Converts Warp arrays to NumPy
- Reshapes flat indices to (N, 3) triangles
- Handles scene handle cleanup for updates
- Respects the
hiddenparameterNote: The
normalsanduvsparameters are intentionally unused as documented ("unused in viser").
206-333: LGTM - Efficient instanced rendering.The implementation correctly:
- Uses viser's
add_batched_meshes_simplefor GPU-accelerated instancing- Supports in-place transform updates when instance count matches
- Falls back to recreation when instance count changes
- Handles colors with caching for updates without explicit colors
docs/tutorials/00_introduction.ipynb (6)
1-27: LGTM - Proper license and notebook setup.The license is correctly placed in a hidden raw cell with
"nbsphinx": "hidden"metadata, ensuring it won't appear in rendered documentation.
52-68: LGTM - Tutorial imports.The imports are appropriate for the tutorial scope. The
pxr.Usdimport requires USD to be installed, which aligns with the tutorial's mesh loading demonstration.
349-371: LGTM - Simulation function with state swapping.The
simulate()function correctly:
- Uses
globalfor state variables (required for CUDA graph capture to work with the same arrays)- Follows the standard Newton simulation loop pattern
- Swaps states after each substep
414-427: LGTM - Viewer setup for documentation.The recording path
docs/_static/recordings/00_introduction.viseris correctly structured for Sphinx documentation integration. Theverbose=Falseis appropriate for notebook output cleanliness.
443-471: LGTM - Complete simulation loop with visualization.The main loop correctly:
- Uses CUDA graph when available for performance
- Logs state and contacts each frame
- Manages frame timing
- Ends with
viewerto trigger_ipython_display_
473-511: LGTM - Comprehensive summary and next steps.The tutorial summary accurately reflects the content covered and provides clear guidance for further learning.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @docs/conf.py:
- Around line 106-115: The exclude_patterns list currently blocks notebooks by
including "tutorials/**/*.ipynb", preventing nbsphinx from processing
docs/tutorials/00_introduction.ipynb; remove that pattern from exclude_patterns
(the exclude_patterns variable in conf.py) or alternatively add an explicit
inclusion (e.g., add a toctree entry or a tutorials index that references
00_introduction.ipynb) so nbsphinx will include the notebook during build.
In @docs/tutorials/00_introduction.ipynb:
- Around line 203-206: Fix the typo in the comment string by changing the
heading "# Optinal: Run the simulation on CPU" to "# Optional: Run the
simulation on CPU" so the inline comment above use_cpu and wp.set_device reads
correctly; update the literal containing that comment in the same block where
use_cpu and wp.set_device are defined.
- Line 262: There's a spelling mistake in the documentation string "For other
available solvers and their features/strenghts, please refer to the [Solvers
feature overview]..." — change "strenghts" to "strengths" in that string (the
sentence containing "For other available solvers and their features/…") so the
link text reads "...features/strengths...".
🧹 Nitpick comments (1)
docs/tutorials/00_introduction.ipynb (1)
390-398: Recording path assumes notebook location context.The relative path
"../_static/recordings/..."depends on the notebook being executed from its expected location (docs/tutorials/). This should work correctly during the Sphinx documentation build, but may fail if users run the notebook from a different working directory.Consider using a more robust path resolution or adding a note for users who want to run the notebook independently.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/api/newton_solvers.rstdocs/conf.pydocs/tutorials/00_introduction.ipynb
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-12-25T21:44:17.047Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: docs/concepts/sensors.rst:12-12
Timestamp: 2025-12-25T21:44:17.047Z
Learning: Future API standardization: The sensor.update() method should be standardized to a single-argument form update(state) across all sensor types. The redundant model parameter will be removed from sensors like SensorFrameTransform. In code reviews, check for API changes that move toward a single-argument update, ensure all sensor implementations conform, and update user-facing docs to reflect the change. If reviewing related code, verify that calls using the old two-argument form are refactored and that any dependent serialization or tests are updated accordingly.
Applied to files:
docs/api/newton_solvers.rst
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The STYLE3D solver in Newton is a new solver that did not exist in warp.sim, so it does not require any migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-07-15T19:00:39.991Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 394
File: newton/examples/example_ik_benchmark.py:132-146
Timestamp: 2025-07-15T19:00:39.991Z
Learning: In the Newton library, `newton.utils.download_asset()` already handles internal caching and can work with both local and remote assets, so conditional logic to check for local paths is unnecessary. The standard practice is to use `download_asset` directly rather than implementing custom path handling logic.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-12-03T09:33:27.083Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_cartpole.py:388-392
Timestamp: 2025-12-03T09:33:27.083Z
Learning: In the Kamino ModelBuilder class located at newton/_src/solvers/kamino/core/builder.py, the attributes `gravity`, `bodies`, `joints`, `collision_geoms`, and `physical_geoms` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.gravity`) rather than called as methods (e.g., `builder.gravity()`).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2026-01-12T10:11:39.312Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py:6061-6091
Timestamp: 2026-01-12T10:11:39.312Z
Learning: Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py (ModelBuilder.finalize)
Learning: The project prefers to keep finalize() validation lightweight; do not add union-find/connected-components validation for loop joints. Current behavior (allow non-articulated joints when the child is in any articulation) is acceptable; avoid overengineering here.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-27T23:32:48.670Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 660
File: pyproject.toml:54-59
Timestamp: 2025-08-27T23:32:48.670Z
Learning: In the newton project's pyproject.toml, self-referential extras (e.g., `examples = ["newton[sim]", "newton[importers]", ...]`) are normal and encouraged for dependency management, rather than inlining the dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
🪛 Ruff (0.14.10)
docs/tutorials/00_introduction.ipynb
182-182: Found useless expression. Either assign it to a variable or remove it.
(B018)
⏰ 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 Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (5)
docs/conf.py (3)
28-30: LGTM!Setting the environment variable at module load time ensures it's available to all subprocesses, including Jupyter kernels spawned by nbsphinx.
93-103: LGTM!The nbsphinx configuration is appropriate:
autoexecution ensures notebooks are up-to-date- 600s timeout accommodates simulation workloads
- Disallowing errors maintains documentation quality
183-183: LGTM!Adding the Viser static assets path enables the documentation to serve the viewer components needed for interactive 3D visualizations embedded in the docs.
docs/api/newton_solvers.rst (1)
84-90: Documentation change is accurate; SolverVBD rigid body support is implemented.The update correctly reflects the implementation. The solver actively uses rigid body kernels (joints, body-body contacts, body-particle contacts) in its step method and is tested with cable and cloth simulations that include rigid body interactions.
docs/tutorials/00_introduction.ipynb (1)
429-430: Thelog_contactsmethod is fully supported by ViewerViser—remove the misleading comment.The method is implemented in
ViewerBaseand inherited byViewerViser, which overrides the supportinglog_linesmethod. The call will execute successfully and render contact lines. The comment "(not supported by the Viser viewer)" is outdated or misleading and should be removed to avoid confusing users.Likely an incorrect or invalid review comment.
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @docs/tutorials/00_introduction.ipynb:
- Around line 262-267: Fix the typo in the linked text of the introduction
notebook: change "strenghts" to "strengths" in the sentence that references the
"Solvers feature overview" (the link text/URL
https://newton-physics.github.io/newton/api/newton_solvers.html#supported-features)
inside docs/tutorials/00_introduction.ipynb so the sentence reads "...their
features/strengths, please refer to the [Solvers feature overview]...".
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.pre-commit-config.yamldocs/guide/development.rstdocs/tutorials/00_introduction.ipynb
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/guide/development.rst
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The STYLE3D solver in Newton is a new solver that did not exist in warp.sim, so it does not require any migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-07-15T19:00:39.991Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 394
File: newton/examples/example_ik_benchmark.py:132-146
Timestamp: 2025-07-15T19:00:39.991Z
Learning: In the Newton library, `newton.utils.download_asset()` already handles internal caching and can work with both local and remote assets, so conditional logic to check for local paths is unnecessary. The standard practice is to use `download_asset` directly rather than implementing custom path handling logic.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-12-03T09:33:27.083Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_cartpole.py:388-392
Timestamp: 2025-12-03T09:33:27.083Z
Learning: In the Kamino ModelBuilder class located at newton/_src/solvers/kamino/core/builder.py, the attributes `gravity`, `bodies`, `joints`, `collision_geoms`, and `physical_geoms` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.gravity`) rather than called as methods (e.g., `builder.gravity()`).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2026-01-12T10:11:39.312Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py:6061-6091
Timestamp: 2026-01-12T10:11:39.312Z
Learning: Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py (ModelBuilder.finalize)
Learning: The project prefers to keep finalize() validation lightweight; do not add union-find/connected-components validation for loop joints. Current behavior (allow non-articulated joints when the child is in any articulation) is acceptable; avoid overengineering here.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-27T23:32:48.670Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 660
File: pyproject.toml:54-59
Timestamp: 2025-08-27T23:32:48.670Z
Learning: In the newton project's pyproject.toml, self-referential extras (e.g., `examples = ["newton[sim]", "newton[importers]", ...]`) are normal and encouraged for dependency management, rather than inlining the dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
🪛 Ruff (0.14.10)
docs/tutorials/00_introduction.ipynb
182-182: Found useless expression. Either assign it to a variable or remove it.
(B018)
⏰ 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 Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (8)
.pre-commit-config.yaml (1)
34-34: LGTM — reasonable exclusion for bundled assets.Excluding
.jsand.cssfiles from typo checks makes sense for the bundled Viser frontend assets introduced in this PR.Optionally, you could make this more targeted by limiting the exclusion to the specific static assets directory:
exclude: newton/_src/viewer/viser/static/.*\.(js|css)$This would ensure any future source
.js/.cssfiles elsewhere in the repo still get typo-checked. Not critical for a Python-focused project, though.docs/tutorials/00_introduction.ipynb (7)
1-53: LGTM!The license cell is correctly hidden from rendered documentation using
"nbsphinx": "hidden", and the introduction provides a clear overview of the core Newton components.
54-158: LGTM!The imports are well-organized with helpful comments, and the progressive demonstration of adding bodies with various collision shapes (sphere, capsule, cylinder, and multi-shape compound collider) provides excellent educational value.
159-216: LGTM!The USD mesh loading workflow and model finalization with optional CPU device selection are well-documented. The static analysis hint (B018) is a false positive arising from analyzing the notebook JSON structure rather than the actual Python code.
281-351: LGTM!The simulation parameters are well-documented, and the simulation function clearly demonstrates the standard Newton simulation loop pattern: clear forces → apply controls → detect collisions → step solver → swap states.
352-377: LGTM!The CUDA graph capture pattern is correct and provides a good demonstration of GPU acceleration with proper fallback for CPU execution.
445-479: LGTM!The closing section provides clear next steps for readers, and the notebook metadata is standard.
418-443: Stalecontactsvariable used inlog_contactscall.The
contactsvariable used at line 434 refers to the initial collision detection from line 250, not the contacts computed during simulation (which are local tosimulate()). While the comment correctly notes that Viser doesn't support contact logging, this teaches a misleading pattern to readers.Consider either removing the call entirely or updating contacts in the main loop for correctness:
Option 1: Remove the misleading call
# Log the current state to the viewer viewer.begin_frame(sim_time) viewer.log_state(state_0) - - # Log contacts to the viewer (not supported by the Viser viewer) - viewer.log_contacts(contacts, state_0) viewer.end_frame()Option 2: Update contacts in the loop (for other viewers)
+ # Update contacts for the current state + contacts = model.collide(state_0) + # Log the current state to the viewer viewer.begin_frame(sim_time) viewer.log_state(state_0) - # Log contacts to the viewer (not supported by the Viser viewer) + # Log contacts to the viewer (note: not supported by ViewerViser) viewer.log_contacts(contacts, state_0) viewer.end_frame()⛔ Skipped due to learnings
Learnt from: nvtw Repo: newton-physics/newton PR: 1221 File: newton/examples/example_sdf.py:277-287 Timestamp: 2025-12-12T08:45:51.908Z Learning: In Newton examples (e.g., example_sdf.py), computing contacts once per frame and reusing them across multiple substeps is an intentional design choice, not a bug. The contacts are computed before the substep loop and intentionally kept constant throughout all substeps within that frame.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/tutorials/00_introduction.ipynb (3)
54-71: Consider adding a note about the USD dependency.The
from pxr import Usdimport requires OpenUSD/pxr to be installed, which is an optional dependency. Consider adding a brief note in the markdown cell above (or as a comment) mentioning this requirement, so users know to install it if they encounter an import error.
389-403: Recording path depends on working directory.The recording path
Path("../_static/recordings/00_introduction.viser")assumes the notebook is executed from thedocs/tutorials/directory. If run from a different working directory (e.g., repository root), the path will resolve incorrectly.This is likely fine for documentation builds (nbsphinx) where execution context is controlled, but consider adding a brief comment noting this assumption for users running the notebook interactively.
413-444: Consider removing the no-oplog_contactscall or clarifying its purpose.The comment on line 433 states that
log_contactsis "not supported by the Viser viewer." If the call has no effect, consider either:
- Removing it to avoid confusion, or
- Keeping it as a placeholder demonstrating the API for viewers that do support it (e.g., OpenGL viewer)
If keeping it, the comment placement is good for clarity.
Also, minor nit: 500 frames at 60 fps is ~8.33 seconds, not "about 8 seconds" (line 410).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/tutorials/00_introduction.ipynb
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The STYLE3D solver in Newton is a new solver that did not exist in warp.sim, so it does not require any migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-07-15T19:00:39.991Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 394
File: newton/examples/example_ik_benchmark.py:132-146
Timestamp: 2025-07-15T19:00:39.991Z
Learning: In the Newton library, `newton.utils.download_asset()` already handles internal caching and can work with both local and remote assets, so conditional logic to check for local paths is unnecessary. The standard practice is to use `download_asset` directly rather than implementing custom path handling logic.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-12-03T09:33:27.083Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_cartpole.py:388-392
Timestamp: 2025-12-03T09:33:27.083Z
Learning: In the Kamino ModelBuilder class located at newton/_src/solvers/kamino/core/builder.py, the attributes `gravity`, `bodies`, `joints`, `collision_geoms`, and `physical_geoms` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.gravity`) rather than called as methods (e.g., `builder.gravity()`).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2026-01-12T10:11:39.312Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py:6061-6091
Timestamp: 2026-01-12T10:11:39.312Z
Learning: Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py (ModelBuilder.finalize)
Learning: The project prefers to keep finalize() validation lightweight; do not add union-find/connected-components validation for loop joints. Current behavior (allow non-articulated joints when the child is in any articulation) is acceptable; avoid overengineering here.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-27T23:32:48.670Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 660
File: pyproject.toml:54-59
Timestamp: 2025-08-27T23:32:48.670Z
Learning: In the newton project's pyproject.toml, self-referential extras (e.g., `examples = ["newton[sim]", "newton[importers]", ...]`) are normal and encouraged for dependency management, rather than inlining the dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
🪛 Ruff (0.14.10)
docs/tutorials/00_introduction.ipynb
182-182: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (7)
docs/tutorials/00_introduction.ipynb (7)
1-53: LGTM!The license cell is properly hidden from nbsphinx rendering, and the introduction provides a clear overview of Newton's core components with appropriate documentation links.
91-158: LGTM!The model building section demonstrates the key concepts well: creating a builder, adding various primitive shapes, and showing how to attach multiple collision shapes to a single body. The code is clean and well-commented.
168-216: LGTM!The mesh loading from USD and model finalization are implemented correctly. The static analysis warning (B018) on line 182 is a false positive—it's flagging the JSON structure of the notebook rather than actual Python code.
236-280: LGTM!The state management pattern with two state objects for ping-pong integration is correctly explained and implemented. Good educational note about solver-specific requirements and differentiable simulation considerations.
324-351: LGTM!The simulation function correctly implements the standard Newton simulation loop pattern. The use of
globalfor state swapping is acceptable in a tutorial context for clarity.
360-377: LGTM!CUDA graph capture is correctly implemented with proper device detection and fallback for non-CUDA devices.
445-479: LGTM!The next steps section provides appropriate follow-up resources, and the notebook metadata is correctly structured.
Signed-off-by: Eric Heiden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/tutorials/00_introduction.ipynb (1)
394-403: Clarify or remove the call to unsupportedlog_contactsmethod.The comment on line 433 states that contact logging is "not supported by the Viser viewer," yet the code immediately calls
viewer.log_contacts(). This is confusing for tutorial readers:
- If the method is truly a no-op for Viser, consider removing the call entirely or adding a note that it's included for API consistency with other viewers.
- If it does something useful, update the comment to reflect the actual behavior.
Option A: Remove the unsupported call
# Log the current state to the viewer viewer.begin_frame(sim_time) viewer.log_state(state_0) - - # Log contacts to the viewer (not supported by the Viser viewer) - viewer.log_contacts(contacts, state_0) viewer.end_frame()Option B: Clarify the comment if keeping for consistency
- # Log contacts to the viewer (not supported by the Viser viewer) + # Log contacts to the viewer (no-op for Viser viewer, included for API consistency) viewer.log_contacts(contacts, state_0)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.pre-commit-config.yamldocs/guide/development.rstdocs/tutorials/00_introduction.ipynb
🚧 Files skipped from review as they are similar to previous changes (2)
- .pre-commit-config.yaml
- docs/guide/development.rst
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T05:17:34.423Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The STYLE3D solver in Newton is a new solver that did not exist in warp.sim, so it does not require any migration documentation.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-07-15T19:00:39.991Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 394
File: newton/examples/example_ik_benchmark.py:132-146
Timestamp: 2025-07-15T19:00:39.991Z
Learning: In the Newton library, `newton.utils.download_asset()` already handles internal caching and can work with both local and remote assets, so conditional logic to check for local paths is unnecessary. The standard practice is to use `download_asset` directly rather than implementing custom path handling logic.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-12-03T09:33:27.083Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1019
File: newton/_src/solvers/kamino/examples/sim/example_sim_basics_cartpole.py:388-392
Timestamp: 2025-12-03T09:33:27.083Z
Learning: In the Kamino ModelBuilder class located at newton/_src/solvers/kamino/core/builder.py, the attributes `gravity`, `bodies`, `joints`, `collision_geoms`, and `physical_geoms` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.gravity`) rather than called as methods (e.g., `builder.gravity()`).
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2026-01-12T10:11:39.312Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py:6061-6091
Timestamp: 2026-01-12T10:11:39.312Z
Learning: Repo: newton-physics/newton PR: 1314
File: newton/_src/sim/builder.py (ModelBuilder.finalize)
Learning: The project prefers to keep finalize() validation lightweight; do not add union-find/connected-components validation for loop joints. Current behavior (allow non-articulated joints when the child is in any articulation) is acceptable; avoid overengineering here.
Applied to files:
docs/tutorials/00_introduction.ipynb
📚 Learning: 2025-08-27T23:32:48.670Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 660
File: pyproject.toml:54-59
Timestamp: 2025-08-27T23:32:48.670Z
Learning: In the newton project's pyproject.toml, self-referential extras (e.g., `examples = ["newton[sim]", "newton[importers]", ...]`) are normal and encouraged for dependency management, rather than inlining the dependencies.
Applied to files:
docs/tutorials/00_introduction.ipynb
🪛 Ruff (0.14.10)
docs/tutorials/00_introduction.ipynb
182-182: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (1)
docs/tutorials/00_introduction.ipynb (1)
1-479: Well-structured introductory tutorial for Newton physics.This notebook provides a comprehensive and educational introduction covering:
- Model construction with various collision shapes (primitives and mesh)
- State management and the simulation loop pattern
- CUDA graph optimization for GPU execution
- Viser-based visualization with recording capability
The structure follows a logical progression and the inline documentation is clear.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/conf.py (2)
368-370: Missing parallel safety return value insetup().Other Sphinx extensions in this project (see
docs/_ext/autodoc_filter.pyanddocs/_ext/autodoc_wpfunc.py) return a dictionary declaring parallel build safety. Without this, Sphinx may disable parallel builds when this extension is loaded.♻️ Suggested fix
def setup(app: Any) -> None: # Copy on build init so `_static/viser/index.html` is always present in the built site. app.connect("builder-inited", _on_builder_inited) + return { + "parallel_read_safe": True, + "parallel_write_safe": True, + }Note: You'll also need to update the return type annotation to
-> dict[str, bool].
342-347: Clean up exception handling and remove unused noqa directive.Two static analysis findings:
- The
# noqa: PLC0415directive is flagged as unused since the rule isn't enabled.- Catching a bare
Exceptionand silently passing can obscure issues during debugging.♻️ Suggested improvement
try: - import newton # noqa: PLC0415 + import newton src_candidates.append(Path(newton.__file__).resolve().parent / "_src" / "viewer" / "viser" / "static") - except Exception: - pass + except ImportError: + pass # newton not installed; will try other candidatesUsing
ImportErroris more precise for import failures, and the comment clarifies the intent.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignoredocs/conf.pynewton/licenses/viser_and_inter-font-family.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use prefix-first naming for classes:ActuatorPD,ActuatorPID(notPDActuator,PIDActuator)
Use prefix-first naming for methods:add_shape_sphere()(notadd_sphere_shape())
Method names must usesnake_case
Prefer nested classes when self-contained: if a helper type or enum is only meaningful inside one parent class and doesn't need a public identity, define it as a nested class instead of a top-level class/module
Follow PEP 8 for Python code
Use Google-style docstrings with clear, concise explanations of what functions do, their parameters, and return values
Files:
docs/conf.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
CLI arguments must use
kebab-case(e.g.,--use-cuda-graph, not--use_cuda_graph)
Files:
docs/conf.pynewton/licenses/viser_and_inter-font-family.txt
🧠 Learnings (3)
📚 Learning: 2026-01-13T07:40:13.635Z
Learnt from: CR
Repo: newton-physics/newton PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T07:40:13.635Z
Learning: Applies to newton/**/*.py : Any user-facing class/function/object added under `_src` must be exposed via the public Newton API through re-exports
Applied to files:
docs/conf.py
📚 Learning: 2025-08-14T17:38:36.106Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/__init__.py:25-29
Timestamp: 2025-08-14T17:38:36.106Z
Learning: The Newton project prefers incremental __all__ building using __all__ += [...] pattern to group exports with their related imports, rather than a single consolidated __all__ at the end of the file.
Applied to files:
docs/conf.py
📚 Learning: 2026-01-13T07:40:13.635Z
Learnt from: CR
Repo: newton-physics/newton PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-13T07:40:13.635Z
Learning: Applies to newton/_src/**/*.py : The `newton/_src/` directory is internal library implementation only and must not be imported by user code (examples, documentation, or external users)
Applied to files:
docs/conf.py
🧬 Code graph analysis (1)
docs/conf.py (3)
newton/_src/utils/recorder.py (1)
append(61-70)docs/_ext/autodoc_filter.py (1)
setup(101-109)docs/_ext/autodoc_wpfunc.py (1)
setup(100-108)
🪛 LanguageTool
newton/licenses/viser_and_inter-font-family.txt
[style] ~67-~67: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losses), ev...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
🪛 Ruff (0.14.11)
docs/conf.py
343-343: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
346-347: try-except-pass detected, consider logging the exception
(S110)
346-346: Do not catch blind exception: Exception
(BLE001)
⏰ 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 Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (4)
newton/licenses/viser_and_inter-font-family.txt (1)
1-113: LGTM! Third-party license notices are properly structured.The file correctly documents the Viser (Apache 2.0) and Inter Font Family (SIL OFL 1.1) licenses with appropriate copyright notices and full license texts. This aligns with the PR's addition of Viser-based viewer functionality.
Note: The static analysis hint about "any and all" being wordy in line 67 should be ignored—this is verbatim text from the official Apache License 2.0 and must not be modified.
docs/conf.py (3)
108-117: Verify the tutorials notebook exclusion pattern.The
exclude_patternslist includes"tutorials/**/*.ipynb"(line 116), but the PR adds nbsphinx specifically to process Jupyter notebooks for documentation. If tutorial notebooks are meant to be rendered in the docs, this pattern would exclude them.Please verify this is intentional—perhaps the notebooks are included via explicit
toctreedirectives rather than auto-discovery, or they're processed through a different mechanism.
327-360: Asset copy logic is well-structured.The function correctly handles both development (repo checkout) and production (installed package) layouts, with graceful degradation via a warning rather than a hard failure. This ensures doc builds remain functional even if Viser assets are temporarily unavailable.
30-32: Good approach for build context detection.Setting
NEWTON_SPHINX_BUILDas an environment variable ensures it's inherited by subprocesses like Jupyter kernels, which is the correct way to propagate build context to nbsphinx-executed notebooks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @newton/_src/viewer/viewer.py:
- Around line 1351-1399: Rename the two internal helper functions
is_jupyter_notebook and is_sphinx_build to _is_jupyter_notebook and
_is_sphinx_build respectively, and update all internal call sites within the
_src package to use the new names; keep the same implementations and docstring
for is_sphinx_build but adjust the name, and ensure there are no public exports
or references remaining to the original names (search for is_jupyter_notebook
and is_sphinx_build and replace them with _is_jupyter_notebook and
_is_sphinx_build).
🧹 Nitpick comments (2)
newton/_src/viewer/viewer.py (2)
1351-1366: Add type hint and docstring for consistency withis_sphinx_build().The function works correctly but lacks a return type annotation and docstring, unlike its sibling
is_sphinx_build(). Per coding guidelines, use Google-style docstrings.✨ Suggested improvement
-def is_jupyter_notebook(): +def is_jupyter_notebook() -> bool: + """ + Detect if code is running inside a Jupyter Notebook or JupyterLab environment. + + Returns: + True if running in Jupyter (ZMQInteractiveShell), False otherwise. + """ try: # Check if get_ipython is defined (available in IPython environments) shell = get_ipython().__class__.__name__
1390-1398: Address static analysis findings in stack inspection block.Two issues flagged:
- Line 1391: The
# noqa: PLC0415directive is unused (PLC0415 is not enabled) — remove it.- Lines 1396-1397: Catching bare
Exceptionand silently passing is overly broad.While the silent pass is acceptable for best-effort detection, consider narrowing the exception type or adding a brief comment explaining why failures are ignored.
✨ Suggested fix
# Check call stack for sphinx-related frames try: - import traceback # noqa: PLC0415 + import traceback for frame_info in traceback.extract_stack(): if "sphinx" in frame_info.filename.lower() or "nbsphinx" in frame_info.filename.lower(): return True - except Exception: + except Exception: # noqa: BLE001 - best-effort detection; stack inspection may fail in edge cases pass
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
docs/guide/visualization.rstnewton/_src/viewer/viewer.pynewton/examples/__init__.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/guide/visualization.rst
- newton/examples/init.py
🧰 Additional context used
📓 Path-based instructions (4)
newton/_src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
The
newton/_src/directory is internal library implementation only and must not be imported by user code (examples, documentation, or external users)
Files:
newton/_src/viewer/viewer.py
newton/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Any user-facing class/function/object added under
_srcmust be exposed via the public Newton API through re-exports
Files:
newton/_src/viewer/viewer.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use prefix-first naming for classes:ActuatorPD,ActuatorPID(notPDActuator,PIDActuator)
Use prefix-first naming for methods:add_shape_sphere()(notadd_sphere_shape())
Method names must usesnake_case
Prefer nested classes when self-contained: if a helper type or enum is only meaningful inside one parent class and doesn't need a public identity, define it as a nested class instead of a top-level class/module
Follow PEP 8 for Python code
Use Google-style docstrings with clear, concise explanations of what functions do, their parameters, and return values
Files:
newton/_src/viewer/viewer.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
CLI arguments must use
kebab-case(e.g.,--use-cuda-graph, not--use_cuda_graph)
Files:
newton/_src/viewer/viewer.py
🧠 Learnings (1)
📚 Learning: 2026-01-13T03:11:40.556Z
Learnt from: jumyungc
Repo: newton-physics/newton PR: 1333
File: newton/_src/sim/builder.py:0-0
Timestamp: 2026-01-13T03:11:40.556Z
Learning: Ensure all VBD damping semantics follow Rayleigh-style damping and are unitless. This convention, demonstrated in SolverVBD, should be applied consistently across all VBD-related constraints in the codebase. In newton/_src/sim/builder.py, validate that any damping terms use the same normalization and are treated as dimensionless; add comments and unit tests to enforce consistency across VBD components.
Applied to files:
newton/_src/viewer/viewer.py
🪛 Ruff (0.14.11)
newton/_src/viewer/viewer.py
1391-1391: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
1396-1397: try-except-pass detected, consider logging the exception
(S110)
1396-1396: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (4)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (1)
newton/_src/viewer/viewer.py (1)
18-19: LGTM!The
osandsysimports are required by the newis_sphinx_build()function for environment variable access and module introspection.
3 participants
ViewerViserwhich uses Viser as back-end for an HTML-based web viewerSummary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.