Fix TetMesh._infer_frequency for length-1 attribute arrays - #3240
Conversation
When a custom attribute array has length 1 and no geometry count (vertex_count, tet_count, tri_count) also equals 1, infer AttributeFrequency.ONCE instead of raising ValueError. When a geometry count does equal 1, raise an ambiguity error asking for an explicit (array, frequency) tuple. Fixes newton-physics#3228
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTetMesh USD loading now separates cached geometry from custom-attribute import, flattens indexed primvars, infers unambiguous length-1 attributes as ChangesTetMesh USD custom attributes
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant USD
participant get_tetmesh
participant TetMesh
participant ModelBuilder
USD->>get_tetmesh: Load geometry and flattened custom values
get_tetmesh->>TetMesh: Construct and validate TetMesh attributes
get_tetmesh-->>ModelBuilder: Provide cached TetMesh geometry
ModelBuilder->>USD: Read registered custom attributes
ModelBuilder->>TetMesh: Attach frequency-validated attributes
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Is it possible to skip attributes with frequencies that are unable to be inferred at all, instead of erroring out? If user has some custom attributes with weird size, but are irrelevant to the newton solver, usd parsing will error out even though the attributes are not relevant to Newton. It would be nice for Newton to print a warning regarding the bad attributes, instead of erroring out. |
Agreed. I am hitting the same issue in IL.
Newton should not hard-fail on an unregistered custom attribute that will be discarded. |
|
@eric-heiden Do you plan to rework and fix the issue? We hit a bug related to this - NVBUG#6445358. |
Keep direct TetMesh construction strict while making USD loading tolerant of arbitrary authored arrays. Warn and omit only attributes whose frequency is ambiguous or unavailable so irrelevant PhysX data cannot discard the whole soft body.
Use ModelBuilder declarations as the authoritative source for USD attribute names and frequencies during soft-body import. Ignore unrelated vendor metadata without losing the TetMesh, and diagnose malformed or unsupported registered arrays. Flatten indexed primvars so direct TetMesh loading and add_usd preserve the expanded value order.
|
@eric-heiden @huidongc @mmichelis I have a few follow up commits regarding this PR on this branch: https://github.com/mzamoramora-nvidia/newton/tree/mzamoramora/pr-3240-merge-ready |
|
@mzamoramora-nvidia yes, that will fix the error. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/_src/geometry/types.py (1)
1438-1445: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
| Noneto thedevicetype hint.As per coding guidelines, use PEP 604 unions for optional arguments. Since the
deviceparameter defaults toNone, its type should explicitly be annotated asDevicelike | None.
newton/_src/geometry/types.py#L1438-L1445: changedevice: Devicelike = Nonetodevice: Devicelike | None = NoneinMesh.finalize.newton/_src/geometry/types.py#L2435-L2440: changedevice: Devicelike = Nonetodevice: Devicelike | None = NoneinGaussian.finalize.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/geometry/types.py` around lines 1438 - 1445, Update the device parameter annotation in Mesh.finalize and Gaussian.finalize in newton/_src/geometry/types.py at lines 1438-1445 and 2435-2440, respectively, from Devicelike to Devicelike | None while retaining the None default.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@newton/_src/geometry/types.py`:
- Around line 1438-1445: Update the device parameter annotation in Mesh.finalize
and Gaussian.finalize in newton/_src/geometry/types.py at lines 1438-1445 and
2435-2440, respectively, from Devicelike to Devicelike | None while retaining
the None default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a84461cb-2229-4a47-a365-de1d59c044f4
📒 Files selected for processing (5)
CHANGELOG.mdnewton/_src/geometry/types.pynewton/_src/usd/utils.pynewton/_src/utils/import_usd.pynewton/_src/utils/import_usd_deformable_volume.py
Keep TetMesh.create_from_usd documentation identical to the public get_tetmesh helper. The API parity test requires both entry points to describe resolved primvar interpolation consistently.
Co-authored-by: Miguel Angel Zamora Mora <mzamoramora@nvidia.com> (cherry picked from commit db91242)
Description
Prevent unrelated USD attributes from aborting TetMesh and volume-deformable imports.
This PR:
AttributeFrequency.ONCE, while preserving the ambiguity error when a geometry count is also one;get_tetmesh()loading warn and omit custom arrays whose frequency cannot be inferred instead of rejecting the entire TetMesh;add_usd()load only builder-registered TetMesh custom attributes, using the registered frequency as authoritative;This fixes PhysX-authored volume deformables containing irrelevant metadata such as
deformablePose:default:omniphysics:purposes = ["bindPose"]orphysxVolumeDeformableSim:simMeshHexCrc. These attributes are no longer normalized eagerly and cannot prevent the simulation TetMesh from loading.Closes #3228
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Also verified manually with the reported OmniPhysics representation:
Direct
get_tetmesh()loading classifies this asONCEon an unambiguous multi-tet mesh, whileModelBuilder.add_usd()imports the soft body without warning about or retaining the unregistered metadata.Bug fix
Steps to reproduce:
UsdGeom.TetMeshwith more than one tetrahedron.deformablePose:default:omniphysics:purposestoken array on the simulation TetMesh.ModelBuilder.add_usd().add_usd()can discard the unregistered attribute.Minimal reproduction:
Summary by CodeRabbit
ModelBuilder.add_usd()now imports only registered TetMesh custom attributes with their declared frequencies.