Skip to content
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

Add a Dynamic BVH implementation. #44531

Merged
merged 1 commit into from
Dec 20, 2020
Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Dec 19, 2020

-Based on Bullet Dbvh, has style and functional changes.
-Helps achieve efficient pairing
-Needed to optimize rendering
-Needed to optimize physics

core/math/dynamic_bvh.cpp Outdated Show resolved Hide resolved
core/math/dynamic_bvh.cpp Show resolved Hide resolved
core/math/dynamic_bvh.h Outdated Show resolved Hide resolved
DynamicBVH::Node *DynamicBVH::_node_sort(Node *n, Node *&r) {
Node *p = n->parent;
ERR_FAIL_COND_V(!n->is_internal(), nullptr);
if (p > n) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to compare the pointers or are we missing a comparison operator?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, it appears to be like this on Bullet too, it does not make much sense to me, I wonder why is this done this way..

Copy link
Member Author

Choose a reason for hiding this comment

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

bullet also allocates on heap, so I should assume it has some sort of meaning, but i really really cant figure it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, I would expect the comparison to have some sort of geometric meaning...

Copy link
Member Author

Choose a reason for hiding this comment

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

My wild guess is that it may be to avoid a redundant rotation or something like this, but the code is too cryptic. Maybe @lawnjelly knows?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's a variation of !=. It doesn't make much sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be related to cache efficiency? With this sorting the nodes closer to the root of the tree will end up closer in memory, and depending on the allocator they could even be in contiguous memory.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is just sorting nodes by memory address for cache optimization. It swaps parent and child completely, including their volumes so I don't think it has a geometric reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I had missed the latest comment. I agree with this interpretation :)

-Based on Bullet Dbvh, has style and functional changes.
-Provides efficient pairing
-Needed to optimize rendering
-Needed to optimize physics

This PR is up for others to review the implementation.
@qarmin
Copy link
Contributor

qarmin commented Dec 20, 2020

I think that this PR should also contains tests, because seems that results will be mostly reproducible and this will prevent from regressions like in LocalVector - #40913

@reduz
Copy link
Member Author

reduz commented Dec 20, 2020

@qarmin I agree, though its difficult to think how tests for this could be created

@reduz reduz merged commit fc4d90e into godotengine:master Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants