bvh
bvh copied to clipboard
svenstaro
optimizations and other improvements
#71
Opening the PR, it's definitely not ready to be merged yet there's a couple more things I'm finishing up but I wanted to open earlier so if you wanted to read things over you'd be able to
Is there any chance you could cut down the PR into smaller chunks? I realize you did a lot of refactoring but are there perhaps any smaller chunks to make reviewing easier?
The FFI stuff can very easily be separated so I'll do that. I'm not even sure what the best way to distribute that is
I think I can also pull the new shapes out into a separate change, I'll see what I can come up with
Hey, how are we looking here? Do you want to fix and then we can see how to get this integrated? If you like, I can even make you co-maintainer.
I've been a bit busy last couple of weeks haven't had time to look at breaking this apart. if that's still something you would prefer I can start working on trying to do so
Well if we can somehow do this in small incremental PRs that would be ideal but I can understand if you don't want to go down that path but it would make reviewing faster for me for sure. You get to decide. :)
I've pulled the ffi bindings out, I can add them back in if you want them in the repo otherwise I will just throw them up in a separate repo.
In terms of reviewing this, the big change that affects the whole crate is refactoring all the types to use the vec/float types defined at the crate root. Otherwise the changes are pretty much file by file but because of that change to all of the types it's kind of a pain to break this down into little commits.
So I decided to try building a toy pathtracer against this branch, and I'm curious about how users are supposed to use the included shapes?
Currently none of them can actually be used in the BVH since none of them have a BHShape impl, so you can't actually include them in the BVH. If you're supposed to shoot rays and intersect them against the included shapes, it'd be nice if they used a Structure of Arrays (SoA) layout (a simple SoA layout gets me ~2x the performance with 100 spheres, or ~3x with 1000 spheres in my pathtracer), however because intersects_ray (from the IntersectionRay impl) doesn't have a way to return an index, you can't currently use your own SOA layouts.
I believe IntersectionAABB also doesn't have any way to return an index, so it also can't be used with SOA layouts.
edit: Remove outdated info.
@Elabajaba the shapes missing impls are just because I haven't had time to write that code. This PR stems entirely from me writing other code and adding stuff to BVH as I needed it. In terms of the shapes not being able to be used in SOA, I chose not to attach an index to any of the shapes because it's very easy if you want to do that yourself. Just define an IndexedShape<T> that stores the shape and it's index. I didn't think that it made sense to force an index onto every shape.
Because of the discussion in the other thread: I wouldn't mind a co-maintainer on this. I don't use it much myself currently and I think someone with more stakes in this could really drive it forward a bit.
Because of the discussion in the other thread: I wouldn't mind a co-maintainer on this. I don't use it much myself currently and I think someone with more stakes in this could really drive it forward a bit.
I may be interested, I plan to use it quite a bit in the future. I am a bit new to the rust side of things, but I generally worked with c++ metaprogramming before my switch to game engine development in rust.
Because of the discussion in the other thread: I wouldn't mind a co-maintainer on this. I don't use it much myself currently and I think someone with more stakes in this could really drive it forward a bit.
I may be interested, I plan to use it quite a bit in the future. I am a bit new to the rust side of things, but I generally worked with c++ metaprogramming before my switch to game engine development in rust.
How about you start off by making one or two PRs that I'll review and then I can make you contrib from there if we click well?
@marstaik how do you wanna play this PR? Shall we close it? Shall we leave it open until you have implemented the bits that you are interested in? I don't think the original author is going to return to this any time soon and the rebase is very gnarly at this point.
I'm going to close this, the original author has already started porting the changes incrementally starting with this: https://github.com/svenstaro/bvh/pull/99