-
-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optimizations and other improvements #80
Closed
Closed
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
38a2bd2
conversion to 64bit, implemented add/remove of a node, add sphere, ca…
dbenson24 a4691fd
fix lints
dbenson24 196560c
parallelize bvh build using rayon
dbenson24 07c4c05
testing rayon parallelization further
dbenson24 3deb2e3
bench test for flamegraph
dbenson24 e5cc3ec
fixing bugs in add/remove, change optimize to just add then remove th…
dbenson24 3b002e3
more qbvh testing
dbenson24 192eed9
reduced allocations in bvh build
dbenson24 1a152b4
completely remove allocations, bit of incorrect logic tho
dbenson24 12e6ead
Revert "completely remove allocations, bit of incorrect logic tho"
dbenson24 da98c79
refactor to prevent stack overflows
dbenson24 5e15776
rewrite to use interoptopus for the ffi bindings
dbenson24 8bd25ed
use smallvec for traversal to avoid panics, handle 0 sized bvhs, fix …
dbenson24 d2dc3bf
start convex generation
dbenson24 169dc30
update interoptopus and prep for f32/f64 features
dbenson24 c0d0d99
refactoring of f64 into real type, move ffi bindings to subcrate
dbenson24 71a080f
merge with upstream, fix optimize bug
dbenson24 d72ff4e
fix docs
dbenson24 2d23f39
add 100p sponza intersection bench
dbenson24 fbc607c
refactor into 32 and 64bit sub crates, optimize build using thread_lo…
dbenson24 c52c1ca
running cargo-fix
dbenson24 f8479a6
remove unneeded clone on optimize
dbenson24 b2e5d86
Merge branch 'master' of https://github.com/svenstaro/bvh
dbenson24 468190e
finish merging with upstream, add missing documentation
dbenson24 902b0c6
rename packages to prepare for upstreaming
dbenson24 fe22c91
changes to support ray tracing
dbenson24 93cd4e6
adjust trait lifetimes, add sphere and ray normals, fix building empt…
dbenson24 b137f65
remove dyn dispatch from iterator, fix sphere compilation error
dbenson24 9e40523
add best_first traversal to bvh, run cargo fmt
dbenson24 f80bf0a
put ray normal calc back in
dbenson24 e8fe6de
fix clippy errors, delete svg file
dbenson24 fd0b05e
get rid of transmute using a safer way
dbenson24 88a9155
run cargo fmt
dbenson24 c55c37d
fix all clippy warnings
dbenson24 016ac8d
move triangle impl, fix test failure
dbenson24 0b822d4
fix subcrate profiles, small tweaks
dbenson24 64263c9
fix all clippy errors in the benchmarks
dbenson24 904b86e
move ffi to a standalone repo + crate
dbenson24 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,41 +1,12 @@ | ||
[package] | ||
name = "bvh" | ||
description = "A fast BVH using SAH" | ||
version = "0.6.0" | ||
edition = "2018" | ||
authors = [ | ||
"Sven-Hendrik Haase <[email protected]>", | ||
"Alexander Dmitriev <[email protected]>" | ||
] | ||
readme = "README.md" | ||
repository = "https://github.com/svenstaro/bvh" | ||
documentation = "https://docs.rs/crate/bvh" | ||
keywords = ["bvh", "bounding", "volume", "sah", "aabb"] | ||
license = "MIT" | ||
|
||
[dependencies] | ||
approx = "0.5" | ||
rand = "0.8" | ||
log = "0.4" | ||
num = "0.4" | ||
glam = "0.20" | ||
serde = { optional = true, version = "1", features = ["derive"] } | ||
|
||
[dev-dependencies] | ||
proptest = "1.0" | ||
obj-rs = "0.7" | ||
float_eq = "0.7" | ||
criterion = "0.3" | ||
|
||
[features] | ||
bench = [] | ||
# Unfortunately can't use "serde" as the feature name until https://github.com/rust-lang/cargo/issues/5565 lands | ||
serde_impls = ["serde", "glam/serde"] | ||
[workspace] | ||
members = ["bvh", "bvh-f64"] | ||
|
||
[profile.release] | ||
lto = true | ||
codegen-units = 1 | ||
debug = true | ||
|
||
[profile.bench] | ||
lto = true | ||
codegen-units = 1 | ||
debug = true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
[package] | ||
name = "bvh-f64" | ||
description = "A fast dynamic BVH using SAH" | ||
version = "0.6.0" | ||
edition = "2018" | ||
authors = [ | ||
"Sven-Hendrik Haase <[email protected]>", | ||
"Alexander Dmitriev <[email protected]>" | ||
] | ||
readme = "README.md" | ||
repository = "https://github.com/svenstaro/bvh" | ||
documentation = "https://docs.rs/crate/bvh" | ||
keywords = ["bvh", "bounding", "volume", "sah", "aabb"] | ||
license = "MIT" | ||
|
||
|
||
[lib] | ||
name = "bvh_f64" | ||
path = "../src/lib.rs" | ||
required-features = ["f64"] | ||
doctest = false | ||
|
||
[dependencies] | ||
approx = "0.5" | ||
rand = "0.8" | ||
log = "0.4" | ||
num = "0.4" | ||
glam = "0.20" | ||
rayon = "1.5.1" | ||
smallvec = "1.6.1" | ||
serde = { optional = true, version = "1", features = ["derive"] } | ||
|
||
|
||
[dev-dependencies] | ||
proptest = "1.0" | ||
obj-rs = "0.7" | ||
float_eq = "0.7" | ||
criterion = "0.3" | ||
itertools = "0.10.1" | ||
serde = { version = "1", features = ["derive"] } | ||
glam = { version = "0.20", features = ["serde"] } | ||
serde_json = "1" | ||
|
||
[features] | ||
default = ["f64"] | ||
bench = [] | ||
f64 = [] | ||
# Unfortunately can't use "serde" as the feature name until https://github.com/rust-lang/cargo/issues/5565 lands | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was stabilized in Rust 1.60. |
||
serde_impls = ["serde", "glam/serde"] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
[package] | ||
name = "bvh" | ||
description = "A fast dynamic BVH using SAH" | ||
version = "0.6.0" | ||
edition = "2018" | ||
authors = [ | ||
"Sven-Hendrik Haase <[email protected]>", | ||
"Alexander Dmitriev <[email protected]>" | ||
] | ||
readme = "README.md" | ||
repository = "https://github.com/svenstaro/bvh" | ||
documentation = "https://docs.rs/crate/bvh" | ||
keywords = ["bvh", "bounding", "volume", "sah", "aabb"] | ||
license = "MIT" | ||
|
||
|
||
[lib] | ||
name = "bvh" | ||
path = "../src/lib.rs" | ||
required-features = [] | ||
|
||
[dependencies] | ||
approx = "0.5" | ||
rand = "0.8" | ||
log = "0.4" | ||
num = "0.4" | ||
glam = "0.20" | ||
rayon = "1.5.1" | ||
smallvec = "1.6.1" | ||
serde = { optional = true, version = "1", features = ["derive"] } | ||
|
||
|
||
[dev-dependencies] | ||
proptest = "1.0" | ||
obj-rs = "0.7" | ||
float_eq = "0.7" | ||
criterion = "0.3" | ||
itertools = "0.10.1" | ||
serde = { version = "1", features = ["derive"] } | ||
glam = { version = "0.20", features = ["serde"] } | ||
serde_json = "1" | ||
|
||
[features] | ||
default = [] | ||
bench = [] | ||
# Unfortunately can't use "serde" as the feature name until https://github.com/rust-lang/cargo/issues/5565 lands | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was stabilized in Rust 1.60. |
||
serde_impls = ["serde", "glam/serde"] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a specialized crate for this? Can't we use traits for this to have Rust do the specialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be pretty cool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has nalgebra fixed the issues that made this crate switch from nalgebra to glam previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Elabajaba do you happen to know what those issues would be? I could look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that most of those issues have been addressed. It does still have a slower compilation time than glam