glfw: Force init before using init dependent functions

Avokadoen commented

2021-11-04 12:19:19 +00:00

(Migrated from github.com)

glfws order dependent API design makes sense according to glfw implementation, but is quite silly from a glfw user perspective. I'm confident we can improve the API (hopefully without deviating too much from the C API)

My previous suggestion did not turn out well. Now, vulkan is a very big API, but one thing that I realized is that the design of the API makes it impossible to call functions in the wrong order by forcing you to instantiate an vkInstance that you then can call functions from. This is again used for later concepts in Vulkan. This concept can be used in the glfw API too!

If init expose all functions that require init through a returned struct, then we can make it impossible to end up with the init error scenario, and this pattern can be used for other similar order dependent function (if there are any)

const glfw_entry = @import("glfw");

pub fn main() !void {
    const glfw = try glfw_entry.init(); // use returned API struct
    defer glfw.terminate();

    // Create our window
    const window = try glfw.Window.create(640, 480, "Hello, mach-glfw!", null, null);
    defer window.destroy();

    // Wait for the user to close the window.
    while (!window.shouldClose()) {
        glfw.pollEvents();
    }
}

glfws order dependent API design makes sense according to glfw implementation, but is quite silly from a glfw user perspective. I'm confident we can improve the API (hopefully without deviating too much from the C API) My previous suggestion [did not turn out well](https://www.reddit.com/r/Zig/comments/qiqz5s/heres_someting_that_absolutely_blows_my_mind/hil9skv/). Now, vulkan is a very big API, but one thing that I realized is that the design of the API makes it impossible to call functions in the wrong order by forcing you to instantiate an ``vkInstance`` that you then can call functions from. This is again used for later concepts in Vulkan. This concept can be used in the glfw API too! If init expose all functions that require init through a returned struct, then we can make it impossible to end up with the init error scenario, and this pattern can be used for other similar order dependent function (if there are any) ```zig const glfw_entry = @import("glfw"); pub fn main() !void { const glfw = try glfw_entry.init(); // use returned API struct defer glfw.terminate(); // Create our window const window = try glfw.Window.create(640, 480, "Hello, mach-glfw!", null, null); defer window.destroy(); // Wait for the user to close the window. while (!window.shouldClose()) { glfw.pollEvents(); } } ```

👍 1

Avokadoen commented

2021-11-04 12:47:01 +00:00

(Migrated from github.com)

I also have similar issue in my own 2d rendering API: issue 41

