Switch to using Query in insert generator and tree traversal #305
Conversation
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
|
Comparing 8f22a7e with 722e82a: The most problematic are 1M hollow meshes on CUDA, about 5-7% slowdown. But this is acceptable. |
dalg24
requested changes
May 18, 2020
There was a problem hiding this comment.
This PR comes too early.
Without introducing the overload
template <typename ExecutionSpace, typename Predicates, typename Callback>
BVH::query(ExecutionSpace const &, Predicates const &, Callback const &);and observing that the change you propose reconciliate the signature of the user-provided callback with what actually happens in the tree traversal implementation, this PR has a very weak rational.
And even so, say we have already introduced the new query overload, we would probably still need to make the case that this is better than wrapping the user callback.
Comment on lines
+249
to
+252
| using NativePredicateType = std::decay_t<decltype( | ||
| Traits::Access<Predicates, Traits::PredicatesTag>::get( | ||
| std::declval<Predicates const &>(), std::declval<int>()))>; | ||
|
|
There was a problem hiding this comment.
Suggested change
| using NativePredicateType = std::decay_t<decltype( | |
| Traits::Access<Predicates, Traits::PredicatesTag>::get( | |
| std::declval<Predicates const &>(), std::declval<int>()))>; | |
| using NativePredicateType = typename Traits::Helper<Access>::type; |
| { | ||
| auto const predicate_index = getData(query); | ||
| auto const &predicate = static_cast<NativePredicateType const &>(query); |
There was a problem hiding this comment.
Comment that you are slicing. This is easy to overlook.
|
OK, we can close this right now if you want. I wanted to show that there is a way to change the callback inside tree traversal to be closer to what we want with minor performance penalty. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.