core: Reject zero-size bindings in createBindGroup #8734
Conversation
wgpu-core/src/device/global.rs
Outdated
| fid.assign(Fallible::Invalid(Arc::new(label.to_string()))); | ||
| } | ||
|
|
||
| pub fn create_bind_group_error( |
There was a problem hiding this comment.
One question, why is this needed when Firefox is able to do this without it? Might we worth a note on the call why we need to do this, as this method is discouraged.
There was a problem hiding this comment.
I think Firefox has the same issue; wouldn't it be better to change the type to Option<u64> at least in wgpu-core? So that users of the API don't have to do this validation outside (Deno, Firefox, Servo, wgpu-native).
There was a problem hiding this comment.
In my head I think I was assuming Firefox had already handled it, but it doesn't look like it has, so I agree it makes sense to have wgpu-core do this.
There was a problem hiding this comment.
Even if Firefox did do the validation: if we can centralize validation here, that makes everything better for everyone.
Previously they were prevented by the type system. Because JavaScript APIs cannot do that, allow requesting a zero-size binding in the API to wgpu-core and then reject it with an error. The `wgpu` API remains the same.
andyleiserson
force-pushed
the
push-tvppwyusplvp
branch
from
5c357cb to
f4022fa
Compare
andyleiserson
changed the title
cwfitzgerald
left a comment
cwfitzgerald
left a comment
There was a problem hiding this comment.
LGTM,B (Looks groovy to me, baby)
4 participants
Fixes a CTS failure caused by mapping
Some(0)toNonewhen converting toOption<NonZeroU64>for the wgpu-core API. A zero-size binding is illegal. Omitting the size of a binding is valid and means to bind until the end of the buffer. Change the wgpu-core API to allowSome(0)and then reject it with an error. wgpu API remains the same.Testing
Enables the relevant CTS test.
Squash or Rebase? Squash