-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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 a command buffer leak that occurs in D3D12 #100151
Fix a command buffer leak that occurs in D3D12 #100151
Conversation
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.
Makes sense to me.
I would like @DarioSamo or @RandomShaper to take a quick look as well before we merge
The rationale makes sense. However, the implementation fails to follow the 'as close to zero allocations as possible' mindset. I think you could use |
@alessand10 Could you make the change requested by RandomShaper? I think after doing that, this should be ready to merge |
6ed73b1
to
080cd48
Compare
10021a3
to
2b7ea62
Compare
You accidentally wiped your branch when trying to restore, to get back to before the rebase please use |
@AThousandShips Sorry for the trouble. Will I need to create a new PR? |
No you can solve this one, try:
And then try to rebase again, and then finally:
I'd help but I'm away from my computer right now |
Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable. If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase |
3d54eb5
to
0095d4d
Compare
@clayjohn @AThousandShips Thank you for your help and guidance! |
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.
Sweet!
Just left a little suggestion, but feel free to ignore it. I consider this is fine the way it already is.
83cc8e7
to
83cc152
Compare
83cc152
to
9ea97c0
Compare
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.
Thanks for your quick responses to feedback! That was a bit of a whirlwind day. But it looks great to merge now
@clayjohn My second PR was definitely more chaotic than I was aiming for, but I appreciate everyone's help and I'm looking forward to learning more about the engine and contribution process. Thanks again! |
Thanks! |
This PR should fix: #94251
Memory allocated for command buffers in DX12 is never freed. In Vulkan, freeing the command pool also frees the command buffers that have been allocated within the pool. This fix mirrors Vulkan behavior by having each command pool track their respective command buffers. When a command pool is freed, its command buffers are as well.