Make simrenderer usable without state and model#379
Conversation
…arp model and state Signed-off-by: twidmer <twidmer@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SimRenderer
participant Model
User->>SimRenderer: __init__(model, path, ...)
alt model provided
SimRenderer->>SimRenderer: infer up_axis from model
SimRenderer->>SimRenderer: populate(model)
else model is None
SimRenderer->>SimRenderer: set up_axis to default (Z)
end
SimRenderer->>SimRenderer: populate(model)
SimRenderer->>SimRenderer: populate_bodies(...)
SimRenderer->>SimRenderer: populate_shapes(...)
SimRenderer->>SimRenderer: populate_joints(...)
User->>SimRenderer: render(...)
SimRenderer->>SimRenderer: render_particles_and_springs(...)
SimRenderer->>SimRenderer: render_muscles(...)
sequenceDiagram
participant SimRenderer
participant User
User->>SimRenderer: render_contacts(body_q, contacts, contact_point_radius)
SimRenderer->>SimRenderer: Render contact points using body_q
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
newton/utils/render.py (3)
203-342: Consider breaking down this method for better maintainability.The
populate_shapesmethod is quite long (139 lines) and handles multiple responsibilities. Consider extracting the geometry creation logic into a separate method like_create_geometry_shapeto improve readability and maintainability.
402-403: Remove commented-out code.These commented lines add unnecessary clutter to the codebase.
- # if body == -1: - # continue
344-432: Consider extracting arrow creation logic.The
populate_jointsmethod could be simplified by extracting the arrow creation and scaling logic into a separate helper method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/utils/render.py(2 hunks)
🔇 Additional comments (4)
newton/utils/render.py (4)
73-97: Constructor refactoring looks good!The changes to make the model parameter optional and improve default handling align well with the PR objectives. The up_axis inference logic is clean and intuitive.
434-441: Method signature is correct despite summary inconsistency.The AI summary incorrectly states this method has no parameters, but the implementation correctly includes and uses the
instance_countparameter.Likely an incorrect or invalid review comment.
124-172: Excellent refactoring for improved modularity!The extraction of rendering logic into specialized helper methods (
populate_bodies,populate_shapes,populate_joints,render_particles_and_springs,render_muscles) significantly improves code organization and aligns perfectly with the PR objectives of making the renderer more modular and usable without requiring a full model/state.Also applies to: 452-472
474-502: Clean extraction of particle rendering logic!The
render_particles_and_springsmethod is well-structured with clear parameter handling and good separation of concerns.
…_and_model Signed-off-by: twidmer <twidmer@nvidia.com> # Conflicts: # newton/utils/render.py
Signed-off-by: twidmer <twidmer@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
eric-heiden
left a comment
There was a problem hiding this comment.
Thanks, to me it looks good overall! It's great to get a bit more modularity with these methods you've added to the SimRenderer.
Signed-off-by: twidmer <twidmer@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
newton/utils/render.py (3)
840-840: Remove unused noqa directive.The static analysis tool correctly identified an unused
noqadirective that should be removed.- from pxr import Sdf, Usd # noqa: PLC0415 + from pxr import Sdf, Usd
668-668: Fix docstring parameter description.The docstring should reflect that
up_axisdefaults to None, not just mention it's optional.- up_axis (newton.AxisType, optional): Up axis for the scene. If None, uses model's up axis. Defaults to None. + up_axis (newton.AxisType | None, optional): Up axis for the scene. If None, uses model's up axis. Defaults to None.
959-959: Update docstring to match type hint.The docstring should reflect the correct type hint for the
up_axisparameter.- up_axis (newton.AxisType, optional): Up axis for the scene. If None, uses model's up axis. Defaults to None. + up_axis (newton.AxisType | None, optional): Up axis for the scene. If None, uses model's up axis. Defaults to None.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/utils/render.py(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:65-82
Timestamp: 2025-07-12T19:46:47.510Z
Learning: In the newton-physics/newton repository AWS GPU workflow, the workflow only triggers on `pull_request_target` and `push` events, not on `pull_request` events. The `pull_request_target` event is used for testing PRs from forks safely on AWS infrastructure.
🪛 Ruff (0.11.9)
newton/utils/render.py
840-840: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-asv-benchmarks
🔇 Additional comments (10)
newton/utils/render.py (10)
68-69: Good documentation addition for the factory function.The docstring clearly explains the purpose of the factory function, improving code readability and maintainability.
75-76: Excellent modularization - model parameter is now optional.Making the model parameter optional with a default value of None is a key improvement that aligns with the PR objective of making the renderer usable without a Newton model. The default path string is also more descriptive.
97-100: Proper handling of up_axis defaults.The logic correctly falls back to the model's up_axis if available, otherwise defaults to newton.Axis.Z. This ensures consistent behavior whether a model is provided or not.
108-109: Conditional model population is correctly implemented.The conditional call to
populate()only when a model is provided is essential for supporting the use case where the renderer is used without a Newton model.
178-206: Well-structured body population method.The
populate_bodiesmethod is well-documented and handles the optional body_env parameter correctly. The logic for environment ID assignment is sound.
208-348: Complex but well-organized shape population method.The
populate_shapesmethod handles all geometry types correctly and includes proper error handling. The geometry caching using hash keys is an efficient approach.
350-439: Joint visualization method is comprehensive.The
populate_jointsmethod properly handles different joint types and includes good scaling logic based on collision radii. The arrow visualization approach is appropriate.
441-448: Simple and effective color management.The
get_new_colormethod provides a clean interface to the color mapping functionality with good documentation.
481-509: Good extraction of particle and spring rendering logic.The
render_particles_and_springsmethod successfully modularizes the rendering logic while maintaining the same functionality.
511-547: Muscle rendering method is well-implemented.The
render_musclesmethod correctly handles the muscle data structures and includes the proper None check for muscle_start that was mentioned in past review comments.
Signed-off-by: twidmer <twidmer@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/utils/render.py (1)
840-841: Remove unused noqa directive.The static analysis tool indicates the
noqadirective forPLC0415is no longer needed. Since the import doesn't appear to be triggering the warning it was meant to suppress, the directive can be safely removed.- from pxr import Sdf, Usd # noqa: PLC0415 + from pxr import Sdf, Usd
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/utils/render.py(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:65-82
Timestamp: 2025-07-12T19:46:47.510Z
Learning: In the newton-physics/newton repository AWS GPU workflow, the workflow only triggers on `pull_request_target` and `push` events, not on `pull_request` events. The `pull_request_target` event is used for testing PRs from forks safely on AWS infrastructure.
🪛 Ruff (0.11.9)
newton/utils/render.py
840-840: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-asv-benchmarks
🔇 Additional comments (12)
newton/utils/render.py (12)
67-68: Good documentation addition.The docstring clearly explains the purpose of the factory function.
75-76: Well-implemented optional model support.The parameter changes correctly implement the PR objective of making the renderer usable without a model by providing sensible defaults.
96-110: Correct handling of optional model parameter.The constructor logic properly:
- Infers up_axis from model when available, otherwise defaults to newton.Axis.Z
- Only calls populate when a model is provided
- Maintains backward compatibility
This aligns well with the PR objective of enabling usage without a Newton model.
128-177: Excellent refactoring for improved modularity.Breaking down the populate method into focused helper methods (
populate_bodies,populate_shapes,populate_joints) significantly improves code organization and maintainability while preserving the original functionality.
178-206: Well-documented and implemented body population method.The
populate_bodiesmethod properly extracts the body registration logic with clear parameter documentation and correct implementation for handling environment mapping.
208-348: Comprehensive shape population implementation.The
populate_shapesmethod handles all geometry types correctly and maintains the original functionality while improving code organization. The detailed docstring clearly explains the purpose and parameters.
350-439: Robust joint visualization implementation.The
populate_jointsmethod properly handles joint arrow visualization with appropriate scaling based on attached shape radii and correct orientation along joint axes.
459-480: Improved render method organization.Simplifying the render method by extracting particle and muscle rendering into separate methods improves readability and maintainability.
481-509: Clean particle and spring rendering extraction.The
render_particles_and_springsmethod properly handles particle rendering with optional mesh surface and spring visualization.
511-547: Correct muscle rendering implementation.The
render_musclesmethod properly handles muscle visualization with appropriate color mapping based on activation levels. The None check formuscle_startwas correctly addressed in previous commits.
648-669: Good type hint improvements.The enhanced type hints and default parameter values improve code clarity and maintain consistency with the base class changes.
84-95: Excellent documentation improvements.The comprehensive docstrings added throughout the file significantly improve code maintainability and developer experience. The parameter descriptions are detailed and helpful, making the API more accessible.
Also applies to: 114-121, 179-189, 224-242, 364-380, 488-494, 520-526, 900-904
Signed-off-by: twidmer <twidmer@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
newton/utils/recorder.py (2)
18-18: Replace deprecatedListwithlisttype annotation.The
typing.Listis deprecated in favor of the built-inlisttype for type annotations.-from typing import List +from typing import List # Can be removed entirely - self.transforms_history: List[wp.array] = [] + self.transforms_history: list[wp.array] = []Also applies to: 37-37
88-88: Add trailing newline at end of file.Consider adding a trailing newline after line 88 to follow standard file conventions.
newton/utils/render.py (1)
841-841: Remove unusednoqadirective.The static analysis indicates this
noqadirective is no longer needed.- from pxr import Sdf, Usd # noqa: PLC0415 + from pxr import Sdf, Usd
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/examples/example_quadruped.py(4 hunks)newton/utils/recorder.py(1 hunks)newton/utils/render.py(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:65-82
Timestamp: 2025-07-12T19:46:47.510Z
Learning: In the newton-physics/newton repository AWS GPU workflow, the workflow only triggers on `pull_request_target` and `push` events, not on `pull_request` events. The `pull_request_target` event is used for testing PRs from forks safely on AWS infrastructure.
🧬 Code Graph Analysis (1)
newton/utils/recorder.py (2)
newton/examples/example_quadruped.py (1)
render(250-258)newton/utils/render.py (1)
SimRendererOpenGL(947-996)
🪛 Ruff (0.11.9)
newton/utils/recorder.py
18-18: typing.List is deprecated, use list instead
(UP035)
37-37: Use list instead of List for type annotation
Replace with list
(UP006)
newton/utils/render.py
841-841: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-asv-benchmarks
🔇 Additional comments (12)
newton/utils/recorder.py (3)
39-48: Good use ofwp.cloneto store transform snapshots.The method correctly clones the transforms before storing to ensure immutability of recorded frames.
49-62: Robust bounds checking in playback method.The method includes proper bounds checking and graceful error handling for invalid frame indices.
73-87: Well-implemented file loading with proper sorting and type conversion.The method correctly handles loading saved transforms, maintaining frame order, and converting back to warp arrays with appropriate device placement.
newton/examples/example_quadruped.py (5)
37-44: Good handling of optional tkinter dependency.The try/except pattern correctly handles the optional tkinter import, allowing the code to run with reduced functionality when tkinter is unavailable.
46-134: Well-designed ImGui interface for recorder controls.The
RecorderImGuiManagerclass provides a comprehensive and user-friendly interface with proper error handling, bounds checking, and graceful degradation when tkinter is unavailable.
185-206: Proper integration of recorder with refactored renderer.The initialization correctly uses the new modular renderer API and sets up the recorder with appropriate GUI controls.
237-249: Correct implementation of pause/record logic.The step method properly handles pausing and records transforms when a recorder is present.
281-288: Interactive main loop implementation.The dual-mode main loop correctly supports interactive control when a renderer is present and falls back to fixed frame count otherwise.
newton/utils/render.py (4)
73-110: Good refactoring to support model-less renderer initialization.The optional model parameter and improved up_axis inference logic increase the renderer's flexibility for use cases where the model might not be available at initialization time.
114-177: Excellent modularization of the populate method.The delegation to specialized helper methods improves code organization and makes the renderer more maintainable and testable.
208-349: Well-implemented shape population with efficient geometry caching.The method handles all geometry types comprehensively and uses hash-based caching to avoid recreating identical geometries, improving performance.
451-480: Clean refactoring of render method with proper delegation.The simplified render method with delegation to specialized helpers improves readability and maintainability.
Signed-off-by: twidmer <twidmer@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/examples/example_quadruped.py (1)
76-76: Potential frame timing inaccuracy in playback.The frame_time calculation assumes the recording was made at the same frame rate as the current simulation (
self.example.frame_dt). This could be inaccurate if the recording was made with a different frame rate or loaded from a file.Consider storing the frame rate with the recording or making the timing display more generic:
- frame_time = self.selected_frame * self.example.frame_dt + frame_time = self.selected_frame * self.example.frame_dt # Assumes current frame rateOr enhance the Recorder class to store the original frame rate.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/examples/example_quadruped.py(4 hunks)newton/utils/recorder.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- newton/utils/recorder.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shi-eric
PR: newton-physics/newton#386
File: .github/workflows/aws-gpu-tests.yml:65-82
Timestamp: 2025-07-12T19:46:47.510Z
Learning: In the newton-physics/newton repository AWS GPU workflow, the workflow only triggers on `pull_request_target` and `push` events, not on `pull_request` events. The `pull_request_target` event is used for testing PRs from forks safely on AWS infrastructure.
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-asv-benchmarks
🔇 Additional comments (6)
newton/examples/example_quadruped.py (6)
36-42: Good defensive programming for optional dependency.The tkinter import handling with exception catching and availability flag is well-implemented to gracefully handle missing dependencies.
184-200: Excellent implementation of explicit body and shape population.This change aligns perfectly with the PR objective of making the sim renderer usable without state and model. The explicit population of bodies and shapes allows the renderer to function independently of the Newton model structure.
202-210: Well-structured integration of recording functionality.The recorder initialization and GUI integration are well-implemented, with proper null checks for headless operation.
236-237: Correct pause handling implementation.The early return when paused prevents unnecessary simulation steps while maintaining the recording/playback functionality.
255-256: Proper conditional body transform updates.The conditional update of body transforms based on pause state is correct and enables smooth playback scrubbing during pause.
288-289: Commented out renderer save functionality.The
renderer.save()call is commented out, which removes the ability to save the USD file output.Verify if this is intentional or if save functionality should be preserved. Consider:
- Interactive mode: Users might want to save after interactive session
- Batch mode: Headless operation might still need save functionality
- UI integration: Could add a save button to the ImGui interface
If save functionality should be preserved, consider adding it to the ImGui interface or providing a keyboard shortcut.
Signed-off-by: twidmer <twidmer@nvidia.com>
…_and_model # Conflicts: # newton/utils/render.py
Signed-off-by: twidmer <twidmer@nvidia.com>
Changes required for updating to be compatible with Isaac Sim 5.0 and Kit 107.3. Notable changes include: - Python version updated to 3.11, which means we need special support for rl-games since it does not include python 3.11 support out of the box. Additionally, rl-games has not updated torch.load with weights_only flag required since torch 2.6. - Updates pytorch to 2.7 for Blackwell support. We now uninstall the torch build that comes with isaac sim as part of the isaacsim.sh/bat script and forces an install of torch 2.7 with torchvision - Removal of deprecated flags in kit that no longer exist - improve_path_friction and SETTING_BACKWARD_COMPATIBILITY - Some tests are still timing out / failing - there seems to be an issue with the first load of assets that's causing checks like `stage.ResolveIdentifierToEditTarget(usd_path)` and `is_prim_path_valid` to take up to a minute. <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) - Breaking change (fix or feature that would cause existing functionality to not work as expected) - This change requires a documentation update - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Signed-off-by: rwiltz <165190220+rwiltz@users.noreply.github.com> Signed-off-by: Kelly Guo <kellyguo123@hotmail.com> Signed-off-by: Ashwin Varghese Kuruttukulam <123109010+ashwinvkNV@users.noreply.github.com> Signed-off-by: Kelly Guo <kellyg@nvidia.com> Signed-off-by: Michael Gussert <michael@gussert.com> Co-authored-by: jaczhangnv <jaczhang@nvidia.com> Co-authored-by: rwiltz <165190220+rwiltz@users.noreply.github.com> Co-authored-by: Yanzi Zhu <yanziz@nvidia.com> Co-authored-by: nv-mhaselton <mhaselton@nvidia.com> Co-authored-by: cosmith-nvidia <141183495+cosmith-nvidia@users.noreply.github.com> Co-authored-by: Michael Gussert <michael@gussert.com> Co-authored-by: CY Chen <cyc@nvidia.com> Co-authored-by: oahmednv <oahmed@Nvidia.com> Co-authored-by: Ashwin Varghese Kuruttukulam <123109010+ashwinvkNV@users.noreply.github.com> Co-authored-by: Rafael Wiltz <rwiltz@nvidia.com> Co-authored-by: Peter Du <peterd@nvidia.com> Co-authored-by: matthewtrepte <mtrepte@nvidia.com> Co-authored-by: chengronglai <chengrongl@nvidia.com> Co-authored-by: pulkitg01 <pulkitg@nvidia.com> Co-authored-by: Connor Smith <cosmith@nvidia.com> Co-authored-by: Ashwin Varghese Kuruttukulam <ashwinvk@nvidia.com>
Signed-off-by: twidmer <twidmer@nvidia.com>
Description
Modularizes the sim renderer by providing method overloads that accept flat arrays as input. This allows the renderer to be used on simulations that don't have a newton model or a newton state at hand which is often the case during early development. The viewer is fully compatible with the previous version.
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this MR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
pre-commit run -aSummary by CodeRabbit
Refactor
Documentation
Style