Skip to content

[BUG FIX] Fix BVH build's radix sort.#1305

Merged
duburcqa merged 33 commits intoGenesis-Embodied-AI:mainfrom
Libero0809:bvh
Jun 28, 2025
Merged

[BUG FIX] Fix BVH build's radix sort.#1305
duburcqa merged 33 commits intoGenesis-Embodied-AI:mainfrom
Libero0809:bvh

Conversation

@Libero0809
Copy link
Contributor

@Libero0809 Libero0809 commented Jun 21, 2025

Description

Maybe continue this PR after the linear corotated is merged into main. I just realized that this branch is based on the corotated one.

  • The BVH's radix sort use an unstable sort which will generate a wrong sorting result. Changed it to sequential for each batch so that the result is correct.
  • BVH's compute bound is not gpu safe since it does not sync the memory. Changed to multi-kernel launch with forced memory sync.
  • Make the tests harder for the bvh. The previous one was too simple.
  • Break the large build kernel into several kernels to make it compile faster.

Checklist:

  • I read the CONTRIBUTING document.
  • I followed the Submitting Code Changes section of CONTRIBUTING document.
  • I tagged the title correctly (including BUG FIX/FEATURE/MISC/BREAKING)
  • I updated the documentation accordingly or no change is needed.
  • I tested my changes and added instructions on how to test it for reviewers.
  • I have added tests to cover my changes.

@Libero0809 Libero0809 changed the title [BUG FIX] Fix BVH build's radix sort and break code into kernels to accelerate compile [BUG FIX] Fix BVH build's radix sort and break one kernel into several kernels to accelerate compile Jun 21, 2025
@Libero0809 Libero0809 changed the title [BUG FIX] Fix BVH build's radix sort and break one kernel into several kernels to accelerate compile [BUG FIX] Fix BVH build's radix sort and conpute bounds, and break one kernel into several kernels to accelerate compile Jun 26, 2025
@duburcqa duburcqa changed the title [BUG FIX] Fix BVH build's radix sort and conpute bounds, and break one kernel into several kernels to accelerate compile [BUG FIX] Fix BVH build's radix sort. Jun 27, 2025
@Libero0809
Copy link
Contributor Author

👍 = Done. Saves me some typing in the future😊

@hughperkins
Copy link
Collaborator

👍 = Done. Saves me some typing in the future😊

Note that we don't get notifications for emoji. So, once you have addressed everyting you're going to address, you might consider posting something like "addressed all comments". (I'm not sure if that is standard tbh, and I typically often end up DM'ing reviewers directly anyway, but I tend to do this anywya).

@Libero0809
Copy link
Contributor Author

@hughperkins , Noted. 👍

@duburcqa duburcqa enabled auto-merge (squash) June 27, 2025 18:46
duburcqa
duburcqa previously approved these changes Jun 27, 2025
auto-merge was automatically disabled June 28, 2025 01:38

Head branch was pushed to by a user without write access

@duburcqa duburcqa enabled auto-merge (squash) June 28, 2025 10:34
@duburcqa duburcqa merged commit 45bc00e into Genesis-Embodied-AI:main Jun 28, 2025
13 checks passed
# Compute prefix sum
for i_b in ti.ndrange(self.n_batches):
self.prefix_sum[i_b, 0] = 0
for j in range(1, 256): # sequential prefix sum
Copy link
Contributor

@erizmr erizmr Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though it has been merged, just to mention there is a built-in parallel prefix sum, which might be useful: https://github.com/taichi-dev/taichi/blob/master/python/taichi/algorithms/_algorithms.py#L42

Copy link
Collaborator

@duburcqa duburcqa Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh really! This is interesting!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I think if we are doing prefix sum for just 256 elements, sequential would be fine.

Milotrince pushed a commit to Milotrince/Genesis that referenced this pull request Jul 5, 2025
chris-la-humalab pushed a commit to HumaLab/Genesis that referenced this pull request Aug 1, 2025
YilingQiao pushed a commit to YilingQiao/Genesis that referenced this pull request Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants