Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional log/assert hooks implemented in Zig? #33

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelbartnett
Copy link
Contributor

I've been hanging onto these local changes for a while and figured I should offer them forward.

Setting the Sokol log macro to use a log function implemented in Zig means that if you happen to use std.log.info your formatting will look all nice and consistent.

Even more helpful is implementing the assert hook in Zig, since then Sokol validation errors will hit the Zig panic handler and show you a nice stacktrace even if you aren't attached in VS.

I saw other PRs/issues talking about adding a config/options struct for buildSokol which I imagine would interfere, so this is more of a suggestion/feature-request PR 🙂

@michaelbartnett michaelbartnett marked this pull request as draft January 16, 2023 15:27
@michaelbartnett michaelbartnett force-pushed the zig-log-and-assert-hooks branch from 63e050a to 65b4986 Compare January 16, 2023 20:34
Also introduced a SokolBuildOptions struct to contain backendselection
and flags for the Zig-side log and assert hooks (these are distinct)
@michaelbartnett michaelbartnett force-pushed the zig-log-and-assert-hooks branch from 65b4986 to 2053343 Compare January 16, 2023 20:52
@floooh
Copy link
Owner

floooh commented Jan 17, 2023

It's better to wait with this until I got around to implement the new combined error reporting and logging from sokol_spine.h into all sokol headers (see here: https://github.com/floooh/sokol/blob/2236713b5eb247907a1a0a8c0b4c44f16e472909/util/sokol_spine.h#L827-L875)

...in debug mode, this allows to output 'clickable' and human-readable error messages with file path and line number, and in release mode, it won't contain human readable strings (only error code) to not bloat the executable with unnecessary string data.

@Darkyenus
Copy link

Darkyenus commented May 7, 2024

It looks like the logging functions are in place now, so it might be a good time to return to this.

Given the logging changes, the logging part of the PR is probably no longer necessary and in my implementation I actually go in the other direction (redirecting zig logging to sokol_log), because sokol_log is better suited to various platform output functions, while std.log just goes to stderr. But the panic/assert redirect is still valuable, because Zig's @panic prints a nice stacktrace, while the current code just aborts.

It would also be nice if sokol_log's panic, which currently aborts, redirected to @panic as well.

Regarding the implementation, just a small nitpick about this line:
#define SOKOL_ASSERT(c) sokol_zig_assert((unsigned int)c, #c)
wouldn't it be better to do the if check right in the macro? It might be sligtly more performant/optimizable that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants