Rerun Viewer improvements, Jupyter Notebook support#1045
Conversation
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdded and re-exported a USD mesh loader Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Importer as Importer / Example
participant USD as pxr.USD (UsdPrim)
participant Utils as newton.usd.get_mesh
participant Mesh as Mesh obj
Importer->>Utils: get_mesh(prim, load_normals?, load_uvs?, ...)
Utils->>USD: read points, faceVertexIndices, counts, normals?, uvs?
Note right of Utils `#f9f6e7`: process face-varying normals\n(vertex_averaging / angle_weighted / vertex_splitting)
Utils->>Mesh: populate vertices, indices, normals, uvs, maxhullvert
Utils-->>Importer: return Mesh
Note right of Importer `#eef6ef`: pass Mesh to add_shape_mesh(mesh)\nor set mesh.maxhullvert
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/viewer/viewer_rerun.py (1)
189-203: Initializevertex_colorsbefore useIf
colorsisNone, the code skips the branch that definesvertex_colorsbut still passes it intorr.Mesh3D, triggering anUnboundLocalErroron the first instancing call without colors. Initializevertex_colors = Nonebefore the conditional and only overwrite it when colors are provided.Apply this diff:
- if colors is not None: + vertex_colors = None + if colors is not None: colors_np = colors.numpy().astype(np.float32) # Take the first instance's color and apply to all vertices first_color = colors_np[0] color_rgb = np.array(first_color * 255, dtype=np.uint8) num_vertices = len(mesh_data["points"]) vertex_colors = np.tile(color_rgb, (num_vertices, 1))
🧹 Nitpick comments (3)
newton/_src/usd/utils.py (3)
658-658: Remove unnecessary float() cast.The
np.dotoperation already returns a scalar that can be compared withcos_thresh. The explicitfloat()cast is redundant.Apply this diff:
- if float(np.dot(mean_dir, n_dir)) >= cos_thresh: + if np.dot(mean_dir, n_dir) >= cos_thresh:
707-707: Consider shorter error message or exception subclass.Static analysis flags this long error message. While the message is clear, consider either shortening it or creating a custom exception class if this pattern repeats.
Example refactor:
- raise ValueError(f"Invalid face_varying_normal_conversion: {face_varying_normal_conversion}") + raise ValueError( + f"Invalid face_varying_normal_conversion: {face_varying_normal_conversion}. " + f"Expected one of: vertex_averaging, angle_weighted, vertex_splitting" + )Or simply:
- raise ValueError(f"Invalid face_varying_normal_conversion: {face_varying_normal_conversion}") + valid = ["vertex_averaging", "angle_weighted", "vertex_splitting"] + raise ValueError(f"Invalid conversion method '{face_varying_normal_conversion}'. Expected: {valid}")
709-733: LGTM! Triangulation and mesh creation logic is sound.The face processing correctly handles:
- Triangle pass-through (line 713)
- Quad-to-triangle split with pattern (0,1,2) and (0,2,3) - lines 715-716
- Graceful skip of unsupported polygons with verbose logging
- Proper winding order flip for left-handed coordinates
Note: The quad triangulation assumes convex quads, which is the USD standard.
Optional: Consider adding validation for meshes with only unsupported polygons. When a USD mesh contains no triangles or quads, the
faceslist remains empty, resulting in a degenerate mesh being created without warning. Adding a check before line 733 (e.g.,if len(faces) == 0: print(...)or raising an error) would provide better visibility into malformed input geometry.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
newton/_src/usd/__init__.py(2 hunks)newton/_src/usd/utils.py(2 hunks)newton/_src/utils/import_usd.py(3 hunks)newton/_src/viewer/viewer.py(2 hunks)newton/_src/viewer/viewer_rerun.py(6 hunks)newton/usd.py(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
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.
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.
📚 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:
newton/_src/usd/__init__.pynewton/_src/usd/utils.pynewton/usd.pynewton/_src/utils/import_usd.py
📚 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:
newton/_src/usd/__init__.pynewton/usd.pynewton/_src/utils/import_usd.py
📚 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:
newton/_src/usd/__init__.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is properly re-exported from the spatial module via "from .spatial import (..., transform_twist, ...)" and included in __all__, making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/_src/usd/__init__.pynewton/usd.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 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:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-08-25T20:10:51.009Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/examples/ik/example_ik_h1.py:0-0
Timestamp: 2025-08-25T20:10:51.009Z
Learning: In Warp, creating a new wp.array() with an existing array and different shape (e.g., wp.array(existing_array, shape=new_shape)) creates a view into the same underlying memory buffer, not a copy. This means modifications through either view are automatically reflected in both arrays.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-08-12T18:04:06.287Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/utils/import_usd.py:364-365
Timestamp: 2025-08-12T18:04:06.287Z
Learning: The quat_between_axes function in Newton accepts AxisType parameters, which includes strings like "X", "Y", "Z" as well as Axis enum values and integers. The function internally handles conversion using Axis.from_any(), so passing strings directly to quat_between_axes is valid and correct behavior.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:04:06.287Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/utils/import_usd.py:364-365
Timestamp: 2025-08-12T18:04:06.287Z
Learning: The quat_between_axes function in Newton accepts AxisType parameters, which includes both Axis enum values and strings. The function internally handles string-to-Axis conversion using Axis.from_any(), so passing strings directly to quat_between_axes is valid and correct.
Applied to files:
newton/_src/utils/import_usd.py
🧬 Code graph analysis (5)
newton/_src/usd/__init__.py (1)
newton/_src/usd/utils.py (1)
load_mesh(572-733)
newton/_src/usd/utils.py (1)
newton/_src/geometry/types.py (1)
Mesh(116-300)
newton/usd.py (1)
newton/_src/usd/utils.py (1)
load_mesh(572-733)
newton/_src/viewer/viewer_rerun.py (2)
newton/_src/utils/mesh.py (1)
create_plane_mesh(405-446)newton/_src/viewer/viewer.py (3)
log_geo(388-471)get_world_extents(135-183)log_mesh(482-492)
newton/_src/utils/import_usd.py (2)
newton/_src/usd/utils.py (1)
load_mesh(572-733)newton/_src/sim/builder.py (1)
add_shape_mesh(3179-3215)
🪛 Ruff (0.14.3)
newton/_src/usd/utils.py
707-707: 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). (2)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (2)
newton/_src/usd/utils.py (2)
18-26: LGTM! Imports are appropriate.The addition of
Literalfor type hints andMeshfrom geometry is correct for the newload_meshfunction. The lazy USD module loading pattern is maintained as expected.
470-569: LGTM! Well-implemented helper functions.The three helper functions are mathematically sound:
_newell_normalcorrectly implements Newell's method for robust polygon normal computation_orthonormal_basis_from_normalproperly handles basis construction with degenerate case fallbackcorner_anglesuses careful 2D projection with good numerical stability measures (edge normalization, clamping, epsilon checks)
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
newton/_src/usd/utils.py (2)
630-630: Reminder: Replace assertion with ValueError.This assertion should be replaced with a proper
ValueErroras suggested in previous review comments. The user requested the fix be applied.Apply this fix:
- assert len(normals) == len(indices) + if len(normals) != len(indices): + raise ValueError( + f"faceVarying normals length ({len(normals)}) must match " + f"number of face-vertex indices ({len(indices)}) for mesh {prim.GetPath()}" + )
638-638: Reminder: Replace assertion with ValueError.This assertion should be replaced with a proper
ValueErroras suggested in previous review comments. The user requested the fix be applied.Apply this fix:
- assert indices.shape[0] == Nfv.shape[0], "indices and faceVarying normals must have same length" + if indices.shape[0] != Nfv.shape[0]: + raise ValueError( + f"Indices and faceVarying normals must have same length " + f"(got {indices.shape[0]} vs {Nfv.shape[0]}) for mesh {prim.GetPath()}" + )
🧹 Nitpick comments (1)
newton/_src/usd/utils.py (1)
724-724: Optional: Consider exception message style.Ruff flags the inline error message as TRY003. For a single validation error like this, the inline message is acceptable and clear. You may optionally define custom exception types if similar validations appear elsewhere.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/pr.yml(1 hunks)docs/api/newton.rst(1 hunks)docs/api/newton_usd.rst(1 hunks)newton/_src/geometry/types.py(1 hunks)newton/_src/usd/utils.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/api/newton_usd.rst
- docs/api/newton.rst
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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.
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.
📚 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:
newton/_src/usd/utils.py
📚 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:
newton/_src/usd/utils.py
🧬 Code graph analysis (1)
newton/_src/usd/utils.py (1)
newton/_src/geometry/types.py (5)
Mesh(116-308)indices(228-229)indices(232-234)normals(237-238)copy(187-216)
🪛 GitHub Actions: Pull Request
newton/_src/usd/utils.py
[error] 615-615: ValueError: invalid array_struct encountered while executing doctest for newton.usd.load_mesh (doctest failure in api/_generated/newton.usd.load_mesh) during Sphinx build. Command: 'uv run --extra docs --extra importers sphinx-build -W -b doctest docs docs/_build/doctest'.
🪛 Ruff (0.14.3)
newton/_src/usd/utils.py
724-724: 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). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (5)
.github/workflows/pr.yml (1)
38-38: LGTM! Correctly enables importer testing.The addition of
--extra importersensures that doctests for the newly exposedload_meshAPI and other USD utilities are executed during CI.newton/_src/geometry/types.py (1)
236-242: LGTM! Clean read-only property exposure.The new
normalsanduvsproperties provide public access to per-vertex mesh attributes, consistent with the existingverticesandindicesproperties. The read-only design is appropriate.newton/_src/usd/utils.py (3)
501-569: LGTM! Robust corner angle computation.The
corner_anglesfunction correctly implements 2D projection onto the face plane using Newell's method with proper handling of degenerate cases and numerical stability considerations.
708-722: LGTM! Angle-weighted normal averaging is a quality improvement.The angle-weighted conversion mode provides better visual quality by weighting face normals by their corner angles, which is superior to simple averaging for meshes with varying triangle sizes.
586-599: Doctest example is incomplete; fix undefinedbuildervariable.The doctest at lines 586-599 uses
builder.add_body()andbuilder.add_shape_mesh()without instantiation. The code fails on line 594 with aNameError, not at line 615 as claimed. The example must initializeModelBuilderbefore use:.. testcode:: from pxr import Usd import newton import newton.examples builder = newton.ModelBuilder() usd_stage = Usd.Stage.Open(newton.examples.get_asset("bunny.usd")) demo_mesh = newton.usd.load_mesh(usd_stage.GetPrimAtPath("/root/bunny")) body_mesh = builder.add_body() builder.add_shape_mesh(body_mesh, mesh=demo_mesh) assert len(demo_mesh.vertices) == 6102 assert len(demo_mesh.indices) == 36600 assert len(demo_mesh.normals) == 6102Likely an incorrect or invalid review comment.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
newton/_src/usd/utils.py (3)
640-640: Replace assertion with proper exception (duplicate of past review).This assertion was flagged in a previous review and you requested the fix be applied, but it's still present in the code. Production code should use explicit exceptions rather than assertions, which can be disabled with
python -O.Apply this diff:
- assert len(normals) == len(indices) + if len(normals) != len(indices): + raise ValueError( + f"faceVarying normals length ({len(normals)}) must match " + f"number of face-vertex indices ({len(indices)}) for mesh {prim.GetPath()}" + )
648-648: Replace assertion with proper exception (duplicate of past review).This assertion was also flagged in a previous review and you requested the fix be applied, but it's still present. Same issue as line 640 - use explicit ValueError instead.
Apply this diff:
- assert indices.shape[0] == Nfv.shape[0], "indices and faceVarying normals must have same length" + if indices.shape[0] != Nfv.shape[0]: + raise ValueError( + f"Indices and faceVarying normals must have same length " + f"(got {indices.shape[0]} vs {Nfv.shape[0]}) for mesh {prim.GetPath()}" + )
625-760: Fix dtype inconsistency between points and normals.Points are loaded as
float64(line 625) but normals are converted tofloat32(lines 704, 717, 732). When passed toMesh.__init__at line 760, this creates a dtype mismatch. According to past review comments, this causes a doctest failure and theMesh.__init__atnewton/_src/geometry/types.py:169should enforcefloat32for vertices to match theverticessetter behavior at line 224.However, the better fix is to convert
pointstofloat32here before constructing theMesh, ensuring consistency at the call site.Apply this diff to convert points to float32 before returning:
if flip_winding: faces = faces[:, ::-1] - return Mesh(points, faces.flatten(), normals=normals, maxhullvert=maxhullvert) + return Mesh( + points.astype(np.float32), + faces.flatten(), + normals=normals, + maxhullvert=maxhullvert + )Alternatively, if the
Mesh.__init__atnewton/_src/geometry/types.py:169is updated to enforcedtype=np.float32, that would also resolve this issue project-wide.
🧹 Nitpick comments (1)
newton/_src/usd/utils.py (1)
744-748: Consider raising exception instead of silent continue for unsupported polygons.When encountering polygons with more than 4 vertices, the code currently prints a verbose message and continues, silently dropping the polygon. This could lead to incomplete mesh loading without clear indication to the user.
Consider raising an exception by default, or at minimum always warn (not just when
verbose=True):elif verbose: print( f"Error while parsing USD mesh {prim.GetPath()}: encountered polygon with {count} vertices, but only triangles and quads are supported." ) - continue + else: + # Always warn about dropped polygons, not just in verbose mode + import warnings + warnings.warn( + f"Skipping polygon with {count} vertices in mesh {prim.GetPath()} " + "(only triangles and quads are supported)", + stacklevel=2 + ) + continue
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/conf.py(1 hunks)newton/_src/usd/utils.py(3 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/conf.py
🧰 Additional context used
🧠 Learnings (8)
📚 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:
pyproject.toml
📚 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:
pyproject.toml
📚 Learning: 2025-08-21T15:51:07.824Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 592
File: pyproject.toml:64-70
Timestamp: 2025-08-21T15:51:07.824Z
Learning: Self-referencing optional dependencies (e.g., "package[extra]") have been supported since pip 21.2 and are valid in pyproject.toml. However, build backends differ in metadata emission: some flatten dependencies while others preserve self-references. This can affect lockfile generators and tooling. Testing with the specific build backend and dependency tools is recommended to ensure expected behavior.
Applied to files:
pyproject.toml
📚 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:
newton/_src/usd/utils.py
📚 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:
newton/_src/usd/utils.py
📚 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:
newton/_src/usd/utils.py
📚 Learning: 2025-09-23T14:47:05.974Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_grain_rendering.py:151-153
Timestamp: 2025-09-23T14:47:05.974Z
Learning: In Newton's MPM grain rendering system, the `points_per_particle` CLI argument should remain as `type=float` rather than `type=int` because it's used in arithmetic calculations (e.g., `grain_radius = options.voxel_size / (3 * options.points_per_particle)`) where fractional values provide meaningful precision, even though some downstream functions eventually convert it to integer dimensions.
Applied to files:
newton/_src/usd/utils.py
🧬 Code graph analysis (1)
newton/_src/usd/utils.py (2)
newton/_src/core/types.py (1)
Axis(63-160)newton/_src/geometry/types.py (5)
Mesh(116-308)indices(228-229)indices(232-234)normals(237-238)copy(187-216)
🪛 Ruff (0.14.3)
newton/_src/usd/utils.py
726-726: 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). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (6)
pyproject.toml (1)
80-80: Add importers to docs dependencies to support new public API documentation.The addition of
"newton[importers]"enables documentation builds to access USD and mesh processing libraries needed for documenting the newload_meshAPI. This follows the established self-referential pattern already used by theexamples,torch-cu12, anddevextras in this file (lines 56, 66, 73), and aligns with the PR objective to add this public API to newton.usd.Note: Per PR 592 learnings, self-referencing optional dependencies are valid since pip 21.2, though build backends differ in metadata emission. Testing with your specific build backend is recommended if using lockfile generators.
newton/_src/usd/utils.py (5)
19-25: LGTM - imports align with new mesh loading functionality.The addition of
Literalfor type hints and importingMESH_MAXHULLVERTandMeshfrom the geometry module are necessary and correctly structured for the newload_meshAPI.
39-45: LGTM - type overloads improve type safety.The overloads correctly distinguish between cases where a default is provided versus not, enabling better type inference for callers.
478-577: LGTM - robust geometric utilities.The helper functions implement standard geometric algorithms with appropriate numerical stability measures:
_newell_normaluses Newell's method for polygon normals_orthonormal_basis_from_normalhandles degenerate cases gracefullycorner_anglesuses vectorized operations and includes epsilon checks for edge cases
666-696: LGTM - vertex splitting logic is well-implemented.The vertex clustering approach based on angular similarity is sound. The code correctly:
- Normalizes corner normals for comparison
- Uses cosine threshold for angular proximity
- Maintains per-vertex clusters with incremental mean updates
- Handles cluster assignment with proper fallback to new vertex creation
753-758: LGTM - winding order handling is correct.The code properly checks the USD orientation attribute and flips face winding for left-handed coordinate systems, ensuring correct face orientation in the Newton mesh representation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
newton/_src/usd/utils.py (2)
648-648: Replace assertion with proper exception (unresolved from previous review).This assertion was also flagged in the previous review. Use explicit ValueError instead of assertion for production-safe error handling.
Apply this diff:
- assert indices.shape[0] == Nfv.shape[0], "indices and faceVarying normals must have same length" + if indices.shape[0] != Nfv.shape[0]: + raise ValueError( + f"Indices and faceVarying normals must have same length " + f"(got {indices.shape[0]} vs {Nfv.shape[0]}) for mesh {prim.GetPath()}" + )
640-640: Replace assertion with proper exception (unresolved from previous review).This assertion was flagged in a previous review and you requested it be fixed, but it remains. Production code should use explicit exceptions rather than assertions, which can be disabled with
python -O.Apply this diff:
- assert len(normals) == len(indices) + if len(normals) != len(indices): + raise ValueError( + f"faceVarying normals length ({len(normals)}) must match " + f"number of face-vertex indices ({len(indices)}) for mesh {prim.GetPath()}" + )
🧹 Nitpick comments (1)
newton/_src/usd/utils.py (1)
734-734: Consider extracting long error message to reduce TRY003 warning.Static analysis flagged this line for having a long message directly in the exception. While this is a minor style issue, you could optionally extract the message to a variable or define custom exception classes if you prefer to follow strict TRY003 guidelines.
Optional refactor:
+ msg = f"Invalid face_varying_normal_conversion: {face_varying_normal_conversion}" - raise ValueError(f"Invalid face_varying_normal_conversion: {face_varying_normal_conversion}") + raise ValueError(msg)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/conf.py(1 hunks)newton/_src/usd/utils.py(3 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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-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:
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:
pyproject.toml
📚 Learning: 2025-08-21T15:51:07.824Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 592
File: pyproject.toml:64-70
Timestamp: 2025-08-21T15:51:07.824Z
Learning: Self-referencing optional dependencies (e.g., "package[extra]") have been supported since pip 21.2 and are valid in pyproject.toml. However, build backends differ in metadata emission: some flatten dependencies while others preserve self-references. This can affect lockfile generators and tooling. Testing with the specific build backend and dependency tools is recommended to ensure expected behavior.
Applied to files:
pyproject.toml
📚 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:
newton/_src/usd/utils.py
📚 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:
newton/_src/usd/utils.py
📚 Learning: 2025-09-23T14:47:05.974Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_grain_rendering.py:151-153
Timestamp: 2025-09-23T14:47:05.974Z
Learning: In Newton's MPM grain rendering system, the `points_per_particle` CLI argument should remain as `type=float` rather than `type=int` because it's used in arithmetic calculations (e.g., `grain_radius = options.voxel_size / (3 * options.points_per_particle)`) where fractional values provide meaningful precision, even though some downstream functions eventually convert it to integer dimensions.
Applied to files:
newton/_src/usd/utils.py
🧬 Code graph analysis (1)
newton/_src/usd/utils.py (1)
newton/_src/geometry/types.py (5)
Mesh(116-308)indices(228-229)indices(232-234)normals(237-238)copy(187-216)
🪛 Ruff (0.14.3)
newton/_src/usd/utils.py
734-734: 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). (1)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (9)
docs/conf.py (1)
146-146: LGTM! Coordinated change to support USD documentation.Removing "pxr" from mock imports aligns with adding "newton[importers]" to docs dependencies in pyproject.toml, enabling proper documentation of the new
load_meshAPI and other USD functionality.pyproject.toml (1)
80-80: LGTM! Enables USD library access for documentation build.Adding "newton[importers]" to docs dependencies provides the USD libraries needed to document the new
load_meshAPI. Self-referential extras are consistent with project conventions.Based on learnings
newton/_src/usd/utils.py (7)
19-19: LGTM! Imports support new type safety and mesh loading functionality.The new imports enable type overloads for better IDE support and provide the types needed for the
load_meshfunction.Also applies to: 25-25
39-45: LGTM! Type overloads improve type safety.The overloads enable better type inference for
get_attribute, helping type checkers understand that the return type depends on whether a non-None default is provided.
478-488: LGTM! Correct implementation of Newell's method.The function correctly implements Newell's method for computing a robust polygon normal, which is particularly useful for non-planar or nearly degenerate polygons.
491-507: LGTM! Robust orthonormal basis construction.The function correctly constructs an orthonormal basis from a normal vector with appropriate stability checks and degenerate case handling.
509-577: LGTM! Sophisticated angle computation with robust handling.The function implements a numerically stable approach to computing corner angles by projecting to 2D and using normalized edge vectors. The handling of degenerate cases is appropriate.
580-621: LGTM! Well-designed public API with comprehensive documentation.The function signature is clean, type-safe, and the docstring provides a clear working example. The three normal conversion methods give users flexibility for different mesh requirements.
736-760: LGTM! Correct triangulation and winding order handling.The triangulation correctly handles triangles and quads (splitting quads as two triangles), and the winding order flip for left-handed coordinate systems is properly implemented.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/api/newton_usd.rst(1 hunks)newton/_src/geometry/types.py(4 hunks)newton/_src/usd/__init__.py(2 hunks)newton/_src/usd/utils.py(3 hunks)newton/_src/utils/import_usd.py(3 hunks)newton/_src/viewer/viewer.py(2 hunks)newton/examples/basic/example_basic_shapes.py(2 hunks)newton/usd.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/utils/import_usd.py
🧰 Additional context used
🧠 Learnings (7)
📚 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:
newton/_src/usd/__init__.pynewton/examples/basic/example_basic_shapes.pynewton/usd.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/examples/basic/example_basic_shapes.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: In NVIDIA Warp's Newton physics library, wp.transform supports direct numerical indexing (e.g., body_tf[0], body_tf[1], body_tf[2] for position and body_tf[3], body_tf[4], body_tf[5], body_tf[6] for quaternion components) to access its elements. This is the standard API used throughout the codebase.
Applied to files:
newton/examples/basic/example_basic_shapes.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/examples/basic/example_basic_shapes.py
📚 Learning: 2025-08-12T17:58:00.815Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:18-20
Timestamp: 2025-08-12T17:58:00.815Z
Learning: In newton/_src/core/__init__.py, the function transform_twist is properly re-exported from the spatial module via "from .spatial import (..., transform_twist, ...)" and included in __all__, making it available for import as "from ...core import transform_twist" from other modules.
Applied to files:
newton/usd.py
📚 Learning: 2025-09-23T14:47:05.974Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_grain_rendering.py:151-153
Timestamp: 2025-09-23T14:47:05.974Z
Learning: In Newton's MPM grain rendering system, the `points_per_particle` CLI argument should remain as `type=float` rather than `type=int` because it's used in arithmetic calculations (e.g., `grain_radius = options.voxel_size / (3 * options.points_per_particle)`) where fractional values provide meaningful precision, even though some downstream functions eventually convert it to integer dimensions.
Applied to files:
newton/_src/usd/utils.py
📚 Learning: 2025-09-17T21:47:26.087Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_granular.py:162-165
Timestamp: 2025-09-17T21:47:26.087Z
Learning: wp.Mesh constructor takes parameters in the order: points, indices, velocities (optional), support_winding_number, bvh_constructor. The second positional parameter is indices, not velocities.
Applied to files:
newton/_src/geometry/types.py
🧬 Code graph analysis (4)
newton/_src/usd/__init__.py (1)
newton/_src/usd/utils.py (1)
get_mesh(580-791)
newton/examples/basic/example_basic_shapes.py (1)
newton/_src/usd/utils.py (1)
get_mesh(580-791)
newton/usd.py (1)
newton/_src/usd/utils.py (1)
get_mesh(580-791)
newton/_src/usd/utils.py (1)
newton/_src/geometry/types.py (6)
Mesh(116-314)indices(234-235)indices(238-240)normals(243-244)copy(187-222)uvs(247-248)
🪛 Ruff (0.14.3)
newton/_src/usd/utils.py
662-664: Avoid specifying long messages outside the exception class
(TRY003)
673-675: Avoid specifying long messages outside the exception class
(TRY003)
761-761: 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). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (9)
newton/_src/geometry/types.py (1)
169-172: Float32 coercion keeps the Mesh invariant tight.Appreciate the explicit float32 cast here—it guarantees host buffers line up with Warp’s vec3 inputs and avoids dtype drift from USD/NumPy callers.
docs/api/newton_usd.rst (1)
22-22: Docs entry forget_meshlooks good.Thanks for wiring the new loader into the autosummary so it shows up in the published API docs.
newton/_src/usd/__init__.py (1)
25-42: Public re-export aligns with the new utility.Re-exporting
get_meshhere keeps the internal and external USD namespaces consistent.newton/usd.py (1)
32-52: Top-level export matches the docs/examples.Good to see
get_meshexposed at the publicnewton.usdlayer so notebooks/examples can rely on it.newton/examples/basic/example_basic_shapes.py (3)
27-27: LGTM: Import simplified to match actual usage.The change from
UsdGeomtoUsdis correct since onlyUsd.Stage.Open()is needed now that mesh extraction is handled by the newnewton.usd.get_mesh()API.
31-31: LGTM: New import for USD utilities.This import provides access to the new
newton.usd.get_mesh()API used below.
80-80: Excellent refactoring to use the new public API.Replacing manual mesh extraction with
newton.usd.get_mesh()is a significant improvement. The new API handles complex scenarios like face-varying normal conversion, UV loading, and mesh handedness that weren't covered by manual extraction. This makes the code cleaner and more robust.newton/_src/viewer/viewer.py (2)
135-183: Well-designed public API for world extent computation.Extracting the world extent calculation into a reusable public method is excellent API design. The implementation correctly:
- Initializes bounds with
wp.inf/-wp.inffor proper min/max tracking- Handles edge cases (no model, no valid worlds)
- Returns the maximum extents across all worlds as a tuple
This makes the functionality accessible to users and improves code organization.
185-203: Clean refactoring using the new helper method.The refactored
_auto_compute_world_offsets()method now properly delegates extent calculation toget_world_extents(), with appropriate early returns for edge cases. This eliminates code duplication and improves maintainability.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Can't see any obvious issues with this. So I guess if it works, I can push the approve button. I don't know enough about using rerun..
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/viewer/viewer_rerun.py (1)
234-240: Initialize vertex_colors before use.When
colorsisNone,vertex_colorsstays undefined and Line 238 raisesUnboundLocalError. Initialize it toNonebefore the conditional so meshes without per-instance colors still log correctly.Apply this diff:
- if colors is not None: + vertex_colors = None + if colors is not None: colors_np = self._to_numpy(colors).astype(np.float32) first_color = colors_np[0] color_rgb = np.array(first_color * 255, dtype=np.uint8) num_vertices = len(mesh_data["points"]) vertex_colors = np.tile(color_rgb, (num_vertices, 1)) - - mesh_3d = rr.Mesh3D( + mesh_3d = rr.Mesh3D( vertex_positions=mesh_data["points"], triangle_indices=mesh_data["indices"], vertex_normals=mesh_data["normals"], - vertex_colors=vertex_colors, + vertex_colors=vertex_colors, )
♻️ Duplicate comments (1)
newton/_src/viewer/viewer_rerun.py (1)
446-459: Guard plane scale indexing.Line 448 still dereferences
geo_scale[1]without checking length, so passing(0.0,)(or any single-value plane scale) crashes withIndexError, recreating the regression highlighted earlier. Normalize the width/length before the check, mirroring the base viewer logic.Apply this diff:
- if geo_type == newton.GeoType.PLANE: - # Handle "infinite" planes encoded with non-positive scales - if geo_scale[0] == 0.0 or geo_scale[1] == 0.0: + if geo_type == newton.GeoType.PLANE: + width = float(geo_scale[0]) if len(geo_scale) >= 1 else 0.0 + length = float(geo_scale[1]) if len(geo_scale) >= 2 else width + # Handle "infinite" planes encoded with non-positive scales + if width <= 0.0 or length <= 0.0: extents = self._get_world_extents() if extents is None: width, length = 10.0, 10.0 else: max_extent = max(extents) * 1.5 width = max_extent length = max_extent else: - width = geo_scale[0] - length = geo_scale[1] if len(geo_scale) > 1 else 10.0 + width = width + length = length if length > 0.0 else 10.0
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/viewer/viewer.py(3 hunks)newton/_src/viewer/viewer_rerun.py(10 hunks)newton/examples/diffsim/example_diffsim_ball.py(3 hunks)newton/examples/diffsim/example_diffsim_bear.py(1 hunks)newton/examples/diffsim/example_diffsim_drone.py(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/_src/viewer/viewer_rerun.pynewton/_src/viewer/viewer.py
📚 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:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-08-25T20:10:51.009Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/examples/ik/example_ik_h1.py:0-0
Timestamp: 2025-08-25T20:10:51.009Z
Learning: In Warp, creating a new wp.array() with an existing array and different shape (e.g., wp.array(existing_array, shape=new_shape)) creates a view into the same underlying memory buffer, not a copy. This means modifications through either view are automatically reflected in both arrays.
Applied to files:
newton/_src/viewer/viewer_rerun.pynewton/_src/viewer/viewer.py
📚 Learning: 2025-09-17T21:34:05.267Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_granular.py:44-47
Timestamp: 2025-09-17T21:34:05.267Z
Learning: wp.Mesh (Warp mesh objects) expose triangle indices through the `indices` attribute, not `triangles`. The correct usage is `wp.Mesh(...).indices`, not `wp.Mesh(...).triangles`.
Applied to files:
newton/_src/viewer/viewer_rerun.pynewton/_src/viewer/viewer.py
📚 Learning: 2025-09-17T21:34:47.200Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_multi_material.py:84-90
Timestamp: 2025-09-17T21:34:47.200Z
Learning: wp.array supports fill_ method on indexed/sliced arrays, including advanced indexing with integer arrays that creates wp.indexedarray objects. Operations like arr[indices].fill_(value) work correctly in Warp.
Applied to files:
newton/_src/viewer/viewer.py
📚 Learning: 2025-09-17T21:47:26.087Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_granular.py:162-165
Timestamp: 2025-09-17T21:47:26.087Z
Learning: wp.Mesh constructor takes parameters in the order: points, indices, velocities (optional), support_winding_number, bvh_constructor. The second positional parameter is indices, not velocities.
Applied to files:
newton/_src/viewer/viewer.py
🧬 Code graph analysis (4)
newton/_src/viewer/viewer_rerun.py (2)
newton/_src/utils/mesh.py (1)
create_plane_mesh(405-446)newton/_src/viewer/viewer.py (6)
log_array(509-510)log_scalar(513-514)log_geo(390-473)_get_world_extents(135-183)log_mesh(484-494)log_points(505-506)
newton/examples/diffsim/example_diffsim_drone.py (2)
newton/_src/viewer/viewer_gl.py (1)
log_scalar(375-379)newton/_src/viewer/viewer_null.py (1)
log_scalar(152-160)
newton/examples/diffsim/example_diffsim_ball.py (4)
newton/examples/diffsim/example_diffsim_drone.py (1)
render(763-796)newton/examples/diffsim/example_diffsim_spring_cage.py (1)
render(232-274)newton/examples/diffsim/example_diffsim_cloth.py (1)
render(190-218)newton/examples/diffsim/example_diffsim_soft_body.py (1)
render(331-359)
newton/_src/viewer/viewer.py (1)
newton/_src/geometry/types.py (2)
indices(234-235)indices(238-240)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/viewer/viewer_rerun.py (1)
226-243: Initializevertex_colorsfor the mesh instancer.When
colorsisNone, the laterrr.Mesh3D(..., vertex_colors=vertex_colors, ...)dereferences an uninitialized local, raisingUnboundLocalError. Seedvertex_colors = Nonebefore the conditional (and only overwrite it when colors are provided) so the no-color path works.- # Handle colors - ReRun doesn't support per-instance colors - # so we just use the first instance's color for all instances - if colors is not None: + # Handle colors - ReRun doesn't support per-instance colors + # so we just use the first instance's color for all instances + vertex_colors = None + if colors is not None: colors_np = self._to_numpy(colors).astype(np.float32) # Take the first instance's color and apply to all vertices first_color = colors_np[0] color_rgb = np.array(first_color * 255, dtype=np.uint8) num_vertices = len(mesh_data["points"]) vertex_colors = np.tile(color_rgb, (num_vertices, 1))
♻️ Duplicate comments (3)
newton/_src/viewer/viewer_rerun.py (3)
450-461: Guard plane scale indexing for short tuples.This branch still assumes
geo_scalehas two entries; with common single-value plane scales (e.g.(10.0,)) we hitIndexError: tuple index out of range. Normalize the tuple lengths before indexing so “infinite” and single-axis planes work again.- if geo_type == newton.GeoType.PLANE: - # Handle "infinite" planes encoded with non-positive scales - if geo_scale[0] == 0.0 or geo_scale[1] == 0.0: + if geo_type == newton.GeoType.PLANE: + width = geo_scale[0] if len(geo_scale) >= 1 else 0.0 + length = geo_scale[1] if len(geo_scale) >= 2 else width + # Handle "infinite" planes encoded with non-positive scales + if width <= 0.0 or length <= 0.0: extents = self._get_world_extents() if extents is None: width, length = 10.0, 10.0 else: max_extent = max(extents) * 1.5 width = max_extent length = max_extent else: - width = geo_scale[0] - length = geo_scale[1] if len(geo_scale) > 1 else 10.0 + width = float(width) + length = float(length) if len(geo_scale) >= 2 else float(width)
112-116: Honor the configured server address when starting gRPC.
self.addressis now ignored, so anyone binding to a non-default host/port loses that ability. Please parse the configured address (at least the port) and forward it intorr.serve_grpc, restoring remote setups.- if self.server: - server_uri = rr.serve_grpc(default_blueprint=blueprint) + if self.server: + host, _, port_str = self.address.partition(":") + grpc_port = int(port_str) if port_str else None + server_uri = rr.serve_grpc( + grpc_port=grpc_port, + default_blueprint=blueprint, + )
175-179: Convert indices to int32 before launching_compute_normals.
_compute_normalsrequiresindiceswith dtypewp.int32, but here we forward the original buffer, which is typicallywp.uint32. Warp raisesTypeError: expected array(dtype=wp.int32)when we hit meshes without normals. Please recast towp.int32locally before callingwp.launch.if normals is None: normals = wp.zeros_like(points) - wp.launch(_compute_normals, dim=len(indices_np), inputs=[points, indices, normals], device=self.device) + indices_i32 = ( + indices + if indices.dtype == wp.int32 + else wp.array(indices_np.reshape(-1), dtype=wp.int32, device=self.device) + ) + wp.launch(_compute_normals, dim=len(indices_np), inputs=[points, indices_i32, normals], device=self.device)
📜 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.rst(7 hunks)newton/_src/viewer/viewer_rerun.py(10 hunks)pyproject.toml(3 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 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:
pyproject.toml
📚 Learning: 2025-08-21T15:51:07.824Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 592
File: pyproject.toml:64-70
Timestamp: 2025-08-21T15:51:07.824Z
Learning: Since pip 21.2 (released 2021), Python packages can reference their own optional dependencies within other optional dependency groups using self-referencing syntax like "package-name[extra]" in pyproject.toml. This allows for clean dependency composition without inlining all dependencies.
Applied to files:
pyproject.toml
📚 Learning: 2025-08-21T15:51:07.824Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 592
File: pyproject.toml:64-70
Timestamp: 2025-08-21T15:51:07.824Z
Learning: Self-referencing optional dependencies (e.g., "package[extra]") have been supported since pip 21.2 and are valid in pyproject.toml. However, build backends differ in metadata emission: some flatten dependencies while others preserve self-references. This can affect lockfile generators and tooling. Testing with the specific build backend and dependency tools is recommended to ensure expected behavior.
Applied to files:
pyproject.toml
📚 Learning: 2025-08-27T23:33:32.527Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 660
File: pyproject.toml:28-28
Timestamp: 2025-08-27T23:33:32.527Z
Learning: In Python projects, it's normal and correct to have different warp-lang version specifications across files: pyproject.toml should specify minimum compatible versions using >= for flexibility, while specific environments like asv.conf.json can pin exact versions using == for reproducibility. These serve different purposes and should not be "aligned" to the same version.
Applied to files:
pyproject.toml
📚 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:
pyproject.tomlnewton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-08-25T20:10:51.009Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/examples/ik/example_ik_h1.py:0-0
Timestamp: 2025-08-25T20:10:51.009Z
Learning: In Warp, creating a new wp.array() with an existing array and different shape (e.g., wp.array(existing_array, shape=new_shape)) creates a view into the same underlying memory buffer, not a copy. This means modifications through either view are automatically reflected in both arrays.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-17T21:34:05.267Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_granular.py:44-47
Timestamp: 2025-09-17T21:34:05.267Z
Learning: wp.Mesh (Warp mesh objects) expose triangle indices through the `indices` attribute, not `triangles`. The correct usage is `wp.Mesh(...).indices`, not `wp.Mesh(...).triangles`.
Applied to files:
newton/_src/viewer/viewer_rerun.py
🧬 Code graph analysis (1)
newton/_src/viewer/viewer_rerun.py (2)
newton/_src/utils/mesh.py (1)
create_plane_mesh(405-446)newton/_src/viewer/viewer.py (6)
log_array(509-510)log_scalar(513-514)log_geo(390-473)_get_world_extents(135-183)log_mesh(484-494)log_points(505-506)
🪛 GitHub Actions: Pull Request
newton/_src/viewer/viewer_rerun.py
[error] 22-22: Sphinx build warning treated as error. 'Inline emphasis start-string without end-string' in docstring. Command: 'uv run --extra docs sphinx-build -W -b html docs docs/_build/html'.
⏰ 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 (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (5)
docs/guide/visualization.rst (3)
12-12: Excellent: Cross-references improve API documentation quality.Replacing generic class references with explicit Sphinx directives (
:class:cross-references) throughout the documentation is a solid improvement. This generates proper links in the built documentation and improves discoverability of the public API across all viewer backends (ViewerGL, ViewerFile, ViewerUSD, ViewerRerun, ViewerNull).Also applies to: 67-67, 135-135, 157-157, 234-234, 266-266, 270-270, 274-274, 278-278, 282-282
184-186: Clear documentation of new default memory behavior and flag.The narrative clearly explains that ViewerRerun keeps no historical data by default (preventing unbounded memory growth), and documents how to opt-in to historical retention via
keep_historical_data_in_viewer=True. This directly supports the PR objective of enabling longer and more memory-efficient simulations.
195-226: Jupyter notebook support section: Example code is incomplete.The new Jupyter notebook documentation is a valuable addition, but the example code (lines 210–221) is missing critical context:
- Line 215:
trangeis used without importing fromtqdm- Lines 218, 223:
sim_timeandframe_dtare used without initialization- Lines 210–213: No setup of
modelandstateobjectsAdditionally, verify that the claim on lines 197–198 that ViewerRerun "automatically detects if it is running inside a Jupyter notebook environment and automatically generates an output widget" is accurate and matches the actual implementation behavior.
Consider either providing a complete, runnable example with all imports and variable initialization, or explicitly marking it as pseudocode/simplified example. For reference, the ViewerGL example (lines 15–27) similarly omits full context, but this is acceptable for documentation if consistent with the established pattern.
If this is intentionally simplified to focus on ViewerRerun-specific usage, add a comment like
# ... (model and state setup as shown in other examples)to signal to readers what is expected.pyproject.toml (2)
83-83: Self-referential extra for docs is appropriate.Adding
"newton[importers]"to the docs group makes sense given the new USD mesh loading and API documentation additions. This follows the established pattern and ensures documentation builds have access to USD and mesh-processing dependencies.
93-100: Notebook dependencies confirmed compatible and well-structured.rerun-sdk[notebook] version 0.26.2 supports Python 3.10+, aligning with your target versions. The versions pinned (JupyterLab 4.0.0+, ipykernel 6.29.0+) are appropriate and no version conflicts exist. The
tqdmduplication between this group and the transitivenewton[examples]→devgroup is intentional and harmless, making dependencies explicit in the notebook group.
|
Are there OSRB self-checkout tickets filed to use rerun-sdk + its transitive deps? |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/viewer/viewer_rerun.py (1)
205-247: Guardvertex_colorsinlog_instancesto avoid unbound variableIn the first-call path of
log_instances:if colors is not None: colors_np = self._to_numpy(colors).astype(np.float32) ... vertex_colors = np.tile(color_rgb, (num_vertices, 1)) # Log the base mesh with optional colors mesh_3d = rr.Mesh3D(..., vertex_colors=vertex_colors)
vertex_colorsis only defined whencolorsis notNone, so callinglog_instanceswithcolors=Nonefor a newnamewill raise anUnboundLocalError.A small guard keeps this robust for any caller:
- if name not in self._instances: - mesh_data = self._meshes[mesh] - - # Handle colors ... - if colors is not None: - colors_np = self._to_numpy(colors).astype(np.float32) - ... - vertex_colors = np.tile(color_rgb, (num_vertices, 1)) - - # Log the base mesh with optional colors - mesh_3d = rr.Mesh3D( - vertex_positions=mesh_data["points"], - triangle_indices=mesh_data["indices"], - vertex_normals=mesh_data["normals"], - vertex_colors=vertex_colors, - ) + if name not in self._instances: + mesh_data = self._meshes[mesh] + + vertex_colors = None + if colors is not None: + colors_np = self._to_numpy(colors).astype(np.float32) + ... + vertex_colors = np.tile(color_rgb, (num_vertices, 1)) + + mesh_3d = rr.Mesh3D( + vertex_positions=mesh_data["points"], + triangle_indices=mesh_data["indices"], + vertex_normals=mesh_data["normals"], + vertex_colors=vertex_colors, + )This keeps current behavior when colors are provided but makes the API safe for future uses that omit them.
♻️ Duplicate comments (2)
newton/_src/viewer/viewer_rerun.py (2)
52-61:addressparameter is still ignored in server mode
__init__documentsaddressas the server address/port, but the code always calls:if self.server: server_uri = rr.serve_grpc(default_blueprint=blueprint)so any non-default
addresspassed by the user is silently ignored, and they can’t control the listening port as before.Consider parsing the port from
self.addressand forwarding it to Rerun:- if self.server: - server_uri = rr.serve_grpc(default_blueprint=blueprint) + if self.server: + host, _, port_str = self.address.partition(":") + grpc_port = int(port_str) if port_str else None + server_uri = rr.serve_grpc(grpc_port=grpc_port, default_blueprint=blueprint)so remote/CI setups relying on a specific port keep working.
Also applies to: 86-92, 109-121
435-467: Fix planegeo_scaleindexing for single-axis / “infinite” planesThe plane override assumes
geo_scalehas at least two components:if geo_scale[0] == 0.0 or geo_scale[1] == 0.0: ... else: width = geo_scale[0] length = geo_scale[1] if len(geo_scale) > 1 else 10.0When
geo_scaleis a single value (e.g.,(10.0,)) or empty,geo_scale[1]raisesIndexError, which can happen with common “infinite plane” encodings.ViewerBase.log_geoalready uses a safer pattern; this override should mirror it and also treat non‑positive values as infinite:- if geo_type == newton.GeoType.PLANE: - # Handle "infinite" planes encoded with non-positive scales - if geo_scale[0] == 0.0 or geo_scale[1] == 0.0: - extents = self._get_world_extents() - if extents is None: - width, length = 10.0, 10.0 - else: - max_extent = max(extents) * 1.5 - width = max_extent - length = max_extent - else: - width = geo_scale[0] - length = geo_scale[1] if len(geo_scale) > 1 else 10.0 + if geo_type == newton.GeoType.PLANE: + # Handle “infinite” planes encoded with missing or non-positive scales + width = geo_scale[0] if len(geo_scale) >= 1 else 0.0 + length = geo_scale[1] if len(geo_scale) >= 2 else width + if width <= 0.0 or length <= 0.0: + extents = self._get_world_extents() + if extents is None: + width, length = 10.0, 10.0 + else: + max_extent = max(extents) * 1.5 + width = max_extent + length = max_extentThe rest of the method (mesh creation and int32 indices) can remain unchanged.
🧹 Nitpick comments (3)
newton/_src/utils/import_usd.py (1)
31-31: Centralizing mesh loading onusd.get_meshis good; consider passingmaxhullvertvia the APISwitching both visual meshes and
MeshShapecollision meshes tousd.get_mesh(prim)is a solid consolidation of USD parsing (normals, UVs, orientation). For the collision branch you currently:resolved_maxhullvert = R.get_value(..., default=mesh_maxhullvert, ...) mesh = usd.get_mesh(prim) mesh.maxhullvert = resolved_maxhullvert builder.add_shape_mesh(scale=scale, mesh=mesh, **shape_params)Since
get_meshalready accepts amaxhullvertargument, you can avoid mutating the mesh afterwards and keep intent in one place:- resolved_maxhullvert = R.get_value(..., default=mesh_maxhullvert, ...) - mesh = usd.get_mesh(prim) - mesh.maxhullvert = resolved_maxhullvert + resolved_maxhullvert = R.get_value(..., default=mesh_maxhullvert, ...) + mesh = usd.get_mesh(prim, maxhullvert=resolved_maxhullvert)Functionally equivalent, but tighter and less error‑prone if
get_meshstarts usingmaxhullvertinternally beyond simply storing it.Also applies to: 381-387, 1380-1392
newton/_src/viewer/viewer_rerun.py (2)
343-393: Consider clearing lines when asked to “hide” them
ViewerBase.log_contactsand_log_jointscalllog_lines(name, None, None, None)to hide contacts/joints. In this implementation:if hidden: return if starts is None or ends is None: return # Nothing to logno Rerun call is made, so previously logged line strips remain visible when toggling
show_contacts/show_jointsoff.To mirror other viewers’ semantics, you could clear the entity when
starts/endsisNone:- if starts is None or ends is None: - return # Nothing to log + if starts is None or ends is None: + rr.log(name, rr.Clear(recursive=False)) + returnso UI toggles reliably hide contact and joint overlays instead of leaving stale geometry on screen.
78-79: Minor docstring typo: “scala” → “scalar”The
keep_scalar_historyargument description currently says “historical scala data”. Consider correcting this to “historical scalar data” for clarity:- keep_scalar_history (bool): If True, historical scala data logged via :meth:`ViewerRerun.log_scalar` is kept in the viewer. + keep_scalar_history (bool): If True, historical scalar data logged via :meth:`ViewerRerun.log_scalar` is kept in the viewer.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/images/rerun_notebook_example.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
docs/api/newton.rst(1 hunks)docs/guide/visualization.rst(7 hunks)newton/_src/utils/import_usd.py(3 hunks)newton/_src/viewer/viewer.py(3 hunks)newton/_src/viewer/viewer_rerun.py(10 hunks)newton/examples/basic/example_basic_shapes.py(2 hunks)pyproject.toml(2 hunks)
🧰 Additional context used
🧠 Learnings (19)
📚 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:
pyproject.toml
📚 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:
pyproject.toml
📚 Learning: 2025-08-21T15:51:07.824Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 592
File: pyproject.toml:64-70
Timestamp: 2025-08-21T15:51:07.824Z
Learning: Self-referencing optional dependencies (e.g., "package[extra]") have been supported since pip 21.2 and are valid in pyproject.toml. However, build backends differ in metadata emission: some flatten dependencies while others preserve self-references. This can affect lockfile generators and tooling. Testing with the specific build backend and dependency tools is recommended to ensure expected behavior.
Applied to files:
pyproject.toml
📚 Learning: 2025-08-21T15:51:07.824Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 592
File: pyproject.toml:64-70
Timestamp: 2025-08-21T15:51:07.824Z
Learning: Since pip 21.2 (released 2021), Python packages can reference their own optional dependencies within other optional dependency groups using self-referencing syntax like "package-name[extra]" in pyproject.toml. This allows for clean dependency composition without inlining all dependencies.
Applied to files:
pyproject.toml
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/_src/viewer/viewer_rerun.pynewton/examples/basic/example_basic_shapes.pynewton/_src/viewer/viewer.py
📚 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:
newton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-17T21:34:05.267Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_granular.py:44-47
Timestamp: 2025-09-17T21:34:05.267Z
Learning: wp.Mesh (Warp mesh objects) expose triangle indices through the `indices` attribute, not `triangles`. The correct usage is `wp.Mesh(...).indices`, not `wp.Mesh(...).triangles`.
Applied to files:
newton/_src/viewer/viewer_rerun.pynewton/_src/viewer/viewer.py
📚 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:
newton/examples/basic/example_basic_shapes.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: In NVIDIA Warp's Newton physics library, wp.transform supports direct numerical indexing (e.g., body_tf[0], body_tf[1], body_tf[2] for position and body_tf[3], body_tf[4], body_tf[5], body_tf[6] for quaternion components) to access its elements. This is the standard API used throughout the codebase.
Applied to files:
newton/examples/basic/example_basic_shapes.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/examples/basic/example_basic_shapes.py
📚 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:
newton/_src/utils/import_usd.py
📚 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:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:04:06.287Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/utils/import_usd.py:364-365
Timestamp: 2025-08-12T18:04:06.287Z
Learning: The quat_between_axes function in Newton accepts AxisType parameters, which includes strings like "X", "Y", "Z" as well as Axis enum values and integers. The function internally handles conversion using Axis.from_any(), so passing strings directly to quat_between_axes is valid and correct behavior.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:04:06.287Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/utils/import_usd.py:364-365
Timestamp: 2025-08-12T18:04:06.287Z
Learning: The quat_between_axes function in Newton accepts AxisType parameters, which includes both Axis enum values and strings. The function internally handles string-to-Axis conversion using Axis.from_any(), so passing strings directly to quat_between_axes is valid and correct.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-09-17T21:34:47.200Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_multi_material.py:84-90
Timestamp: 2025-09-17T21:34:47.200Z
Learning: wp.array supports fill_ method on indexed/sliced arrays, including advanced indexing with integer arrays that creates wp.indexedarray objects. Operations like arr[indices].fill_(value) work correctly in Warp.
Applied to files:
newton/_src/viewer/viewer.py
📚 Learning: 2025-09-17T21:47:26.087Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_granular.py:162-165
Timestamp: 2025-09-17T21:47:26.087Z
Learning: wp.Mesh constructor takes parameters in the order: points, indices, velocities (optional), support_winding_number, bvh_constructor. The second positional parameter is indices, not velocities.
Applied to files:
newton/_src/viewer/viewer.py
📚 Learning: 2025-08-25T20:10:51.009Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/examples/ik/example_ik_h1.py:0-0
Timestamp: 2025-08-25T20:10:51.009Z
Learning: In Warp, creating a new wp.array() with an existing array and different shape (e.g., wp.array(existing_array, shape=new_shape)) creates a view into the same underlying memory buffer, not a copy. This means modifications through either view are automatically reflected in both arrays.
Applied to files:
newton/_src/viewer/viewer.py
🧬 Code graph analysis (4)
newton/_src/viewer/viewer_rerun.py (2)
newton/_src/utils/mesh.py (1)
create_plane_mesh(405-446)newton/_src/viewer/viewer.py (6)
log_array(531-532)log_scalar(535-536)log_geo(412-495)_get_world_extents(139-187)log_mesh(506-516)log_points(527-528)
newton/examples/basic/example_basic_shapes.py (1)
newton/_src/usd/utils.py (1)
get_mesh(580-797)
newton/_src/utils/import_usd.py (4)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-42)newton/_src/geometry/inertia.py (1)
compute_sphere_inertia(33-52)newton/_src/usd/utils.py (1)
get_mesh(580-797)newton/_src/sim/builder.py (1)
add_shape_mesh(3176-3212)
newton/_src/viewer/viewer.py (1)
newton/_src/geometry/types.py (2)
indices(234-235)indices(238-240)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
🔇 Additional comments (6)
docs/api/newton.rst (1)
63-64: JOINT_LIMIT_UNLIMITED constant doc looks fineThe new constant entry is clear and consistent; just ensure the value stays in sync with the actual
newton.JOINT_LIMIT_UNLIMITEDdefinition when that changes.pyproject.toml (1)
80-88: Docs and notebook extras are well-factoredAdding
newton[importers]todocsand a dedicatednotebookextra (pulling innewton[examples],rerun-sdk[notebook],tqdm,jupyterlab) cleanly wires up USD docs and Jupyter/Rerun usage without bloating core dependencies. This matches the existing self-referential extras pattern.Also applies to: 90-96
docs/guide/visualization.rst (1)
12-12: Viewer docs and Rerun/Jupyter section align with the new APIThe updated references to
ViewerGL/File/USD/Rerun/Null, the explanation ofkeep_historical_data, and the Jupyter notebook workflow (includinguv run --extra notebook jupyter labandviewer.show_notebook()/viewer) all match the implementation inViewerBaseandViewerRerun. No issues from a code/API perspective.Also applies to: 67-67, 135-135, 157-187, 195-236, 243-243, 275-292
newton/examples/basic/example_basic_shapes.py (1)
27-27: Example correctly switched tonewton.usd.get_meshUsing
Usd.Stage.Open(...)plusnewton.usd.get_mesh(usd_stage.GetPrimAtPath("/root/bunny"))and passing the resultingMeshintobuilder.add_shape_meshis consistent with the new USD utility API and removes duplicated mesh-extraction logic from the example.Also applies to: 31-31, 74-79
newton/_src/viewer/viewer.py (1)
139-207: World-extents helper and int32 mesh indices look correctThe new
_get_world_extentshelper plus_auto_compute_world_offsetsrefactor cleanly centralize multi‑world spacing logic, with sensible early exits when there is ≤1 world or no valid bounds. Initializing bounds with ±wp.infand masking out untouched worlds avoids bogus extents, and calling this only fromset_modelkeeps overhead low.Switching
log_geo’s MESH/CONVEX_MESH indices towp.int32matches theMesh.indicesdtype and the_compute_normalskernel signature used byViewerRerun, removing the earlier uint32/int32 mismatch risk.Also applies to: 445-445
newton/_src/viewer/viewer_rerun.py (1)
139-203: Normals computation path is consistent with mesh indices now
log_meshnow derivesindices_npfor Rerun while handing the original Warpindicesarray to_compute_normals, which expectswp.int32. Given that:
newton.Mesh.indicesis stored asnp.int32andViewerBase.log_geonow createswp.array(..., dtype=wp.int32)for MESH/CONVEX_MESH, and_log_trianglespassesmodel.tri_indices.flatten()(already int32 in the core geometry),the normals kernel should now receive int32 indices everywhere it’s invoked, avoiding the earlier uint32 vs int32 TypeError while still computing vertex-averaged normals.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
newton/_src/viewer/viewer_rerun.py (3)
447-460: Fix IndexError for single-element plane scales.Line 449 accesses
geo_scale[1]without checking array length. Whengeo_scalehas only one element (e.g.,(10.0,)), this raisesIndexError. Line 459 properly checks length, but the condition at line 449 does not.Apply this diff to guard the index access:
if geo_type == newton.GeoType.PLANE: # Handle "infinite" planes encoded with non-positive scales - if geo_scale[0] == 0.0 or geo_scale[1] == 0.0: + width = geo_scale[0] if len(geo_scale) >= 1 else 0.0 + length = geo_scale[1] if len(geo_scale) >= 2 else width + if width <= 0.0 or length <= 0.0: extents = self._get_world_extents() if extents is None: width, length = 10.0, 10.0 else: max_extent = max(extents) * 1.5 width = max_extent length = max_extent - else: - width = geo_scale[0] - length = geo_scale[1] if len(geo_scale) > 1 else 10.0
112-115: Custom server address is ignored.Line 115 calls
rr.serve_grpc(default_blueprint=blueprint)without usingself.address(stored at line 87). Users cannot specify a custom address/port, which is a functional regression from the previous implementation.Apply this diff to restore custom address support:
if self.server: - server_uri = rr.serve_grpc(default_blueprint=blueprint) + host, _, port_str = self.address.partition(":") + grpc_port = int(port_str) if port_str else None + server_uri = rr.serve_grpc(grpc_port=grpc_port, default_blueprint=blueprint)
175-180: Fix dtype mismatch in normals computation.The
_compute_normalskernel (line 568) expectsindices: wp.array(dtype=wp.int32), but theindicesparameter passed tolog_meshmay bewp.uint32(e.g., from primitive geometry at line 493 of viewer.py). Line 177 passes the warp array directly without dtype conversion, causing aTypeErrorat runtime.Apply this diff to ensure type compatibility:
if normals is None: normals = wp.zeros_like(points) - wp.launch(_compute_normals, dim=len(indices_np), inputs=[points, indices, normals], device=self.device) + indices_i32 = indices if indices.dtype == wp.int32 else wp.array(indices_np.astype(np.int32), dtype=wp.int32, device=self.device) + wp.launch(_compute_normals, dim=len(indices_np), inputs=[points, indices_i32, normals], device=self.device) # normalize the normals wp.map(wp.normalize, normals, out=normals)
🧹 Nitpick comments (1)
newton/_src/utils/import_usd.py (1)
1387-1388: Consider passingmaxhullvertdirectly toget_mesh.Instead of loading the mesh with the default
maxhullvertand then overwriting it, pass the resolved value directly to the function call for cleaner code.Apply this diff:
- mesh = usd.get_mesh(prim) - mesh.maxhullvert = resolved_maxhullvert + mesh = usd.get_mesh(prim, maxhullvert=resolved_maxhullvert)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/images/rerun_notebook_example.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
docs/api/newton.rst(1 hunks)docs/guide/visualization.rst(7 hunks)newton/_src/utils/import_usd.py(3 hunks)newton/_src/viewer/viewer.py(3 hunks)newton/_src/viewer/viewer_rerun.py(10 hunks)newton/examples/basic/example_basic_shapes.py(2 hunks)pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/examples/basic/example_basic_shapes.py
- docs/guide/visualization.rst
- docs/api/newton.rst
🧰 Additional context used
🧠 Learnings (17)
📚 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:
newton/_src/utils/import_usd.py
📚 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:
newton/_src/utils/import_usd.py
📚 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:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:04:06.287Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/utils/import_usd.py:364-365
Timestamp: 2025-08-12T18:04:06.287Z
Learning: The quat_between_axes function in Newton accepts AxisType parameters, which includes strings like "X", "Y", "Z" as well as Axis enum values and integers. The function internally handles conversion using Axis.from_any(), so passing strings directly to quat_between_axes is valid and correct behavior.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-08-12T18:04:06.287Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/utils/import_usd.py:364-365
Timestamp: 2025-08-12T18:04:06.287Z
Learning: The quat_between_axes function in Newton accepts AxisType parameters, which includes both Axis enum values and strings. The function internally handles string-to-Axis conversion using Axis.from_any(), so passing strings directly to quat_between_axes is valid and correct.
Applied to files:
newton/_src/utils/import_usd.py
📚 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:
pyproject.toml
📚 Learning: 2025-08-21T15:51:07.824Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 592
File: pyproject.toml:64-70
Timestamp: 2025-08-21T15:51:07.824Z
Learning: Since pip 21.2 (released 2021), Python packages can reference their own optional dependencies within other optional dependency groups using self-referencing syntax like "package-name[extra]" in pyproject.toml. This allows for clean dependency composition without inlining all dependencies.
Applied to files:
pyproject.toml
📚 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:
pyproject.toml
📚 Learning: 2025-08-21T15:51:07.824Z
Learnt from: shi-eric
Repo: newton-physics/newton PR: 592
File: pyproject.toml:64-70
Timestamp: 2025-08-21T15:51:07.824Z
Learning: Self-referencing optional dependencies (e.g., "package[extra]") have been supported since pip 21.2 and are valid in pyproject.toml. However, build backends differ in metadata emission: some flatten dependencies while others preserve self-references. This can affect lockfile generators and tooling. Testing with the specific build backend and dependency tools is recommended to ensure expected behavior.
Applied to files:
pyproject.toml
📚 Learning: 2025-09-17T21:34:05.267Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_granular.py:44-47
Timestamp: 2025-09-17T21:34:05.267Z
Learning: wp.Mesh (Warp mesh objects) expose triangle indices through the `indices` attribute, not `triangles`. The correct usage is `wp.Mesh(...).indices`, not `wp.Mesh(...).triangles`.
Applied to files:
newton/_src/viewer/viewer.pynewton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-17T21:34:47.200Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_multi_material.py:84-90
Timestamp: 2025-09-17T21:34:47.200Z
Learning: wp.array supports fill_ method on indexed/sliced arrays, including advanced indexing with integer arrays that creates wp.indexedarray objects. Operations like arr[indices].fill_(value) work correctly in Warp.
Applied to files:
newton/_src/viewer/viewer.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/_src/viewer/viewer.pynewton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-09-17T21:47:26.087Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_granular.py:162-165
Timestamp: 2025-09-17T21:47:26.087Z
Learning: wp.Mesh constructor takes parameters in the order: points, indices, velocities (optional), support_winding_number, bvh_constructor. The second positional parameter is indices, not velocities.
Applied to files:
newton/_src/viewer/viewer.py
📚 Learning: 2025-08-25T20:10:51.009Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/examples/ik/example_ik_h1.py:0-0
Timestamp: 2025-08-25T20:10:51.009Z
Learning: In Warp, creating a new wp.array() with an existing array and different shape (e.g., wp.array(existing_array, shape=new_shape)) creates a view into the same underlying memory buffer, not a copy. This means modifications through either view are automatically reflected in both arrays.
Applied to files:
newton/_src/viewer/viewer.pynewton/_src/viewer/viewer_rerun.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/viewer/viewer_rerun.py
📚 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:
newton/_src/viewer/viewer_rerun.py
🧬 Code graph analysis (3)
newton/_src/utils/import_usd.py (2)
newton/_src/usd/utils.py (1)
get_mesh(580-797)newton/_src/sim/builder.py (1)
add_shape_mesh(3176-3212)
newton/_src/viewer/viewer.py (1)
newton/_src/geometry/types.py (2)
indices(234-235)indices(238-240)
newton/_src/viewer/viewer_rerun.py (2)
newton/_src/utils/mesh.py (1)
create_plane_mesh(405-446)newton/_src/viewer/viewer.py (5)
log_array(531-532)log_scalar(535-536)_get_world_extents(139-187)log_mesh(506-516)log_points(527-528)
🔇 Additional comments (13)
newton/_src/utils/import_usd.py (2)
31-31: LGTM on the import refactoring.The removal of
Meshfrom the direct imports is correct since the code now usesusd.get_mesh(prim)to obtain mesh instances rather than constructing them directly.
381-389: LGTM on the visual mesh loading refactor.The code correctly uses the new
usd.get_mesh(prim)API and passes the mesh object toadd_shape_mesh. The defaultmaxhullvertvalue is appropriate for visual shapes.pyproject.toml (2)
80-80: LGTM: Docs now includes importers for comprehensive USD API coverage.Adding
"newton[importers]"to docs enables documentation of the new USD mesh loading APIs. This aligns with the PR's introduction of public USD utilities and is consistent with the enriched summary noting expanded API documentation.
90-96: Dependency version mismatch: rerun-sdk[notebook]>=0.27.1 does not exist.The latest stable release of rerun-sdk on PyPI is 0.26.2 (released Oct 27, 2025), but
pyproject.tomlspecifiesrerun-sdk[notebook]>=0.27.1, which does not exist. This will cause installation failures. Update the version to>=0.26.2or verify that 0.27.1 is an intentional pre-release version.The
jupyterlab>=4.4.10constraint is acceptable; version 4.4.10 is the latest stable on PyPI (released Oct 22, 2025).⛔ Skipped due to learnings
Learnt from: shi-eric Repo: newton-physics/newton PR: 592 File: pyproject.toml:64-70 Timestamp: 2025-08-21T15:51:07.824Z Learning: Self-referencing optional dependencies (e.g., "package[extra]") have been supported since pip 21.2 and are valid in pyproject.toml. However, build backends differ in metadata emission: some flatten dependencies while others preserve self-references. This can affect lockfile generators and tooling. Testing with the specific build backend and dependency tools is recommended to ensure expected behavior.Learnt from: dylanturpin Repo: newton-physics/newton PR: 635 File: newton/_src/sim/builder.py:3191-3195 Timestamp: 2025-08-25T20:20:35.899Z Learning: When reviewing PRs that update Python version requirements (e.g., dropping Python 3.9 support), focus on syntax modernization rather than suggesting behavioral changes. Adding explicit `strict=False` to zip() calls is appropriate for Python 3.10+ compatibility and maintains existing behavior while satisfying linters.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.newton/_src/viewer/viewer.py (2)
139-187: LGTM! Well-structured helper method.The
_get_world_extentsmethod properly handles edge cases (no model, no valid worlds) and uses appropriate dtypes for bounds computation.
189-207: LGTM! Clean refactoring.Delegating to
_get_world_extentsimproves code organization and maintainability.newton/_src/viewer/viewer_rerun.py (7)
43-50: LGTM! Useful conversion helper.The
_to_numpystatic method provides clean conversion logic for warp arrays and array-like objects.
126-137: LGTM! Smart blueprint generation.The method conditionally includes the time series panel only when scalars are logged, providing a cleaner UI when not needed.
229-279: LGTM! Proper instance handling.The method correctly uses
_to_numpyfor conversions, hides the reference mesh (line 249), and respects thekeep_historical_dataflag for thestaticparameter.
344-392: LGTM! Comprehensive line logging.The implementation properly handles edge cases (hidden, None, empty arrays) and supports both single and per-line colors.
409-433: LGTM! Smart blueprint update on first scalar.The method dynamically adds the time series panel when the first scalar is logged (lines 428-431), improving the initial viewer UX.
470-524: LGTM! Robust point rendering.The method handles various input shapes for radii and colors, broadcasting scalars appropriately (lines 494-499, 506-511).
565-582: LGTM! Standard normal computation.The kernel correctly computes per-face normals and accumulates them at vertices using atomic operations.
| def _is_jupyter_notebook(): | ||
| try: | ||
| # Check if get_ipython is defined (available in IPython environments) | ||
| shell = get_ipython().__class__.__name__ | ||
| if shell == "ZMQInteractiveShell": | ||
| # This indicates a Jupyter Notebook or JupyterLab environment | ||
| return True | ||
| elif shell == "TerminalInteractiveShell": | ||
| # This indicates a standard IPython terminal | ||
| return False | ||
| else: | ||
| # Other IPython-like environments | ||
| return False | ||
| except NameError: | ||
| # get_ipython is not defined, so it's likely a standard Python script | ||
| return False |
There was a problem hiding this comment.
Add linting suppression for magic function.
Line 550 uses get_ipython(), which is a magic function defined in IPython environments. Add a # noqa comment to prevent linting errors.
Apply this diff:
- shell = get_ipython().__class__.__name__
+ shell = get_ipython().__class__.__name__ # noqa: F821🤖 Prompt for AI Agents
In newton/_src/viewer/viewer_rerun.py around lines 547 to 562, the call to
get_ipython() (line ~550) triggers linter warnings about an undefined/magic
function; suppress that linting error by appending a noqa comment to the
get_ipython() call (e.g., get_ipython() # noqa) so static analyzers ignore the
magic-function complaint while leaving the rest of the function unchanged.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/api/newton_geometry.rst(1 hunks)newton/_src/usd/utils.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/api/newton_geometry.rstnewton/_src/usd/utils.py
📚 Learning: 2025-09-23T14:47:05.974Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/examples/mpm/example_mpm_grain_rendering.py:151-153
Timestamp: 2025-09-23T14:47:05.974Z
Learning: In Newton's MPM grain rendering system, the `points_per_particle` CLI argument should remain as `type=float` rather than `type=int` because it's used in arithmetic calculations (e.g., `grain_radius = options.voxel_size / (3 * options.points_per_particle)`) where fractional values provide meaningful precision, even though some downstream functions eventually convert it to integer dimensions.
Applied to files:
newton/_src/usd/utils.py
🧬 Code graph analysis (1)
newton/_src/usd/utils.py (1)
newton/_src/geometry/types.py (6)
Mesh(116-314)indices(234-235)indices(238-240)normals(243-244)uvs(247-248)copy(187-222)
🪛 GitHub Actions: Pull Request
newton/_src/usd/utils.py
[error] 1-1: DOCTEST failure during Sphinx run: TypeError: object of type 'NoneType' has no len() in doctest for api/_generated/newton.usd.get_mesh. Command: 'uv run --extra docs sphinx-build -W -b doctest docs docs/_build/doctest'.
🪛 Ruff (0.14.4)
newton/_src/usd/utils.py
666-668: Avoid specifying long messages outside the exception class
(TRY003)
677-679: Avoid specifying long messages outside the exception class
(TRY003)
769-769: 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). (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 GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (4)
newton/_src/usd/utils.py (3)
39-45: LGTM: Well-defined overloads for type safety.The overload declarations properly distinguish between nullable and non-nullable return types based on the default parameter.
478-577: LGTM: Robust geometric utility functions.The helper functions for normal processing implement well-established geometric algorithms with proper handling of degenerate cases and numerical stability.
580-798: Well-structured mesh loader with comprehensive normal handling.The
get_meshfunction provides three well-documented strategies for faceVarying normal conversion, proper triangulation, and respects USD conventions. The implementation addresses previous review feedback on assertions, dtype consistency, and UV duplication.docs/api/newton_geometry.rst (1)
36-36: LGTM: Documentation addition for heightfield_to_mesh.The addition of
heightfield_to_meshto the public API documentation is straightforward and follows the existing format.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Description
Allows running the Rerun visualizer from a Jupyter notebook:
The Rerun viewer has been improved:
keep_historical_data_in_viewerViewerRerun setting (False by default). Using this it is possible to run the viewer with fairly complex simulations, multiple parallel worlds.The
newton.usdmodule now has aget_meshfunction that parses a triangle mesh from USD mesh prims and handles normals parsing while respecting the mesh face orientation flag (face-varying normals get converted to vertex-varying automatically).Closes #1043.
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
New Features
Updates
Examples
Documentation
Chores