-
Notifications
You must be signed in to change notification settings - Fork 948
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
Implement a missing part of bind group layout deduplication #4200
Conversation
Do you think we can wait to do this after Arcanization or do you prefer me to re-implement/re-check it after your submit? |
It blocks some important stuff in firefox and our deadlines are getting pretty tight, so we can wait for a few days but not much longer (we actually held off #3925 for about 6 weeks because we thought arcanization was about to land, but as you know it's a difficult beast to push over the finish line!). |
Oki, this week I'll be away for work, so I'll be able to be back on this during the weekend. If you need to go on, no problem, I'll catch up 👍🏼 |
@gents83: Does this actually look like a significant amount of effort to adjust arcanization for? I don't think there are any significant changes to how tracking with |
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.
Crazily enough (given my newness to wgpu
), I actually feel confident in approving on this. @nical onboarded me to his solution synchronously yesterday, which helped a great deal. 🙂
Left mostly non-blocking feedback. For most code style nitpicks, I left fixup!
commits. Gonna trust @nical to address feedback before merging, and leave a green checkmark here for now.
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.
Looks good :)
@nical merge when ready, don't want to merge early if you're not done adjusting. |
Checklist
cargo clippy
.Connections
Description
render_pipeline_get_bind_group_layout
should do bind group layout de-duplication, but it currently does not.It will probably help the review to consider the two commits separately:
render_pipeline_get_bind_group_layout
. The latter must create a new (duplicate) BGL if the ID is provided externally but was missing that part before this commit.I wrote this with the assumption that the
MultiRefCount
should be zero at the creation of a bind group layout, I believe it is correct but it would not hurt to look at it with a critical eye during the review.@gents83 This PR will probably collide a bit with arcanization as most do I assume. The main thing that matters for arcanization is the change in the second commit where instead of creating a new BGL, a duplicate that references an existing one is created, similarly to what was done in #3925
Testing
That's covered by the CTS in Firefox, but unfortunately tedious to test here because wgpu does not exercise this code path.