refactor(metal): Use descriptive names in update_bind_group_state
#8628
Conversation
* Give the `set_*` arguments descriptive names. * Introduce a helper to recover some characters so the preceding change does not cause lines to split.
inner-daemons
left a comment
inner-daemons
left a comment
There was a problem hiding this comment.
Thanks for pinging me on this one, this is definitely the direction we want to be heading here. A number of places, some of which I didn't comment on, where we could maybe move the big match blocks to an out-of-the-way function. Otherwise LGTM
| #[expect(clippy::too_many_arguments)] | ||
| fn update_bind_group_state( | ||
| &mut self, | ||
| stage: naga::ShaderStage, | ||
| render_encoder: Option<&metal::RenderCommandEncoder>, | ||
| compute_encoder: Option<&metal::ComputeCommandEncoder>, |
There was a problem hiding this comment.
Glad we've been able to get rid of some of this!
wgpu-hal/src/metal/command.rs
Outdated
| E::Vertex(encoder) => encoder.set_vertex_buffer(index, buf_ptr, offset), | ||
| E::Fragment(encoder) => encoder.set_fragment_buffer(index, buf_ptr, offset), | ||
| E::Task(encoder) => encoder.set_object_buffer(index, buf_ptr, offset), | ||
| E::Mesh(encoder) => encoder.set_mesh_buffer(index, buf_ptr, offset), | ||
| E::Compute(encoder) => encoder.set_buffer(index, buf_ptr, offset), |
There was a problem hiding this comment.
There's some new and somewhat similar logic for mesh pipeline creation that was handled in #8139, although I doubt it would be applicable here. Still, maybe we could move this into a function on the encoder to simplify this function
There was a problem hiding this comment.
I think this is the logic from #8139?
My preference would be to defer additional refactoring (extracting these to encoder methods) until after objc2 lands, to minimize conflict resolution. Do you mind putting it off for a bit? I can file an issue so I don't forget.
There was a problem hiding this comment.
I don't really mind. Yeah most of the code I'm commenting on is originally mine, but since you introduced the Encoder struct, I'm thinking we might as well use it.
There was a problem hiding this comment.
Oh, I see what you mean now. In my head I was thinking of something more complicated -- while working on this I started looking at changing the encoder Options in CommandState to an enum, but realized that was going to be a much bigger change. When I first read your comment I was thinking of something like that.
Moving them onto Encoder isn't that bad, I'll just do it.
| let a1 = (resource_indices.buffers + index) as u64; | ||
| let a2 = Some(buf.ptr.as_native()); | ||
| let a3 = offset; |
There was a problem hiding this comment.
Eww gross, glad we got rid of "a1, a2, a3"
wgpu-hal/src/metal/command.rs
Outdated
| match encoder { | ||
| E::Vertex(encoder) => encoder.set_vertex_bytes(index, length, bytes_ptr), | ||
| E::Fragment(encoder) => encoder.set_fragment_bytes(index, length, bytes_ptr), | ||
| E::Task(encoder) => encoder.set_object_bytes(index, length, bytes_ptr), | ||
| E::Mesh(encoder) => encoder.set_mesh_bytes(index, length, bytes_ptr), | ||
| E::Compute(encoder) => encoder.set_bytes(index, length, bytes_ptr), |
There was a problem hiding this comment.
Same here, lets put this on Encoder
2 participants
As noted in the description of 7e0eac3, the order of arguments to various
set_*functions is different betweenmetalandobjc2-metal. To avoid possible confusion, this gives the variables passed for those arguments descriptive names rather thana1/a2/a3.Since the long names would cause lines to split (presumably this was part of the reason for the short names originally), also introduce a helper to recover some characters and avoid that.
Preparation for #5641. CC @madsmtm.
Testing
Refactor, relying on existing tests.
Squash or Rebase? Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.