Honor target_voxel_size in texture SDF construction#2464
Conversation
) The texture SDF path silently ignored `target_voxel_size` and always fell back to the default `max_resolution=64`, causing the texture SDF to disagree with the sparse SDF (which honored the requested voxel size) and producing visually-mismatched collision debug meshes. The documented precedence is that `target_voxel_size` takes priority over `max_resolution` when both are provided. - `create_texture_sdf_from_mesh` now accepts an optional `target_voxel_size` and derives `max_resolution` from it, rounding up to a multiple of 8 to match the tiled SDF builders. This mirrors the conversion already done in the sparse path in `_compute_sdf_from_shape_impl`. - `SDF.create_from_mesh` and the primitive-mesh path in `ModelBuilder` now forward `target_voxel_size` into the texture call instead of dropping it. - Added four regression tests in `test_sdf_texture.py`: voxel-size scaling, precedence over `max_resolution`, end-to-end propagation through `Mesh.build_sdf`, and input validation. Fixes newton-physics#2407 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughCreate_texture_sdf_from_mesh gained a new optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/_src/geometry/sdf_utils.py`:
- Around line 398-407: Add a CHANGELOG.md entry under the [Unreleased] → Fixed
section noting that the target_voxel_size parameter now influences texture-SDF
resolution; mention that target_voxel_size is forwarded to
create_texture_sdf_from_mesh (used by the texture SDF code path) and that this
changes how max_resolution is derived for texture SDFs so users know the
externally visible behavior change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 321d94be-d24c-4115-aea6-81c14752bc07
📒 Files selected for processing (4)
newton/_src/geometry/sdf_texture.pynewton/_src/geometry/sdf_utils.pynewton/_src/sim/builder.pynewton/tests/test_sdf_texture.py
devshahofficial
left a comment
There was a problem hiding this comment.
@eric-heiden can you review? Thanks!
nvtw
left a comment
There was a problem hiding this comment.
Looks good, thanks for your contribution
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Let's merge here first and then I'll take care of fixing potential merge conflicts in #2475 |
The pre-release audit found user-facing commits that landed since v1.1.0 without an [Unreleased] entry. Backfill five of them here. Kamino-related commits (newton-physics#2280, newton-physics#2517, newton-physics#2554, newton-physics#2575) are intentionally omitted: Kamino is still alpha and not currently tracked in CHANGELOG. - Changed: contact-conversion overhead reduction in SolverMuJoCo ([newton-physicsGH-2393](newton-physics#2393)) - Fixed: target_voxel_size ignored on the texture-SDF path ([newton-physicsGH-2464](newton-physics#2464)) - Fixed: Newton-to-mujoco-warp material-combination mismatch ([newton-physicsGH-2439](newton-physics#2439)) - Fixed: eq_objtype mismatch on joint equality / mimic constraints in SolverMuJoCo ([newton-physicsGH-2459](newton-physics#2459)) - Fixed: _tiled_sum_kernel launch-dim handling under warp-lang 1.13 ([newton-physicsGH-2546](newton-physics#2546))
Summary
Fixes #2407 — the texture SDF path silently ignored
target_voxel_sizeand always fell back to the defaultmax_resolution=64, causing the texture SDF to disagree with the sparse SDF (which did honor the requested voxel size) and producing visually-mismatched collision debug meshes. The documented precedence is thattarget_voxel_sizetakes priority overmax_resolutionwhen both are provided.Root cause
In
newton/_src/geometry/sdf_utils.py::SDF.create_from_mesh:When the user passed only
target_voxel_size=0.1,effective_max_resolutionended upNone,resfell through to64, andtarget_voxel_sizewas dropped on the floor before reaching the texture-SDF builder. The same pattern existed innewton/_src/sim/builder.pyfor primitive shapes.Fix
newton/_src/geometry/sdf_texture.py—create_texture_sdf_from_meshnow accepts an optionaltarget_voxel_sizeand derivesmax_resolutionfrom it usingceil(max_padded_extent / target_voxel_size), rounded up to a multiple of 8 to match the tiled-SDF builder convention. This mirrors the conversion already done in the sparse path in_compute_sdf_from_shape_impl(sdf_utils.py:778-787), so the two paths now agree on resolution for any giventarget_voxel_size.max_resolutionis nowint | Nonewith a default ofNone; the prior behavior (64when both are unset) is preserved.newton/_src/geometry/sdf_utils.py—SDF.create_from_meshnow forwardstarget_voxel_sizeinto the texture call instead of dropping it.newton/_src/sim/builder.py— the primitive-mesh path at the hydroelastic call site now forwardssdf_target_voxel_sizeinto the texture call.ShapeConfig.configure_sdf(target_voxel_size=...)was already plumbed throughself.shape_sdf_target_voxel_sizebut discarded at the final call.Behavior
Before (on this branch without the fix):
After:
(Matches the reporter's observation in #2407.)
Test plan
Added four regression tests in
newton/tests/test_sdf_texture.py:test_texture_sdf_target_voxel_size_scales— sweepingtarget_voxel_sizeacross (0.2, 0.1, 0.05) produces a strictly-increasing number of texture blocks on a unit cube. Would have failed onmain(identical counts).test_texture_sdf_target_voxel_size_takes_precedence— passing bothmax_resolution=8andtarget_voxel_size=0.05produces more blocks thanmax_resolution=8alone, confirmingtarget_voxel_sizewins.test_mesh_build_sdf_target_voxel_size_propagates_to_texture— end-to-end path through the publicMesh.build_sdf(target_voxel_size=...)API, validating thatSDF.texture_block_coordsactually scales (the exact symptom reported in [BUG] Texture SDF generation ignorestarget_voxel_sizeprecedence #2407).test_create_texture_sdf_from_mesh_validates_target_voxel_size— invalid inputs (0.0, negative) raiseValueErrorwith a clear message.add_function_testagainstget_cuda_test_devices(), consistent with the rest oftest_sdf_texture.py; they auto-skip on non-CUDA environments.ruff check/ruff format --checkpass on all four modified files (my changes introduce no new lint errors).create_texture_sdf_from_meshstill work — the signature change is backwards-compatible (max_resolutionbecameint | Nonewith the same 64 default preserved in the no-args path).Fixes #2407
Summary by CodeRabbit
New Features
Tests