optimizations and other improvements #80

dbenson24 · 2022-01-14T15:07:17Z

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

…psule and aabb intersection tests. Expose C FFI

…e nodes

This reverts commit 1a152b4.

…bug with using the wrong surface_area

…cals and by computing centroid bounds during the bucket phase

svenstaro · 2022-01-15T03:58:20Z

bvh-lib/Cargo.toml

+lto = true
+codegen-units = 1
+debug = true


Do we want debug here?

svenstaro · 2022-01-15T03:58:24Z

bvh-lib/Cargo.toml

+lto = true
+codegen-units = 1
+debug = true


Do we want debug here?

svenstaro · 2022-01-15T03:59:16Z

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?

dbenson24 · 2022-01-15T11:23:43Z

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

svenstaro · 2022-01-15T17:30:43Z

bvh-f64/Cargo.toml

@@ -0,0 +1,59 @@
+[package]
+name = "bvh-f64"


Do we really need a specialized crate for this? Can't we use traits for this to have Rust do the specialization?

So I tried a few approaches to this. It might be a possibility with nalgebra backing things to have the float be a generic on everything, but with glam there's no trait to abstract dvec3 from vec3 so we would have to wrap that ourselves. This trick using separate crates in the workspace is something I got from the author of nalgebra and how he builds rapier (2d 3d 32 and 64bit). When the workspace is all setup with this cargo test runs the test for all the versions and you'll get compiler errors from rust analyzer for both at the same time

I would very much like to see this move to nalgebra, it would make the usage much cleaner via Bvh<T>. This would definitely be possible with the nalgebra vector types and number bounds.

Heck, because of the AABB's, assuming there are no dot products/cross products (which I don't think there are - haven't checked), it should work with integer types as well, and maybe even unsigned types (but there would have to be better bounds checking).

I could try and help get this working with nalgebra as well.

I think that would be pretty cool.

I'm not sure how to approach this for now though, should I wait for these changes to get merged in? There are a lot of stacked up changes in this MR, so it might be easier to do after all the merge.

Hmmm it might be easier for me to port it now, and have this PR rebase on it. Otherwise we would be splitting code and then remerging.

Has nalgebra fixed the issues that made this crate switch from nalgebra to glam previously?

@Elabajaba do you happen to know what those issues would be? I could look into it.

iirc the main issues with nalgebra used to be compile times, slower runtime performance for non-simd types, and the confusing docs. I haven't really used nalgebra for a few years at this point, so I'm not sure how it compares these days.

I believe that most of those issues have been addressed. It does still have a slower compilation time than glam

dbenson24 · 2022-02-12T13:39:04Z

ok I think I've fixed all the lints now

dbenson24 · 2022-02-12T14:09:57Z

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

svenstaro · 2022-02-14T23:10:51Z

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. :)

dbenson24 · 2022-03-16T11:39:52Z

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.

Elabajaba · 2022-05-30T04:49:25Z

src/shapes/capsule.rs

