floooh / sokol

minimal cross-platform standalone C headers
https://floooh.github.io/sokol-html5
zlib License
6.53k stars 467 forks source link

Changes to support generating the sokol-fetch bindings in zig #1048

Closed trace-andreason closed 1 month ago

trace-andreason commented 1 month ago

Related to this issue: https://github.com/floooh/sokol-zig/issues/63. Not ready to be merged, just thought it might be helpful for that discussion

floooh commented 1 month ago

...that fetch callback should actually be handled here:

https://github.com/floooh/sokol/blob/f70cc07f0fcd763ab6ff0171a6b8fe1dca3ed367/bindgen/gen_zig.py#L339-L340

...I'm currently investigating why it isn't...

floooh commented 1 month ago

Aha, because it's a typedef in the sokol_fetch.h header, and that's not detected in the bindings generation script.

If I remove the sfetch_callback_t typedef from sokol_fetch.h and replace the callback declaration in sfetch_request_t to this:

typedef struct sfetch_request_t {
    ...
    void (*callback) (const sfetch_response_t*);
    ...
} sfetch_request_t;

...it works, and generates the following Zig struct:

pub const Request = extern struct {
    channel: u32 = 0,
    path: [*c]const u8 = null,
    callback: ?*const fn ([*c]const Response) callconv(.C) void = null,
    chunk_size: u32 = 0,
    buffer: Range = .{},
    user_data: Range = .{},
};

...can you integrate this sokol_fetch.h change in your PR and remove your "manual exception hack"?

trace-andreason commented 1 month ago

yep, sounds good! Are you cool with these overloaded names?

    'sfetch_continue':                      'fetch_continue', # 'fetch' is reserved in Zig
    'sfetch_desc':                          'description'     # 'desc' shadowed by earlier definiton

fetch_continue might be weird because it'll probably end up being fetch.fetch_continue() in zig...

trace-andreason commented 1 month ago

made those changes

floooh commented 1 month ago

...for the sfetch_desc override I would actually prefer sfetch_get_desc which would then end up as fetch.getDesc() on the Zig side.

For continue maybe resume? Since it's "resuming from paused state".

...tbh, sfetch_resume() would also be a good change in the C header.

floooh commented 1 month ago

...one other change I'd like is to restrict sokol_fetch.h to the Zig bindings for now, e.g. remove the sokol_fetch item from the tasks array in gen_all.py, and instead do this down in the gen_zig block:

zig_tasks = [
    *tasks,
    [ '../sokol_fetch.h', 'sfetch_', [] ],
]
for task in zig_tasks:
   ...

...should have recommended this from the start but didn't think of the other language bindings...

trace-andreason commented 1 month ago

turns out resume is also reserved

trace-andreason commented 1 month ago

So I'll give you my 2 cents on naming here, coming from a beginner and copying examples written in C to zig. If we made continue be continue_fetching or something that starts with continue, you will at least get some editor help when just typing in the name of the c function.

trace-andreason commented 1 month ago

I went ahead and named it continue_fetching, but open to any other suggestion. All other comments are incorporated.

floooh commented 1 month ago

Yeah continue_fetching is fine :)

trace-andreason commented 1 month ago

bindings build step is going to fail until sokol_fetch.c file is added to the repo

floooh commented 1 month ago

Yep, I think I can take over from here. I'll also need to do a quick check if the dropped typedef breaks anything (I don't think so, but technically it's a breaking change). I'll try to look into it later today.

floooh commented 1 month ago

Zig master might have had a breaking change in LazyPath. I'm looking into it.

PS: yep, I'll check what's needed to fix sokol-zig (and park the current zig-0.12.0 compatible version in a branch)

floooh commented 1 month ago

Ok, took a bit longer then I had hoped, but the sokol-zig build.zig is now fixed for the current zig head version.

PS: the new D bindings suffer from the same problem because that also uses Zig as build system, but let's ignore that for now ;)

floooh commented 1 month ago

I'll go ahead and merge this PR.

The next step would then be in sokol-zig again to add the C source file to build.zig here:

https://github.com/floooh/sokol-zig/blob/7d569775df73d1e869a1df12dfc647e56c7cb77a/build.zig#L265

...and the fetch.zig module here:

https://github.com/floooh/sokol-zig/blob/7d569775df73d1e869a1df12dfc647e56c7cb77a/src/sokol/sokol.zig#L9

Ideally we would also have a new sample, but I can do that at a later time.

floooh commented 1 month ago

...also added a little blurb to the CHANGELOG about the theoretically breaking change in sokol_fetch.h

trace-andreason commented 1 month ago

next steps for sokol-zig: https://github.com/floooh/sokol-zig/pull/67/files