Open kainino0x opened 2 weeks ago
My 2 cents: It would make sense to match the JS API (using entry.bufferBinding.buffer
for nullability), leaving as-is is fine, ad-hoc renaming with "bufferBinding" prefix sounds less ideal to me.
Nothing really matches JS perfectly since JS has a real union and we have three values in a trenchcoat. Unlike GPUBindGroupLayoutEntry
which uses this trenchcoat pattern in JS and therefore needs to carry over to C, we could technically use a real tagged union here. It would honestly be less ergonomic though, and it doesn't really map any better - either Wasm has to inject an error if more than one of the three types is used, or it has to inject an error if the tag is invalid. (Or it can just debug-assert, which is probably how I'd implement it.)
All that said, I would probably pick entry.buffer.buffer
if doing this from scratch, but it doesn't really seem worth changing. We can write docs that explain what offset
and size
are.
Tentatively closing as I'd like to go with no change, but leaving the label to discuss if we want to reopen.
Nov 21 meeting:
Decided it's easier to separate this out from #438. Mirroring the option numbers there:
0 - Today:
struct WGPUBindGroupEntry {
WGPUChainedStruct const * nextInChain;
uint32_t binding;
WGPU_NULLABLE WGPUBuffer buffer;
uint64_t offset;
uint64_t size;
WGPU_NULLABLE WGPUSampler sampler;
WGPU_NULLABLE WGPUTextureView textureView;
};
static const WGPUSType WGPUSType_EmscriptenExternalTextureBinding = 0x00040003;
struct WGPUEmscriptenExternalTextureBinding {
WGPUChainedStruct chain; // WGPUSType_EmscriptenExternalTextureBinding
some_handle_type externalTexture;
};
1 - Tagged union:
// EDIT: Separated this from #438's WGPUBindingType because they're not 1:1.
enum WGPUBindingResourceType {
WGPUBindingResourceType_Buffer = 1,
WGPUBindingResourceType_Sampler = 2,
WGPUBindingResourceType_TextureView = 3,
};
static const WGPUBindingResourceType WGPUBindingResourceType_EmscriptenExternalTexture = 0x00040000;
struct WGPUBindGroupEntry {
WGPUChainedStruct const * nextInChain;
uint32_t binding;
WGPUBindingResourceType type;
union {
struct {
WGPU_NONNULL WGPUBuffer buffer;
uint64_t offset;
uint64_t size;
} buffer;
WGPU_NONNULL WGPUSampler sampler;
WGPU_NONNULL WGPUTextureView textureView;
// Pointer to a new thing, like a WGPUEmscriptenExternalTexture object or a new struct
WGPU_NONNULL void * other;
};
};
2 - Use struct chaining instead of tags (verbose because we need a chained struct for each one):
enum WGPUSType {
// ...
WGPUSType_BindGroupEntryBuffer = ...,
WGPUSType_BindGroupEntrySampler = ...,
WGPUSType_BindGroupEntryTextureView = ...,
};
struct WGPUBindGroupEntry {
WGPUChainedStruct const * nextInChain; // Everything goes in here now.
uint32_t binding;
};
// Each of these is chained in WGPUBindGroupEntry. The presence of one in the
// chain indicates which thing we're binding, so there must be exactly one in the chain.
struct WGPUBindGroupEntryBuffer {
WGPUChainedStruct chain; // WGPUSType_BindGroupEntryBuffer
WGPU_NONNULL WGPUBuffer buffer;
uint64_t offset;
uint64_t size;
};
struct WGPUBindGroupEntrySampler {
WGPUChainedStruct chain; // WGPUSType_BindGroupEntrySampler
WGPU_NONNULL WGPUSampler sampler;
};
struct WGPUBindGroupEntryTextureView {
WGPUChainedStruct chain; // WGPUSType_BindGroupEntryTextureView
WGPU_NONNULL WGPUTextureView textureView;
};
static const WGPUSType WGPUSType_EmscriptenBindGroupEntryExternalTexture = 0x00040004;
struct WGPUEmscriptenBindGroupEntryExternalTexture {
WGPUChainedStruct chain; // WGPUSType_EmscriptenBindGroupEntryExternalTexture
some_handle_type externalTexture;
};
3 - Using STypes but with a union of pointers:
// Same WGPUSType
struct WGPUBindGroupEntry {
WGPUChainedStruct const * nextInChain; // May not be needed
uint32_t binding;
union {
// Using the WGPUBindGroupEntry* definitions from 2. Tagged by SType,
// instead of by an outside tag. It is like a normal chained struct,
// except the first one in the chain must be one of the layout types.
WGPU_NONNULL WGPUBindGroupEntryBuffer * buffer;
WGPU_NONNULL WGPUBindGroupEntrySampler * sampler;
WGPU_NONNULL WGPUBindGroupEntryTextureView * textureView;
// A new thing, such as a WGPUEmscriptenBindGroupEntryExternalTexture
WGPU_NONNULL WGPUChainedStruct * other;
// Alternative: make the core ones all non-pointers; I couldn't figure
// out a non-messy way of making it extensible, though.
};
};
struct WGPUEmscriptenBindGroupEntryExternalTexture {
WGPUChainedStruct chain; // WGPUSType_EmscriptenBindGroupEntryExternalTexture
some_handle_Type externalTexture;
};
3b - Use RTTI inside the types??? (more "flat" version of 3)
struct WGPUBindGroupEntry {
WGPUChainedStruct const * nextInChain;
uint32_t binding;
// Instead of a tag, define that internally these objects are all distinguishable via
// "RTTI" - using an internal sType that lives at the same exact offset as struct
// sTypes (so that .otherStruct can work).
union {
struct {
WGPU_NONNULL WGPUBuffer buffer;
uint64_t offset;
uint64_t size;
} buffer;
WGPU_NONNULL WGPUSampler sampler;
WGPU_NONNULL WGPUTextureView textureView;
WGPU_NONNULL void * otherObject; // may be a WGPUEmscriptenExternalTexture
WGPU_NONNULL WGPUChainedStruct * otherStruct; // (not used yet)
};
};
In
WGPUBindGroupEntry
theoffset
andsize
fields are supposed to apply only if the binding is abuffer
, but that's not really clear. We could:bufferBindingOffset
andbufferBindingSize
WGPUBufferBinding
like in JSNot sure it's actually worth the effort to change.