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

BVH - fix stale current_tree in deactivate function #49057

Merged
merged 1 commit into from
May 26, 2021

Conversation

lawnjelly
Copy link
Member

Fix a bug in deactivate where current_tree variable was stale. This may have resulted in visual anomalies.

Changes passing of current_tree from a member variable to a function argument, making bugs due to stale state less likely.

May fix #48749
May fix #48790

Notes

  • Deactivate is only currently used in the render tree, as part of showing and hiding objects.
  • This is split from BVH - thread safety option #48892. Originally that PR did two things, the fix here, and also added thread safety. After extensive testing for thread contention neither myself or @pouleyKetchoupp could see it happening, so we can make a separate decision about adding thread safety and it makes sense to separate the PRs. I will change that PR to only include the thread safety.
  • I've also changed the name of the variable from current_tree to tree_id, as that name seems to make more sense when passing it as an argument rather than using it as a state.

@lawnjelly lawnjelly requested a review from a team as a code owner May 25, 2021 09:40
Changes passing of current_tree from a member variable to a function argument, making bugs due to stale state less likely.

Fix a bug in deactivate where current_tree variable was stale. This may have resulted in visual anomalies.
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Looks good to me. I haven't spotted any problem while testing in render-intensive and physics-intensive projects.

@akien-mga akien-mga merged commit e9909b7 into godotengine:3.x May 26, 2021
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the bvh_current_tree branch May 26, 2021 09:33
@pouleyKetchoupp
Copy link
Contributor

We should also port this PR to master to keep things in sync (although it's used only for physics for now on master so the bug is not present).

@akien-mga
Copy link
Member

Cherry-picked for 3.3.3.

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