Use centroids for SAH partition #1096
Conversation
📝 WalkthroughWalkthroughModifies the BVH SAH partition computation in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-09-28T18:49:59.528ZApplied to files:
📚 Learning: 2025-09-28T18:45:48.198ZApplied to files:
🧬 Code graph analysis (1)warp/native/bvh.cpp (1)
🔇 Additional comments (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
Greptile OverviewGreptile SummaryThis PR improves the quality of Surface Area Heuristic (SAH) BVH builds by using centroids instead of full bounds when determining the split axis. Key Changes:
Technical Correctness: Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Caller as build_recursive
participant SAH as partition_sah_indices
participant Bounds as bounds3
Caller->>SAH: Call with lowers, uppers, indices, range_bounds
Note over SAH: Old: Used range_bounds.edges() for axis selection<br/>New: Compute centroid_bounds from item centers
loop For each item in [start, end)
SAH->>SAH: Compute item_center = 0.5 * (lower + upper)
SAH->>Bounds: centroid_bounds.add_point(item_center)
end
SAH->>SAH: edges = centroid_bounds.edges()
SAH->>SAH: split_axis = longest_axis(edges)
Note over SAH: Use centroid_bounds for range_start/range_end<br/>instead of full bounds
SAH->>SAH: range_start = centroid_bounds.lower[split_axis]
SAH->>SAH: range_end = centroid_bounds.upper[split_axis]
alt range_end <= range_start (degenerate case)
SAH-->>Caller: Return range_start (triggers midpoint fallback)
else Normal case
loop For each item in [start, end)
SAH->>SAH: Assign item to bucket based on centroid position
SAH->>SAH: Accumulate item bounds in bucket
end
SAH->>SAH: Compute SAH costs for each split point
SAH->>SAH: Find minimum cost split
SAH-->>Caller: Return split_point on split_axis
end
Caller->>Caller: Partition indices using split_point
Caller->>Caller: Recursively build left and right subtrees
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
warp/native/bvh.cpp (1)
330-336: Optional: Consider centroid-based axis selection for consistency.For consistency with the SAH approach,
partition_mediancould also compute centroid bounds and uselongest_axis(centroid_bounds.edges())instead oflongest_axis(range_bounds.edges()). The partitioning predicate already uses centroids (lines 318-319), so this would align the axis-selection logic across both SAH and median methods.Example refactor:
int TopDownBVHBuilder::partition_median(const vec3* lowers, const vec3* uppers, int* indices, int start, int end, bounds3 range_bounds) { assert(end-start >= 2); - vec3 edges = range_bounds.edges(); + bounds3 centroid_bounds; + for (int i = start; i < end; ++i) + { + vec3 item_center = 0.5f * (lowers[indices[i]] + uppers[indices[i]]); + centroid_bounds.add_point(item_center); + } + vec3 edges = centroid_bounds.edges(); int axis = longest_axis(edges);
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
warp/native/bvh.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: StafaH
Repo: NVIDIA/warp PR: 925
File: warp/native/bvh.h:343-357
Timestamp: 2025-09-28T18:49:59.528Z
Learning: In warp/native/bvh.h, CPU-built BVHs now allocate keys (may be zero-initialized), so bvh_get_group_root doesn't need null pointer guards for bvh.keys. User will document proper usage guidelines.
Learnt from: StafaH
Repo: NVIDIA/warp PR: 925
File: warp/native/bvh.h:232-234
Timestamp: 2025-09-28T18:45:51.495Z
Learning: In warp/native/bvh.h, the __ldg float4 casting pattern in bvh_load_node is acceptable because it's guarded by #ifdef __CUDA_ARCH__ and only runs in CUDA device code where alignment/aliasing behavior is predictable.
Learnt from: StafaH
Repo: NVIDIA/warp PR: 925
File: warp/native/bvh.h:359-369
Timestamp: 2025-09-28T18:45:45.955Z
Learning: StafaH prefers to defer the BVH shared stack stride issue (hardcoded WP_BVH_BLOCK_DIM vs runtime blockDim.x) for a future update rather than implementing the dynamic shared memory solution now.
📚 Learning: 2025-09-28T18:49:59.528Z
Learnt from: StafaH
Repo: NVIDIA/warp PR: 925
File: warp/native/bvh.h:343-357
Timestamp: 2025-09-28T18:49:59.528Z
Learning: In warp/native/bvh.h, CPU-built BVHs now allocate keys (may be zero-initialized), so bvh_get_group_root doesn't need null pointer guards for bvh.keys. User will document proper usage guidelines.
Applied to files:
warp/native/bvh.cpp
🧬 Code graph analysis (1)
warp/native/bvh.cpp (1)
warp/native/vec.h (1)
longest_axis(2087-2101)
🔇 Additional comments (2)
warp/native/bvh.cpp (2)
396-404: LGTM! Centroid-based axis selection improves SAH quality.The change correctly computes centroid bounds and uses them for split axis selection, avoiding bias from elongated item bounds. This is the standard approach for high-quality SAH BVH construction.
407-416: Guard condition correctly handles degenerate centroid case.When all item centroids coincide (zero extent along split axis), this guard prevents division by zero and triggers the fallback to midpoint split at lines 548-552. The comment accurately describes the behavior.
|
@StafaH (not sure if I left a comment somewhere) Can you also add a changelog entry for this change? |
|
Maybe seek @AnkaChan's guidance on whether this is considered as a bug or a change |
|
This is a mis-implementation that hinders the performance. Current implementation does not accord with the one described in Physics Based Rendering book. Mustafa changed it back to be aligned with the algorithm described in the book. I would say it's a change. Because the old implementation is totally a viable algorithm, it's just that its quality is a bit lower. |
Signed-off-by: Mustafa Haiderbhai <[email protected]> Update changelog Signed-off-by: Mustafa Haiderbhai <[email protected]>
StafaH
force-pushed
the
stafah/centroid
branch
from
4af3378 to
595473a
Compare
|
@shi-eric Changelog is updated, and linked to a change rather than a bug. |
|
I think this looks all good to me! |
3 participants
This PR improves the quality of SAH BVH builds by using centroids rather than the full bounds when deciding which axis to split along. Elongated bounds can bias the axis choice, using centroids avoid this.
Before your PR is "Ready for review"
__init__.pyi,functions.rst)pre-commit run -aSummary by CodeRabbit
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.