Zig UI demo #154

ilidemi · 2023-10-03T08:46:31Z

ilidemi commented

2023-10-03 08:46:31 +00:00

Tried to make it as zig as possible but my knowledge is far from perfect. Nitpicks are welcome.

Notes:

Added Arena.pushStr(). Not entirely sure whether it belongs there or should be null-terminated, suggestions are welcome
Flag enums ported as packed structs, except for the animation mask, which needs array conversions inside the bitfield. It should be doable with setter methods but we don't have usage examples so leaving as is.
UiBox, TextState and ClipboardState have Str8 and Str32 inside but my understanding is they're not expected to be accessed directly, at least often.
UiSelector doesn't expose the linked list element in zig-facing API but that may be confusing in terms of how they relate to patterns. Mentioned that in a doc comment.
Added _str8() variants to all widgets. It's probably desirable that they aren't exposed in the C headers but all bindings will need them.

Zig bindings thoughts

Str8List is exposed to the user and should probably become something []const u8 or []u8-based.
Arena APIs can return AllocError, and UI internally uses a frameArena and a pool which could overflow too but doesn't return errors. C layer doesn't propagate those, and calling widgets would become even more convoluted.
oc_on_raw_event now takes C-style event which has a conversion method to a Zig-style one. If we provide language-specific event handlers that could be nicer (along with camelCase naming) but not sure if it's possible to only wrap the ones that are defined in the user code.

Tried to make it as zig as possible but my knowledge is far from perfect. Nitpicks are welcome. Notes: * Added Arena.pushStr(). Not entirely sure whether it belongs there or should be null-terminated, suggestions are welcome * Flag enums ported as packed structs, except for the animation mask, which needs array conversions inside the bitfield. It should be doable with setter methods but we don't have usage examples so leaving as is. * UiBox, TextState and ClipboardState have Str8 and Str32 inside but my understanding is they're not expected to be accessed directly, at least often. * UiSelector doesn't expose the linked list element in zig-facing API but that may be confusing in terms of how they relate to patterns. Mentioned that in a doc comment. * Added _str8() variants to all widgets. It's probably desirable that they aren't exposed in the C headers but all bindings will need them. Zig bindings thoughts * Str8List is exposed to the user and should probably become something `[]const u8` or `[]u8`-based. * Arena APIs can return AllocError, and UI internally uses a frameArena and a pool which could overflow too but doesn't return errors. C layer doesn't propagate those, and calling widgets would become even more convoluted. * `oc_on_raw_event` now takes C-style event which has a conversion method to a Zig-style one. If we provide language-specific event handlers that could be nicer (along with camelCase naming) but not sure if it's possible to only wrap the ones that are defined in the user code.

ilidemi force-pushed zig-ui from 14bd0880e6 to 47cba86c03

2023-10-03 08:59:39 +00:00

Compare

ilidemi added 1 commit 2023-10-03 09:00:33 +00:00

fd308f9a3a Zig UI demo

rdunnington reviewed 2023-10-03 12:47:39 +00:00

src/orca.zig Outdated

													
				@ -1443,1 +1486,3 @@

				    Count = 349,

				};

				pub const key_code_count: usize = 349;

rdunnington commented

2023-10-03 12:47:39 +00:00

Might want this to be some sort of comptime thing where it looks up the last field of the enum and uses that value instead of hardcoding it. Something like this:

pub const key_code_count: usize = comptime blk: {
    const enum_fields = @typeInfo(T).Enum.fields;
    break :blk enum_fields[enum_fields.len - 1].value + 1;
};

Might want this to be some sort of comptime thing where it looks up the last field of the enum and uses that value instead of hardcoding it. Something like this: ```zig pub const key_code_count: usize = comptime blk: { const enum_fields = @typeInfo(T).Enum.fields; break :blk enum_fields[enum_fields.len - 1].value + 1; }; ```

ilidemi marked this conversation as resolved

rdunnington reviewed 2023-10-03 12:48:21 +00:00

src/orca.zig Outdated

													
				@ -1451,10 +1505,111 @@ pub const MouseButton = enum(c_uint) {

				    Ext2 = 0x04,

				};

				pub const mouse_button_count: usize = 5;

rdunnington commented

2023-10-03 12:48:21 +00:00

Same as key_code_count above

ilidemi marked this conversation as resolved

rdunnington commented

2023-10-03 13:12:25 +00:00

Thanks a ton, this is obviously a lot of work! Still reading through the changes, I'll try to finish it up sometime tonight.

re your notes:

I was just thinking the same thing about Str8List and Str8ListElt. I should probably rewrite them to use zig native slices instead of the wrapper struct. Since the zig slice layout now matches oc_str8/16/32, it should be transparent. This will also pair nicely with a change I've been thinking about making to the APIs where any function that takes a oc_str gets a zig wrapper function that takes a slice and converts it internally to make them more ergonomic.
I think Str8.pushBuffer() is trying to do what Arena.pushStr() does, although it goes the extra mile of wrapping it in a Str8. I think duplicating the functionality is fine, and null-terminating it is probably good as well since I suspect some of the C APIs used internally expect null-terminated strings. fwiw zig string literals are null-terminated as well.
re: native zig event handlers. Was thinking about this a bit but haven't tried it yet. We might be able to leverage comptime and weak linking to hook orca-native event handlers in orca.zig, then use them as trampolines to zig-native event handlers defined in userland. I haven't tried this yet because I hadn't seen a compelling reason yet, but looking at the @export documentation I suspect it's possible: https://ziglang.org/documentation/0.11.0/#export
I don't think I understand this part of your note about AllocError: "calling widgets would become even more convoluted". I was originally using nulls to signal alloc failure like Orca does but then realized zig's error system would be a more natural fit. Do you think the AllocError approach doesn't fit well with the UI system? Or should we go back to using nulls?

Thanks a ton, this is obviously a lot of work! Still reading through the changes, I'll try to finish it up sometime tonight. re your notes: * I was just thinking the same thing about Str8List and Str8ListElt. I should probably rewrite them to use zig native slices instead of the wrapper struct. Since the zig slice layout now matches `oc_str8/16/32`, it should be transparent. This will also pair nicely with a change I've been thinking about making to the APIs where any function that takes a `oc_str` gets a zig wrapper function that takes a slice and converts it internally to make them more ergonomic. * I think `Str8.pushBuffer()` is trying to do what `Arena.pushStr()` does, although it goes the extra mile of wrapping it in a `Str8`. I think duplicating the functionality is fine, and null-terminating it is probably good as well since I suspect some of the C APIs used internally expect null-terminated strings. fwiw zig string literals are null-terminated as well. * re: native zig event handlers. Was thinking about this a bit but haven't tried it yet. We might be able to leverage comptime and weak linking to hook orca-native event handlers in `orca.zig`, then use them as trampolines to zig-native event handlers defined in userland. I haven't tried this yet because I hadn't seen a compelling reason yet, but looking at the `@export` documentation I suspect it's possible: https://ziglang.org/documentation/0.11.0/#export * I don't think I understand this part of your note about `AllocError`: "calling widgets would become even more convoluted". I was originally using nulls to signal alloc failure like Orca does but then realized zig's error system would be a more natural fit. Do you think the `AllocError` approach doesn't fit well with the UI system? Or should we go back to using nulls?

rdunnington reviewed 2023-10-04 02:19:27 +00:00

