USD-Exporting improvements #820
Conversation
Signed-off-by: Wenchao Huang <[email protected]>
Signed-off-by: Wenchao Huang <[email protected]>
Signed-off-by: Wenchao Huang <[email protected]>
📝 WalkthroughWalkthroughAdds an optional face_varying_uv parameter to log_mesh across viewer interfaces and backends. ViewerUSD implements UV primvar creation/assignment with interpolation chosen by the flag and sets both TimeCodesPerSecond and FramesPerSecond on the USD stage. Other backends only gain the new parameter and docstrings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ViewerBase
participant ViewerUSD
participant USDStage as "USD Stage"
participant MeshPrim as "Mesh Prim"
Client->>ViewerBase: log_mesh(name, points, indices, normals, uvs, ..., face_varying_uv)
ViewerBase->>ViewerUSD: log_mesh(...)
ViewerUSD->>USDStage: Ensure stage (SetTimeCodesPerSecond(fps), SetFramesPerSecond(fps))
ViewerUSD->>MeshPrim: Define/get mesh prim and set geometry
alt uvs provided
ViewerUSD->>MeshPrim: Get/create "st" primvar
alt face_varying_uv == true
note right of MeshPrim: interpolation = faceVarying\nexpect len(uvs) == len(indices)
else
note right of MeshPrim: interpolation = vertex\nexpect len(uvs) == len(points)
end
ViewerUSD->>MeshPrim: Write UV data to primvar
else no uvs
note right of ViewerUSD: Skip UV primvar handling
end
ViewerUSD-->>Client: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
WenchaoHuang
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
WenchaoHuang
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
newton/_src/viewer/viewer_rerun.py
Outdated
| indices (wp.array): Triangle indices (wp.uint32). | ||
| normals (wp.array, optional): Vertex normals (wp.vec3). | ||
| uvs (wp.array, optional): UV coordinates (wp.vec2). | ||
| uv_indices (wp.array, optional): Triangle indices (wp.uint32) for UVs |
There was a problem hiding this comment.
This argument doesn't exist here.
newton/_src/viewer/viewer_usd.py
Outdated
| assert len(uvs) == len(indices_np) | ||
| st.SetInterpolation(UsdGeom.Tokens.faceVarying) | ||
| else: | ||
| assert len(uvs) == len(points_np) |
There was a problem hiding this comment.
It would be good to raise a ValueError here with a useful error message.
There was a problem hiding this comment.
Addressed. Please take a look.
| points (wp.array): Vertex positions as a warp array of wp.vec3. | ||
| indices (wp.array): Triangle indices as a warp array of wp.uint32. | ||
| normals (wp.array, optional): Vertex normals as a warp array of wp.vec3. | ||
| uvs (wp.array, optional): UV coordinates as a warp array of wp.vec2. |
There was a problem hiding this comment.
Can you clarify here the shape of these UV coordinates and how this is affected by face_varying_uv?
newton/_src/viewer/viewer_usd.py
Outdated
| hidden (bool, optional): If True, mesh will be hidden. Default is False. | ||
| backface_culling (bool, optional): If True, enable backface culling. Default is True. | ||
| face_varying_uv (bool, optional): If True, enable face varying UV. Default is False (vertex varying). |
There was a problem hiding this comment.
Nit: maybe use a hyphen for "face-varying" and "vertex-varying".
newton/_src/viewer/viewer_rerun.py
Outdated
| uv_indices (wp.array, optional): Triangle indices (wp.uint32) for UVs | ||
| hidden (bool): Whether the mesh is hidden (unused). | ||
| backface_culling (bool): Whether to enable backface culling (unused). | ||
| face_varying_uv (bool): Whether to enable face varying UV (default: vertex varying). |
There was a problem hiding this comment.
Nit: maybe use a hyphen for "face-varying" and "vertex-varying".
Also here it would be good to clarify this flag is currently not used (always vertex-varying).
newton/_src/viewer/viewer_gl.py
Outdated
| uvs (wp.array, optional): Vertex UVs. | ||
| hidden (bool): Whether the mesh is hidden. | ||
| backface_culling (bool): Enable backface culling. | ||
| face_varying_uv (bool): Enable face varying UV. |
There was a problem hiding this comment.
Can you use the same docstring as for the other viewers?
Also here it would be good to clarify this flag is currently not used (always vertex-varying).
Signed-off-by: Wenchao Huang <[email protected]>
WenchaoHuang
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
WenchaoHuang
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
WenchaoHuang
requested a deployment
to
external-pr-approval
GitHub Actions
Waiting
WenchaoHuang
requested a deployment
to
external-pr-approval
GitHub Actions
Waiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
70-76: Fix potential UnboundLocalError forserver_uriwhenserver=False
server_uriis referenced whenlaunch_viewer=Truebut only assigned ifserver=True.Apply this diff:
- if self.server: - server_uri = rr.serve_grpc() - - # Optionally launch viewer client - if self.launch_viewer: - rr.serve_web_viewer(connect_to=server_uri) + server_uri = rr.serve_grpc() if self.server else None + + # Optionally launch viewer client + if self.launch_viewer: + rr.serve_web_viewer(connect_to=server_uri) if server_uri else rr.serve_web_viewer()
🧹 Nitpick comments (5)
newton/_src/viewer/viewer_null.py (1)
53-69: Clarify the unused flag and silence unused parameterAlign docstring with other viewers and avoid ARG002.
Apply this diff:
def log_mesh( self, name, points: wp.array, indices: wp.array, normals: wp.array = None, uvs: wp.array = None, hidden=False, backface_culling=True, face_varying_uv=False, ): """ No-op implementation for logging a mesh. Args: name: Name of the mesh. points: Vertex positions. indices: Mesh indices. normals: Vertex normals (optional). uvs: Texture coordinates (optional). hidden: Whether the mesh is hidden. backface_culling: Whether to enable backface culling. - face_varying_uv: Whether the mesh UVs are face-varying (optional). + face_varying_uv: Whether the mesh UVs are face-varying (currently not used; always vertex-varying). """ - pass + # Unused in Null backend; kept for API parity + _ = face_varying_uv + passnewton/_src/viewer/viewer_usd.py (3)
69-69: Honor theup_axisargument instead of hardcoding ZRespect the constructor parameter to avoid incorrect orientation in Y‑up pipelines.
Apply this diff:
- UsdGeom.SetStageUpAxis(self.stage, UsdGeom.Tokens.z) + axis = str(self.up_axis).upper() + axis_token = UsdGeom.Tokens.z if axis.startswith("Z") else UsdGeom.Tokens.y + UsdGeom.SetStageUpAxis(self.stage, axis_token)
148-159: Docstring: specify expected UV shapes for clarityExplicit shapes reduce ambiguity.
Suggested docstring tweak:
- Vertex‑varying: uvs shape should be [num_points, 2].
- Face‑varying: uvs shape should be [num_indices, 2] (for triangles, 3*num_faces by 2).
177-205: Validate before setting primvar and raise ValueError on mismatchesAvoid partially setting invalid data and prefer ValueError for input issues.
Apply this diff:
# Set UVs if provided (do not set every frame) if uvs is not None: primvars_api = UsdGeom.PrimvarsAPI(mesh_prim) - # Get or create the 'st' primvar - if not primvars_api.GetPrimvar("st"): - st = primvars_api.CreatePrimvar("st", Sdf.ValueTypeNames.TexCoord2fArray, UsdGeom.Tokens.vertex) - else: - st = primvars_api.GetPrimvar("st") - - st.Set(uvs.numpy().astype(np.float32)) - if face_varying_uv: if len(uvs) != len(indices_np): - raise RuntimeError( + raise ValueError( "UV count mismatch: when using face-varying UVs, " "the length of 'uvs' must equal the length of 'indices'." ) - else: - st.SetInterpolation(UsdGeom.Tokens.faceVarying) + interp = UsdGeom.Tokens.faceVarying else: if len(uvs) != len(points_np): - raise RuntimeError( + raise ValueError( "UV count mismatch: when using vertex-varying UVs, " "the length of 'uvs' must equal the length of 'points'." ) - else: - st.SetInterpolation(UsdGeom.Tokens.vertex) + interp = UsdGeom.Tokens.vertex + + # Get or create the 'st' primvar and then set values/interp + st = primvars_api.GetPrimvar("st") + if not st: + st = primvars_api.CreatePrimvar("st", Sdf.ValueTypeNames.TexCoord2fArray, interp) + else: + st.SetInterpolation(interp) + st.Set(uvs.numpy().astype(np.float32))newton/_src/viewer/viewer_rerun.py (1)
90-105: Silence unusedface_varying_uvand keep docstring alignedRerun backend doesn’t use this arg; mark it used and keep API parity.
Apply this diff:
def log_mesh( self, name, points: wp.array, indices: wp.array, normals: wp.array = None, uvs: wp.array = None, hidden=False, backface_culling=True, face_varying_uv=False, ): @@ assert normals is None or isinstance(normals, wp.array) assert uvs is None or isinstance(uvs, wp.array) + # Unused in Rerun backend; kept for API parity + _ = face_varying_uv
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newton/_src/viewer/viewer_gl.py(2 hunks)newton/_src/viewer/viewer_null.py(2 hunks)newton/_src/viewer/viewer_rerun.py(2 hunks)newton/_src/viewer/viewer_usd.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T21:47:26.087Z
Learnt from: gdaviet
PR: newton-physics/newton#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_rerun.py
🪛 Ruff (0.13.1)
newton/_src/viewer/viewer_rerun.py
90-90: Unused method argument: face_varying_uv
(ARG002)
newton/_src/viewer/viewer_usd.py
191-194: Avoid specifying long messages outside the exception class
(TRY003)
199-202: Avoid specifying long messages outside the exception class
(TRY003)
newton/_src/viewer/viewer_gl.py
203-203: Unused method argument: face_varying_uv
(ARG002)
🔇 Additional comments (2)
newton/_src/viewer/viewer_gl.py (1)
203-231: Silence unusedface_varying_uv(Ruff ARG002) and keep docstring consistent across viewersGL backend doesn’t use this arg; silence the linter and note API parity.
Apply this diff:
def log_mesh( self, name, points: wp.array, indices: wp.array, normals: wp.array = None, uvs: wp.array = None, hidden=False, backface_culling=True, face_varying_uv=False, ): @@ assert isinstance(points, wp.array) assert isinstance(indices, wp.array) assert normals is None or isinstance(normals, wp.array) assert uvs is None or isinstance(uvs, wp.array) + # Unused in GL backend; kept for API parity with other viewers + _ = face_varying_uvAlso, ensure this docstring matches other viewers verbatim for the
face_varying_uvdescription.newton/_src/viewer/viewer_usd.py (1)
65-69: Setting both TimeCodesPerSecond and FramesPerSecond is correctGood addition for accurate DCC playback.
Description
Implemented following improvements:
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