+    fn intersects_aabb(&self, aabb: &AABB) -> bool {
+        /*
+        // Use Distance from closest point
+        let mut point: Vector3 = self.start;


Commented out code. Delete it if it isn't used?

Elabajaba · 2022-05-30T05:16:58Z

src/shapes/ray.rs

+        }
+
+        // Only required for bounded intersection intervals.
+        // if z_min > ray_min {


More commented out code.

Elabajaba · 2022-05-30T06:59:56Z

src/shapes/ray.rs

-            sign_z: (direction.z < 0.0) as usize,
-        }
+/// This trait can be implemented on anything that can intersect with a `Ray`
+pub trait IntersectionRay {


Do you have your own IntersectionRay impls outside of this library? I'm wondering what the performance tradeoff of using a vtable here vs an enum would be.

Also, is there a reason why IntersectionRay is only implemented for sphere+triangle, but not aabb/capsule/obb?

Elabajaba · 2022-05-30T18:56:19Z

bvh-f64/Cargo.toml

+default = ["f64"]
+bench = []
+f64 = []
+# Unfortunately can't use "serde" as the feature name until https://github.com/rust-lang/cargo/issues/5565 lands


This was stabilized in Rust 1.60.

Elabajaba · 2022-05-30T18:57:24Z

bvh/Cargo.toml

+[features]
+default = []
+bench = []
+# Unfortunately can't use "serde" as the feature name until https://github.com/rust-lang/cargo/issues/5565 lands


This was stabilized in Rust 1.60.

Elabajaba · 2022-06-02T04:25:12Z

src/mesh/convex_hull.rs

@@ -0,0 +1,5 @@
+use crate::Vector3;


Empty file.

Elabajaba · 2022-06-02T04:25:26Z

src/mesh/mod.rs

@@ -0,0 +1,2 @@
+pub mod convex_hull;


Also empty.

Elabajaba · 2022-06-02T19:24:52Z

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 · 2022-06-02T19:28:36Z

src/testbase.rs

@@ -166,7 +163,8 @@ pub fn traverse_some_bh<BH: BoundingHierarchy>() {
 }

 /// A triangle struct. Instance of a more complex `Bounded` primitive.
-#[derive(Debug)]
+#[derive(Copy, Clone, Debug)]
+#[cfg_attr(feature = "serde_impls", derive(serde::Serialize, serde::Deserialize))]
 pub struct Triangle {


I feel like this should be removed since this pr includes a triangle shape, and having 2 public triangle structs is confusing.

dbenson24 · 2022-06-12T11:40:23Z

@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 that stores the shape and it's index. I didn't think that it made sense to force an index onto every shape.

svenstaro · 2023-04-10T20:39:47Z

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.

marstaik · 2023-04-10T22:43:33Z

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.

svenstaro · 2023-04-11T01:25:26Z

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?

svenstaro · 2023-05-07T02:10:20Z

@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.

marstaik · 2023-05-07T04:45:23Z

I'm going to close this, the original author has already started porting the changes incrementally starting with this:
#99

dbenson24 added 26 commits June 15, 2021 17:12

conversion to 64bit, implemented add/remove of a node, add sphere, ca…

38a2bd2

…psule and aabb intersection tests. Expose C FFI

fix lints

a4691fd

parallelize bvh build using rayon

196560c

testing rayon parallelization further

07c4c05

bench test for flamegraph

3deb2e3

fixing bugs in add/remove, change optimize to just add then remove th…

e5cc3ec

…e nodes

more qbvh testing

3b002e3

reduced allocations in bvh build

192eed9

completely remove allocations, bit of incorrect logic tho

1a152b4

Revert "completely remove allocations, bit of incorrect logic tho"

12e6ead

This reverts commit 1a152b4.

refactor to prevent stack overflows

da98c79

rewrite to use interoptopus for the ffi bindings

5e15776

use smallvec for traversal to avoid panics, handle 0 sized bvhs, fix …

8bd25ed

…bug with using the wrong surface_area

start convex generation

d2dc3bf

update interoptopus and prep for f32/f64 features

169dc30

refactoring of f64 into real type, move ffi bindings to subcrate

c0d0d99

merge with upstream, fix optimize bug

71a080f

fix docs

d72ff4e

add 100p sponza intersection bench

2d23f39

refactor into 32 and 64bit sub crates, optimize build using thread_lo…

fbc607c

…cals and by computing centroid bounds during the bucket phase

running cargo-fix

c52c1ca

remove unneeded clone on optimize

f8479a6

Merge branch 'master' of https://github.com/svenstaro/bvh

b2e5d86

finish merging with upstream, add missing documentation

468190e

rename packages to prepare for upstreaming

902b0c6

changes to support ray tracing

fe22c91

svenstaro requested changes Jan 15, 2022

View reviewed changes

get rid of transmute using a safer way

fd0b05e

run cargo fmt

88a9155

dbenson24 added 3 commits February 12, 2022 11:36

fix all clippy warnings

c55c37d

move triangle impl, fix test failure

016ac8d

fix subcrate profiles, small tweaks

0b822d4

dbenson24 added 2 commits February 15, 2022 07:26

fix all clippy errors in the benchmarks

64263c9

move ffi to a standalone repo + crate

904b86e

Elabajaba reviewed May 30, 2022

View reviewed changes

src/shapes/ray.rs

}

// Only required for bounded intersection intervals.

// if z_min > ray_min {

Copy link

Contributor

Elabajaba May 30, 2022

Choose a reason for hiding this comment

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

More commented out code.

Elabajaba reviewed May 30, 2022

View reviewed changes

Elabajaba reviewed Jun 2, 2022

View reviewed changes

src/mesh/convex_hull.rs

@@ -0,0 +1,5 @@

use crate::Vector3;

Copy link

Contributor

Elabajaba Jun 2, 2022

Choose a reason for hiding this comment

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

Empty file.

Elabajaba reviewed Jun 2, 2022

View reviewed changes

src/mesh/mod.rs

@@ -0,0 +1,2 @@

pub mod convex_hull;

Copy link

Contributor

Elabajaba Jun 2, 2022

Choose a reason for hiding this comment

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

Also empty.

Elabajaba reviewed Jun 2, 2022

View reviewed changes

Elabajaba mentioned this pull request Dec 20, 2022

Parallel BVH creation using rayon mockersf/bvh2d#2

Draft

dbenson24 mentioned this pull request May 3, 2023

Move math types over to nalgebra with Generic dimensions > 2 and f32/f64 support #96

Merged

svenstaro requested a review from marstaik May 4, 2023 12:17

marstaik closed this May 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimizations and other improvements #80

optimizations and other improvements #80

dbenson24 commented Jan 14, 2022

svenstaro Jan 15, 2022

svenstaro Jan 15, 2022

svenstaro commented Jan 15, 2022

dbenson24 commented Jan 15, 2022

svenstaro Jan 15, 2022

dbenson24 Jan 15, 2022

marstaik Apr 10, 2023

svenstaro Apr 10, 2023

marstaik Apr 10, 2023 •

edited

Loading

marstaik Apr 10, 2023

Elabajaba Apr 10, 2023

marstaik Apr 10, 2023

Elabajaba Apr 10, 2023

dbenson24 Apr 10, 2023

dbenson24 commented Feb 12, 2022

dbenson24 commented Feb 12, 2022

svenstaro commented Feb 14, 2022

dbenson24 commented Mar 16, 2022

Elabajaba May 30, 2022

Elabajaba May 30, 2022

Elabajaba May 30, 2022

Elabajaba May 30, 2022

Elabajaba May 30, 2022

Elabajaba Jun 2, 2022

Elabajaba Jun 2, 2022

Elabajaba commented Jun 2, 2022 •

edited

Loading

Elabajaba Jun 2, 2022

dbenson24 commented Jun 12, 2022

svenstaro commented Apr 10, 2023

marstaik commented Apr 10, 2023

svenstaro commented Apr 11, 2023

svenstaro commented May 7, 2023

marstaik commented May 7, 2023

optimizations and other improvements #80

optimizations and other improvements #80

Conversation

dbenson24 commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenstaro commented Jan 15, 2022

dbenson24 commented Jan 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marstaik Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbenson24 commented Feb 12, 2022

dbenson24 commented Feb 12, 2022

svenstaro commented Feb 14, 2022

dbenson24 commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elabajaba commented Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

dbenson24 commented Jun 12, 2022

svenstaro commented Apr 10, 2023

marstaik commented Apr 10, 2023

svenstaro commented Apr 11, 2023

svenstaro commented May 7, 2023

marstaik commented May 7, 2023

marstaik Apr 10, 2023 •

edited

Loading

Elabajaba commented Jun 2, 2022 •

edited

Loading