Representation of GPUBindGroupEntry

[kainino0x]

In WGPUBindGroupEntry the offset and size fields are supposed to apply only if the binding is a buffer, but that's not really clear. We could:

• Leave as is
• Rename to bufferBindingOffset and bufferBindingSize
• Collect the three fields together into a WGPUBufferBinding like in JS

Not sure it's actually worth the effort to change.

[eliemichel]

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.

[kainino0x]

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.

[kainino0x]

Tentatively closing as I'd like to go with no change, but leaving the label to discuss if we want to reopen.

[kainino0x]

Nov 21 meeting:

• KN: Probably not really worth changing. I already added docs to clarify.
• CF: Should really be a tagged union.
• [Matches better with JS which actually uses a union, unlike WGPUBindGroupLayoutEntry.]
• KN: Concerned about extensibility (pattern for extending is messy). Maybe use SType?
• Investigate more

[kainino0x]

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)
    };
};

[kainino0x]

Dec 7 meeting:

• Same conclusion as Representation of GPUBindGroupLayoutEntry #438, it's not worth changing to a tagged union.