API changes to make certain things feel more natural in Zig

[bronter]

Related to the discussion in #23:

• use slices instead of separate count and pointer
• return error instead of null when null actually indicates a failure
• return the value instead of putting it into a pointer in some functions like surface.getCurrentTexture
• use a zig bool instead of WGPUBool
• pass the descriptor structs, and the structs they may contain, directly as value instead of as pointers
• u8 slices instead of StringViews
• Return error unions rather than wgpu.Status
• Get rid of Procs structs since they are unused and difficult to maintain
• Use packed structs to represent WGPUFlags

[bronter]

Copying over from #23 :

I'm still thinking about how I want to wrap the callbacks so that we can apply the same changes there that we do everywhere else. We might be able to leverage the fact that we can pass in types so that the callback doesn't have to do all that weird @ptrCast(@alignCast( stuff on the userdata parameters.

It might also be worth revisiting methods like requestAdapterSync; I've always kind of cringed at the struct they return and would rather return an error union. I wonder if it would be better to just log the message using std.log.err() if we get anything other than RequestAdapterStatus.success and then just coalesce the status into an error union and return either the error or the adapter pointer from requestAdapterSync.

[TotoShampoin]

If I want to help convert all the null returns to error unions, what would be your guideline for what the error types are?

[bronter]

I'm open to suggestion, but the way I've been doing it so far is to create a public error union named after the opaque struct the methods belong to, so for errors coming from Instance methods I did pub const InstanceError = error {...errors listed here...};.

I also tried to name the specific error similar to the method that generated it, so if the method has create in the name I do something like FailedToCreateInstance, if it has get in the name I do something like FailedToGetCapabilities.

With the method that used std.mem.Allocator I just merged std.mem.Allocator.Error into the error union using ||, since subsets of errors can be coalesced into larger sets. That way I can return InstanceError as the error type on all of the Instance methods.

[bronter]

I just want it to be be somewhat obvious which method generated an error, so that if I have an application that has a fn render(ctx: *RenderContext) !void {}, and context.render() returns an error I know which method call within render() generated the error before I even look at the stack trace.

[TotoShampoin]

Taking a look at what you did in Instance, as I try to bring similar changes for Device, I notice that you used []u8 (variable) slices for strings. This causes issues, as Zig treats strings (with quotes) as []const u8, thus making them unstorable in []u8.

I checked with this dummy code:

const SomeStruct = struct {
    a: []u8,
};

pub fn main() !void {
    const d = SomeStruct{
        .a = "aca",
    };

    @import("std").debug.print("{s}\n", .{d.a});
}

// outputs:
\\\ slice.zig:7:10: error: expected type '[]u8', found '*const [3:0]u8'
\\\         .a = "aca",
\\\         ~^~~~~~~~~
\\\ slice.zig:7:10: note: cast discards const qualifier

[bronter]

Yeah, I should've run tests on that, will update it to use []const u8.

[bronter]

I've figured out how to do callbacks like this:

// In adapter.zig:
pub fn MakeRequestAdapterCallbackTrampoline(
    comptime UserDataPointerType: type,
) type {
    const CallbackType = *const fn(RequestAdapterStatus, ?*Adapter, ?[]const u8, UserDataPointerType) void;

    // The compiler doesn't seem to like it if I don't have the function inside of a struct
    return struct {
        pub fn callback(status: RequestAdapterStatus, adapter: ?*Adapter, message: StringView, userdata1: ?*anyopaque, userdata2: ?*anyopaque) callconv(.C) void {
            const wrapped_callback: CallbackType = @ptrCast(userdata2);
            const userdata: UserDataPointerType = @ptrCast(@alignCast(userdata1));
            wrapped_callback(status, adapter, message.toSlice(), userdata);
        }
    };
}

//...

// In instance.zig, inside of Instance
// requestAdapter2 is just a temporary name so that I don't break existing tests
pub fn requestAdapter2(
    self: *Instance,
    mode: ?CallbackMode,
    userdata: anytype,
    callback: *const fn(RequestAdapterStatus, ?*Adapter, ?[]const u8, @TypeOf(userdata)) void,
    options: ?RequestAdapterOptions,
) Future {
    if (@typeInfo(@TypeOf(userdata)) != .pointer) {
        @compileError("userdata should be a pointer type");
    }
    const Trampoline = MakeRequestAdapterCallbackTrampoline(@TypeOf(userdata));
    const callback_info = RequestAdapterCallbackInfo {
        .mode = mode orelse CallbackMode.allow_process_events,
        .callback = Trampoline.callback,
        .userdata1 = @ptrCast(userdata),
        .userdata2 = @constCast(@ptrCast(callback)),
    };
    if (options) |o| {
        return wgpuInstanceRequestAdapter(self, &o, callback_info);
    } else {
        return wgpuInstanceRequestAdapter(self, null, callback_info);
    }
}

//...later on when we actually use it

fn adapterRequestHandler(status: RequestAdapterStatus, adapter: ?*Adapter, message: ?[]const u8, userdata: *?*Adapter) void {
    _ = message;
    if (status == .success) {
        userdata.* = adapter;
    }
}
test "requestAdapter with custom userdata type" {
    const instance = try Instance.create(null);
    var maybe_adapter: ?*Adapter = null;

    _ = instance.requestAdapter2(null, &maybe_adapter, adapterRequestHandler, null);

    try std.testing.expect(maybe_adapter != null);
}

It comes at a price though: in order to avoid having to allocate memory for a second function pointer, I had to dedicate userdata2 to pass our custom callback into the trampoline callback (the one that wgpu-native understands). This means that passing in two things through userdata will require putting them into a struct first. Though that's pretty much how it worked prior to wgpu-native v24.x.x.x anyway, so 🤷🏻‍♂️. I guess the anytype is a bit confusing too, but so is anyopaque.

I have a bit of work to do to for requestAdapterSync and the tests/examples before I push it up, but even with the loss of one of the userdata parameters I think doing it this way will be less frustrating than the previous callback format.

[TotoShampoin]

I believe that the 2nd userdata is meant to be used by engines that use webgpu anyway, so yeah

Another thing you could do, if you really want to keep 2 userdatas (as this is still a webgpu interface), is put them in a tuple of opaque pointers, and pass that as parameter 1

[TotoShampoin]

Side note, *anyopaque isn't confusing if you think that it's equivalent to C's void*, which means "pointer to an unknown type"

[TotoShampoin]

I just noticed that I forgot one WGPUBool in the Adapter PR (in RequestAdapterOptions). Not a big deal, it is a draft after all.

But it makes me think, considering more and more extern structs are getting a Zig interface and a layer of conversion in the function calls, maybe it would be a good idea to make that conversion layer into struct methods? Something like this:

pub const DeviceDescriptor = struct {
    ...

    pub fn toWgpu(self: DeviceDescriptor) WGPUDeviceDescriptor { ... }
};

[bronter]

Yeah, that would certainly cut down on the clutter in the opaque struct methods.

[bronter]

So I started working on bind_group.zig, and ran into a bit of an issue with BindGroupEntry/BindGroupEntryExtras. If I do what I did before and make a wrapper for BindGroupEntry that adds an optional native_extras field, then in BindGroupDescriptor.toWGPU() I'd have to allocate a new list and call toWGPU() on each BindGroupEntry, which means we'd have to pass in an allocator when we create the bind group.

Not necessarily a problem, but I wonder if having to pass in an allocator for things that previously didn't require it would be frustrating to users? Even if I kept the chained struct format, we'd probably run into a similar issue with lists somewhere else, so now is probably a good time to put some thought into it.

I've thought about writing a wrapper around Instance that holds an allocator, and writing wrapper structs around Adapter, Device, etc. that hold a pointer to the Instance wrapper. I can see some pros and some cons to that though:

Pros:

• No need to explicitly pass in an allocator to any of our wrapper methods
• We could create a WGPUAllocator, that wraps around the user-provided allocator (sort of like how std.heap.ArenaAllocator does) and is aware of the allocations wgpu_native does for us, so it knows when to call freeMembers() and might eliminate the need to do allocator.dupe() anywhere.
• Adapter.requestDeviceSync() would no longer need the Instance to be passed in explicitly.

Cons:

• Some people probably prefer that any function that does allocation explicitly has an allocator as one of its parameters, so that it's easy to see which functions do allocations and which don't.
• Might be some issues with thread safety that users might accidentally run into if they aren't aware that a method is going to call into the allocator. It would probably be easier to spot thread safety issues if the allocator was passed into the methods explicitly. std.heap.ThreadSafeAllocator might work for most cases though, unless someone wanted to have hundreds of threads with their own pipelines.

[TotoShampoin]

Another con I see to doing this is we stray further away from the native webgpu api, which, if I wanted that, I would have just gone with the Zig-Gamedev implementation.

Using .toWGPU was my initial idea to do some separation of concerns, but if we keep in mind that the motive here is to convert idiomatic zig structs to what WebGPU expects, then I have a few ideas:

We can create a slice using just the pointer and the length and inversely, as long as the original variables are not deallocated (const slice = your_buffer[0..length]). So one thing that could be done is simply to not guarantee that the values will be preserved.

Maybe there would be a .toAllocatedWGPU and a .toUnsafeWGPU, where the latter does exactly that. That way, there would be an implementation for both preferences (which seems to be in line with what Zig's std tends to do), and the interface could just use .toUnsafeWGPU internally.
Like, I don't think that WebGPU will face dangling pointers after the bind group will be created as I believe it does not need the descriptor once that's done? I could be wrong though. Or maybe I misunderstood where you're having allocation issues.

[bronter]

I'd agree with not straying too far from the native webgpu api.

The issue is with slices of wrapper structs. If I have an array of BindGroupEntry, and BindGroupEntry has a different size or memory layout than WGPUBindGroupEntry, then I can't trivially convert a slice of BindGroupEntry to a slice of WGPUBindGroupEntry; I'd have to allocate memory and construct a new array of WGPUBindGroupEntry.

[bronter]

So as far as I can tell, we either have to avoid slices of wrapper structs with different sizes than the native structs, or pass in an allocator. Or if the native structs are the same size or smaller, we might be able to re-use the buffer, but only if we allow it to be mutable.

[bronter]

Also you're right that wgpu-native doesn't need the descriptor after it is passed in; it makes a copy to keep internally.

[TotoShampoin]

Oohh, now I understand the issue...

Well, not only would you have to avoid different sizes than the native structs, but you'd also have to avoid different layouts. As in, if a zig slice stores the pointer first but wgpu-native stores the length first, we're cooked.

Another issue is that this is extra work at runtime for allocating and copying or converting the same data...

[bronter]

Yeah, pretty much. Luckily most of the slices wgpu_native takes in are slices of enums or opaque pointers.

[TotoShampoin]

Yes, but bind groups and layouts are what users will use the most, as they are required for rendering.

Ideally, if we want this to feel like it's the same but in Zig, we would hide any allocation done for conversion. Maybe the library could have a fixed buffer allocator ready inside.
But that isn't the Zig way, is it... Not to mention this is still manual conversion work that will slow down execution time, which is not ideal if this is supposed to be a port from the JavaScript API.

It's not ideal, but it'd be a way to avoid forcing the user to pass an allocator. Generally, I never liked the idea of passing an allocator to a function that is only ever gonna use it internally and never return any heap allocated data.

[bronter]

Another option would be to use a pattern similar to what we have for shader module descriptors in shader.zig. Instead of making BindGroupEntry into a wrapper struct with a toWGPU() method, we could keep BindGroupEntry as the native struct, but provide a function that can construct it taking a wrapper struct (like WrappedBindGroupEntry), for example:

fn bindGroupEntry(entry: WrappedBindGroupEntry) BindGroupEntry {
  //...
}

Then BindGroupDescriptor can take a slice of BindGroupEntry, and it stills looks fairly normal to construct, like:

const bind_group = device.createBindGroup(&BindGroupDescriptor {
  //...other stuff in the descriptor
  entries: &[_]BindGroupEntry {
    bindGroupEntry(.{
      //...
    }),
    bindGroupEntry(.{
      //...
    }),
  },
});

[TotoShampoin]

I guess that can work. It does fall in line with the pattern that you'd used everywhere. Bonus point if it is trivial enough that it can be inlined and optimized out into constructing the original struct

[bronter]

I could make the function inline, though if the structs are all on the stack I'd be surprised if it wasn't optimized out anyway. I'll see if there's a way to test that.

[TotoShampoin]

Well, it's something I found out by using a debugger, finding out that a lot of unnecessary jumps (especially switches) were dissipated even in debug mode

I don't remember the intricate details, and I have yet to figure out how to debug a zig executable in vscode on windows

[TotoShampoin]

I'm just gonna quote @filipcro's suggestion from the original issue

If you are doing the custom Zig like API, I would suggest using packed struct into the integer instead of the flags. For example:

pub const BufferUsage = packed struct(WGPUFlags) {
    map_read: bool = false,
    map_write: bool = false,
    copy_src: bool = false,
    copy_dst: bool = false,
    index: bool = false,
    vertex: bool = false,
    uniform: bool = false,
    storage: bool = false,
    indirect: bool = false,
    query_resolve: bool = false,
    _: u54 = 0,
};

  // usage example
  const descriptor = wgpu.BufferDescriptor{
      .usage = .{  .copy_dst = true, .vertex = true }, 
      .size = 1024,
  };

I will mention, in case you (or anyone) didn't know: If you do a packed struct but the original api's flags don't line up, you will need to count the bits and add useless paddings to the struct. Otherwise, you won't be able to properly bit cast it

[TotoShampoin]

Fortunately, it appears that all flag structs' bits are adjacent, meaning they only require one padding at the end

(I could be wrong)

[bronter]

Yeah, the padding thing is a bit annoying, though I think it makes sense since Zig tries not to have any hidden behavior and you'd want to be able to see what the rest of the bits are explicitly set to.

I think you're right about all the flag bits being adjacent. Some flags have been added or removed over time, but when that happens the rest of the flags are changed so that there aren't any gaps. The enums sometimes have gaps, but for those it doesn't really matter.

[bronter]

I added some functions to construct the DeviceLostCallbackInfo and UncapuredErrorCallbackInfo structs used in DeviceDescriptor, though I haven't tested them yet; working on that.

If all goes well though, I wonder if it might be worthwhile to try to separate out that logic for Instance.requestAdapter() and Adapter.requestDevice() as well. They could take in RequestAdapterCallbackInfo and RequestDeviceCallbackInfo structs as they did previously, so the constructor function is there for those who want it, and those who don't can continue to use the CallbackInfo structs as they did before.