samples/zig-ui/src/main.zig

													
				@ -0,0 +166,4 @@

				fn widgets(arena: *oc.Arena) void {

				    columnBegin("Widgets", 1.0 / 3.0);

				    defer columnEnd();

rdunnington commented

2023-10-04 02:19:27 +00:00

What do you think about putting the ui and column prefixed functions into a ui struct to namespace them and make it a bit clearer they are intended to be used together?

What do you think about putting the ui and column prefixed functions into a `ui` struct to namespace them and make it a bit clearer they are intended to be used together?

ilidemi commented

2023-10-04 03:23:43 +00:00

Good suggestion!

👍 1

ilidemi marked this conversation as resolved

rdunnington reviewed 2023-10-04 02:23:24 +00:00

src/orca.zig Outdated

													
				@ -1695,1 +1852,4 @@

				    pub fn array(rect: *Rect) *[4]f32 {

				        return @ptrCast(rect);

				    }

rdunnington commented

2023-10-04 02:23:24 +00:00

Nice, I like these changes. Makes it a lot easier to work with the base rect but keeps the other "views" of the data around.

👍 1

ilidemi marked this conversation as resolved

ilidemi added 1 commit 2023-10-04 03:22:53 +00:00

30d45cd7fd Comptime enum counts, namespace UI, clarify names/labels

ilidemi commented

2023-10-04 03:35:38 +00:00

I don't think I understand this part of your note about AllocError: "calling widgets would become even more convoluted". I was originally using nulls to signal alloc failure like Orca does but then realized zig's error system would be a more natural fit. Do you think the AllocError approach doesn't fit well with the UI system? Or should we go back to using nulls?

This one is on UI rather than on the bindings. AllocError is great, it's just the UI layer operates under assumption that 1MB of memory is enough and doesn't handle/propagate errors, even in C. With error propagation, box making would become even more noisy (_ = try ui.boxBegin("content", .{});) and functions like ui.panelEnd() would be un-deferrable because try is not allowed there. I guess we'd need to document that UI expects memory to be available and add asserts where it is allocated.

> I don't think I understand this part of your note about `AllocError`: "calling widgets would become even more convoluted". I was originally using nulls to signal alloc failure like Orca does but then realized zig's error system would be a more natural fit. Do you think the `AllocError` approach doesn't fit well with the UI system? Or should we go back to using nulls? This one is on UI rather than on the bindings. `AllocError` is great, it's just the UI layer operates under assumption that 1MB of memory is enough and doesn't handle/propagate errors, even in C. With error propagation, box making would become even more noisy (`_ = try ui.boxBegin("content", .{});`) and functions like `ui.panelEnd()` would be un-deferrable because `try` is not allowed there. I guess we'd need to document that UI expects memory to be available and add asserts where it is allocated.

👍 1

ilidemi referenced this pull request

2023-10-04 03:42:46 +00:00

Handle allocation errors in UI layer #155

MartinFouilleul commented

2023-10-04 19:12:24 +00:00

This one is on UI rather than on the bindings. AllocError is great, it's just the UI layer operates under assumption that 1MB of memory is enough and doesn't handle/propagate errors, even in C.

This isn't quite true. Arenas grow as long as we can give them more memory. So for wasm32 allocations will fail somewhere a bit under 4GB, with an out of memory assertion managed by the runtime. Virtual memory backed arenas on wasm64 could grow much larger. But the main idea is that if you fill your whole address space, there's not much we can do except displaying an OOM message box and closing the program.

> This one is on UI rather than on the bindings. `AllocError` is great, it's just the UI layer operates under assumption that 1MB of memory is enough and doesn't handle/propagate errors, even in C. This isn't quite true. Arenas grow as long as we can give them more memory. So for wasm32 allocations will fail somewhere a bit under 4GB, with an out of memory assertion managed by the runtime. Virtual memory backed arenas on wasm64 could grow much larger. But the main idea is that if you fill your whole address space, there's not much we can do except displaying an OOM message box and closing the program.

MartinFouilleul referenced this pull request

2023-10-04 19:17:33 +00:00

Handle allocation errors in UI layer #155

ilidemi commented

2023-10-04 20:00:22 +00:00

Sorry I wasn't clear, yes the UI will happily take up as much address space as needed and available. I'm pointing at a discrepancy between how arenas explicitly return an error to the user and UI doesn't check the allocation results at all, for example in the chain from oc_ui_box_make_str8 to oc_arena_chunk_alloc. If we expect the runtime to abort on allocation errors, the wasm side doesn't need to worry about AllocErrors at all (at the expense of being less systems-y). If I understand the code correctly, the runtime will abort when running out of wasm address space and (at least on win32) will silently drop the error if VirtualAlloc(MEM_COMMIT) fails, so actually there isn't a case where the wasm side has to handle a null.

Sorry I wasn't clear, yes the UI will happily take up as much address space as needed and available. I'm pointing at a discrepancy between how arenas explicitly return an error to the user and UI doesn't check the allocation results at all, for example in the chain from `oc_ui_box_make_str8` to `oc_arena_chunk_alloc`. If we expect the runtime to abort on allocation errors, the wasm side doesn't need to worry about `AllocError`s at all (at the expense of being less systems-y). If I understand the code correctly, the runtime will abort when running out of wasm address space and (at least on win32) will silently drop the error if `VirtualAlloc(MEM_COMMIT)` fails, so actually there isn't a case where the wasm side has to handle a null.

rdunnington reviewed 2023-10-05 02:15:46 +00:00

src/orca.zig

													
				@ -1848,0 +3269,4 @@

				        text: []u8,

				    };

				    pub fn textBox(name: []const u8, arena: Arena, text: []const u8) TextBoxResult {

rdunnington commented

2023-10-05 02:15:46 +00:00

I think this Arena needs to be a pointer. Same on 3195

I think this `Arena` needs to be a pointer. Same on 3195

ilidemi commented

2023-10-05 02:40:40 +00:00

Good catch!

ilidemi marked this conversation as resolved

rdunnington reviewed 2023-10-05 02:20:50 +00:00

src/orca.zig

													
				@ -1848,0 +2797,4 @@

				        theme: *Theme,

				        extern fn oc_ui_init(context: *Context) void;

				        pub const init = oc_ui_init;

rdunnington commented

2023-10-05 02:20:50 +00:00

Zig style usually has the init function take either no arguments or some config options. In this case, maybe we can provide a wrapper function that returns a new Context by value after calling oc_ui_init() on it, so that all the user has to do is:

var ui_ctx = oc.ui.Context.init();

Zig style usually has the init function take either no arguments or some config options. In this case, maybe we can provide a wrapper function that returns a new `Context` by value after calling `oc_ui_init()` on it, so that all the user has to do is: ```zig var ui_ctx = oc.ui.Context.init(); ```

ilidemi commented

2023-10-05 02:41:56 +00:00

Here the address of ui_ctx is important. Moved init outside of the Context.

👍 1

ilidemi marked this conversation as resolved

rdunnington commented

2023-10-05 02:23:53 +00:00

Re: the alloc stuff. It sounds like in that case the Zig bindings should just always assume valid memory will be returned from Arena allocs, since if Orca OOMs, it will be "handled" on the native side via a shutdown. So I can just remove AllocError.

Re: the alloc stuff. It sounds like in that case the Zig bindings should just always assume valid memory will be returned from Arena allocs, since if Orca OOMs, it will be "handled" on the native side via a shutdown. So I can just remove `AllocError`.

ilidemi added 1 commit 2023-10-05 02:40:13 +00:00

f1727dbc8d Text box arena, ui init func

rdunnington merged commit f1727dbc8d into zig_ffi

2023-10-05 04:51:18 +00:00

Sign in to join this conversation.

No reviewers

No Label

macOS

windows

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hmn/orca#154