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

Fix not refitting upward from leaf nodes #82482

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Sep 28, 2023

Previously, the wrong node id (the root node id) was used. Dirty leaf nodes do not actually recalculate aabb.

Additionally, when requesting a new leaf, mark dirty as false in clear().

Make sure to only mark the leaf as dirty when shrinking the border of the leaf when removing items.

In other cases, the leaf node's aabb will get the correct result immediately.

  1. When adding an item, the leaf nodes will be calculated immediately.
  2. Removing the item within the border of the leaf node has no effect on the original aabb.

Fix #82436 (comment).

Previously, the wrong node id (root node id) was used. Dirty leaf nodes
do not actually recalculate aabb.

Additionally, when requesting a new leaf, mark `dirty` as `false` in `clear()`.

Make sure to only mark the leaf as **dirty** when shrinking the border of
the leaf when removing items.

In other cases, the leaf node's aabb will get the correct result immediately.
1. When adding an item, the leaf nodes will be calculated immediately.
2. Removing the item within the border of the leaf node has no effect on the
original aabb.
@Rindbee Rindbee requested a review from a team as a code owner September 28, 2023 12:32
@lawnjelly
Copy link
Member

lawnjelly commented Sep 28, 2023

Great work!

I'm just doing more testing on this with a 2D benchmark, and there are indeed quite a lot of refits going on per tick now (although they don't seem to be taking the frame rate down). This is somewhat artificial - I have 10000 moving boxes, and thus the border items on each leaf will be moving every frame, thus nearly every leaf is made dirty and asking for a refit every frame. But even so this scenario can happen.

I'll have a look in a follow up PR if we can reduce this burden as it is probably a bit excessive as these should be done slowly and incrementally.

Also note we need to duplicate any changes in 3.x so we can keep them in sync. I usually do duplicate PRs I don't know if there's any special magic for this, so I just do manually. 👍

UPDATE:
I've done a test limiting refits to one leaf per frame, and I'm not sure there is a massive difference, so maybe this just isn't a bottleneck. Will do some more profiling, but this looks good to go imo.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Sep 28, 2023
@YuriSizov YuriSizov changed the title Fix not refitting upward from leaf nodes. Fix not refitting upward from leaf nodes Sep 28, 2023
@YuriSizov YuriSizov merged commit fbe611e into godotengine:master Sep 28, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@lawnjelly
Copy link
Member

@Rindbee reminder to also make 3.x version. I can do this (if you don't have time), but the snag is I can't self-approve my own PRs, so if you make the PR I can approve and it should get merged quicker.

@Rindbee
Copy link
Contributor Author

Rindbee commented Sep 28, 2023

I compared the differences of bvh* files in master and 3.x, it could be possible to cherry pick. I also prepared another PR (#82502) for quick merge.

@Rindbee Rindbee deleted the fix-bugs-in-bvh branch September 28, 2023 22:14
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.

3 participants