I also have similar issue in my own 2d rendering API: [issue 41](https://github.com/Avokadoen/zig_vulkan/issues/41)

👍 1

emidoots commented

2021-11-08 04:13:43 +00:00

(Migrated from github.com)

If others have feedback here on whether or not they like/dislike this, I'd love to hear it. One thing I struggle with here is:

While this is a clear win, it's also a clear substantial deviation from the current GLFW API. Where we otherwise added methods, enums, etc. you'd have to pass those around regardless. But with this, you need to pass around the API struct too / can't rely on globals. This could make porting existing code harder.

For new APIs such as your 2D rendering API, though, this is a clear win obviously. I'm just not sure if the benefits (relatively small chance you forget to call glfw.init) outweight the costs (harder to port code). Not to say I am against this, just... unsure?

If others have feedback here on whether or not they like/dislike this, I'd love to hear it. One thing I struggle with here is: While this is a clear win, it's also a clear substantial deviation from the current GLFW API. Where we otherwise added methods, enums, etc. you'd have to pass those around regardless. But with this, you need to pass around the API struct too / can't rely on globals. This could make porting existing code harder. For new APIs such as your 2D rendering API, though, this is a clear win obviously. I'm just not sure if the benefits (relatively small chance you forget to call glfw.init) outweight the costs (harder to port code). Not to say I am against this, just... unsure?

👍 1

InKryption commented

2021-11-08 09:52:24 +00:00

(Migrated from github.com)

I personally would really like this change, but playing the devil's advocate here: I think it could be argued that something like this would be best made as a separate abstraction on top of the bindings, to remain as consistent as possible with canonical GLFW.

Avokadoen commented

2021-11-08 10:02:41 +00:00

(Migrated from github.com)

I agree this change would be a deviation from the original API. I am also not sure if the gains are worth it, so feel free to close the issue for now @slimsag 😅

emidoots commented

2021-11-15 23:30:48 +00:00

(Migrated from github.com)

(just clarifying, PR for this is welcome)

👍 1

InKryption commented

2021-11-20 22:45:33 +00:00

(Migrated from github.com)

I've tried my hand at making a draft of the proposed solution, but it generally always ended up being more cumbersome than what we currently have, being that you can't actually use an instance of a struct to access its public decls, leading to the need to have more C-like createWindow declarations within the returned struct, with a self parameter, to be able to make the guarantees this proposal aims to promise.

Though, an alternative occurred to me, working on #81: we could instead try and guarantee function call order via runtime checks when in debug mode, which are disabled when in any other mode. E.g.:

var debug_glfw_initialized = if (@import("builtin").mode == .Debug) false else @as(void, {});
pub inline fn init(hints: InitHints) Error!void {
    if (@import("builtin").mode == .Debug) debug_glfw_initialized = true;
    errdefer @import("builtin").mode == .Debug debug_glfw_initialized = false;
    ...
}

pub inline fn terminate() void {
    if (@import("builtin").mode == .Debug) std.debug.assert(debug_glfw_initialized);
    c.glfwTerminate();
}

Rough example, obviously could be greatly improved by modularizing/namespacing these things, such that they would be accessible from within all relevant parts of the library, but not outside, but this is the outline. Any thoughts, in favor, in opposition?

Edit: and to clarify, I really did try a slew of ideas before coming to this, such as having comptime type fields to limit the access of things like Window to the instance, which has its own issues and inconveniences.

I've tried my hand at making a draft of the proposed solution, but it generally always ended up being more cumbersome than what we currently have, being that you can't actually use an instance of a struct to access its public decls, leading to the need to have more C-like `createWindow` declarations within the returned struct, with a self parameter, to be able to make the guarantees this proposal aims to promise. Though, an alternative occurred to me, working on #81: we could instead try and guarantee function call order via runtime checks when in debug mode, which are disabled when in any other mode. E.g.: ```zig var debug_glfw_initialized = if (@import("builtin").mode == .Debug) false else @as(void, {}); pub inline fn init(hints: InitHints) Error!void { if (@import("builtin").mode == .Debug) debug_glfw_initialized = true; errdefer @import("builtin").mode == .Debug debug_glfw_initialized = false; ... } pub inline fn terminate() void { if (@import("builtin").mode == .Debug) std.debug.assert(debug_glfw_initialized); c.glfwTerminate(); } ``` Rough example, obviously could be greatly improved by modularizing/namespacing these things, such that they would be accessible from within all relevant parts of the library, but not outside, but this is the outline. Any thoughts, in favor, in opposition? Edit: and to clarify, I really did try a slew of ideas before coming to this, such as having `comptime` `type` fields to limit the access of things like `Window` to the instance, which has its own issues and inconveniences.

emidoots commented

2021-11-21 00:00:14 +00:00

(Migrated from github.com)

you can't actually use an instance of a struct to access its public decls, leading to the need to have more C-like createWindow declarations within the returned struct, with a self parameter, to be able to make the guarantees this proposal aims to promise.

Oh wow, I hadn't thought of that, I guess we would need stack capturing ala https://github.com/ziglang/zig/issues/6965 in order to make that a nice experience. Very interesting.

> you can't actually use an instance of a struct to access its public decls, leading to the need to have more C-like createWindow declarations within the returned struct, with a self parameter, to be able to make the guarantees this proposal aims to promise. Oh wow, I hadn't thought of that, I guess we would need stack capturing ala https://github.com/ziglang/zig/issues/6965 in order to make that a nice experience. Very interesting.

emidoots commented

2021-11-21 00:02:59 +00:00

(Migrated from github.com)

I like your proposal a lot. At first, I wasn't sure what benefit it would give us over what GLFW already does internally (after all, it's functions are documented as returning an error if GLFW is not initialized). I see two major benefits:

You get an assertion at runtime, rather than an error (which e.g. maybe someone logs by accident.)
We could eliminate that error altogether and that's one less thing for people to need to care about / handle.

Sounds like the right path forward to me!

I like your proposal a lot. At first, I wasn't sure what benefit it would give us over what GLFW already does internally (after all, it's functions are documented as returning an error if GLFW is not initialized). I see two major benefits: 1. You get an assertion at runtime, rather than an error (which e.g. maybe someone logs by accident.) 2. We could eliminate [that error altogether](https://github.com/hexops/mach/blob/e024ced5412f215927eba74375be24bc24ac72e9/glfw/src/errors.zig#L7-L11) and that's one less thing for people to need to care about / handle. Sounds like the right path forward to me!

👍 1

emidoots commented

2021-11-21 00:03:35 +00:00

(Migrated from github.com)

I also like that this reduces my original complaint here: "This could make porting existing code harder."

InKryption commented

2021-11-21 00:09:28 +00:00

(Migrated from github.com)

Righto, if that's the new direction, I'll look into trying to come up with a more elaborated draft proposal in the coming days, 'less someone else makes one first.

❤️ 1 🚀 1

Rows
Columns

glfw: Force init before using init dependent functions #61