feat(screenshot): wire engine.requestedScreenshot() into raylib + sokol templates (#227) - #210
Conversation
…ol templates (#227) Adds a per-frame screenshot capture block to both desktop templates. The block reads `engine.requestedScreenshot()` once (which checks the `LABELLE_SCREENSHOT_PATH` env var the CLI sets), and once `after_sec` wall-clock elapses calls `window.takeScreenshot(req.path)` exactly once, then quits cleanly so CI / agent flows that use the screenshot as their signal don't have to wait for a separate timeout. Raylib: existing `window.takeScreenshot` shim calls raylib's builtin `TakeScreenshot`, which picks PNG/BMP/TGA by extension. Sokol: adds a `window.takeScreenshot` stub that prints a "not yet supported" warning. Real sokol-gfx readback (Metal blit / GL `glReadPixels` / D3D11 staging) is a follow-up — see comment in backends/sokol/src/window.zig. The template wiring lands now so the CLI flag + engine helper can ship together; once the real readback lands no template changes are needed. When `LABELLE_SCREENSHOT_PATH` is unset, `engine.requestedScreenshot()` returns null and the per-frame branch is a single `null` test the optimizer collapses — no change to generated frame loops for normal runs. Bumps assembler to 0.33.0 — template change, minor bump because it changes generated `main.zig` shape for projects that set the env var. Requires labelle-engine >= 1.45.0 (provides `requestedScreenshot`).
PR SummaryLow Risk Overview Both raylib and sokol desktop templates add a per-frame block that waits until Sokol adds a Assembler version bumps 0.32.3 → 0.33.0 for the template change. Reviewed by Cursor Bugbot for commit f90f82a. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Code Review
This pull request introduces support for automated screenshot capture across the raylib and sokol backends, triggered after a specified delay to assist with CI and agent workflows. The raylib backend leverages its native screenshot capabilities, while the sokol backend currently stubs the feature with a warning. The review feedback suggests optimizing the timing logic by pre-calculating target timestamps in nanoseconds rather than performing float conversions and divisions on every frame. Additionally, it is recommended to use standard logging (std.log.warn) instead of std.debug.print for user-facing warnings in the sokol backend.
| var screenshot_req: ?engine.ScreenshotRequest = engine.requestedScreenshot(); | ||
| const loop_start_ns: i128 = std.time.nanoTimestamp(); |
There was a problem hiding this comment.
Instead of tracking the loop start time and calculating the elapsed seconds on every frame, we can pre-calculate the target timestamp in nanoseconds once on startup. This avoids float conversions, divisions, and runtime @intCast checks on every frame.
var screenshot_req: ?engine.ScreenshotRequest = engine.requestedScreenshot();
var screenshot_target_ns: ?i128 = if (screenshot_req) |req|
std.time.nanoTimestamp() + @as(i128, @intFromFloat(req.after_sec * 1_000_000_000.0))
else
null;
There was a problem hiding this comment.
Declining: the per-frame work is one float div + cmp once screenshot_req is null (the common case), which the optimizer keeps near-free. The current code's units-in-seconds reads more cleanly against the after_sec field on the request struct, and tracking loop_start_ns makes the "since main-loop entry" semantics explicit in the variable name. Happy to revisit if profiling ever shows this matters.
| if (screenshot_req) |req| { | ||
| const now_ns: i128 = std.time.nanoTimestamp(); | ||
| const elapsed_sec: f32 = @as(f32, @floatFromInt(@as(i64, @intCast(now_ns - loop_start_ns)))) / 1_000_000_000.0; | ||
| if (elapsed_sec >= req.after_sec) { | ||
| window.takeScreenshot(req.path); | ||
| screenshot_req = null; | ||
| // Exit cleanly so CI / agent flows that use the | ||
| // screenshot as their signal don't have to wait for | ||
| // a separate `--timeout` to fire. | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the pre-calculated screenshot_target_ns to check if the screenshot delay has elapsed. This simplifies the per-frame check to a simple integer comparison.
if (screenshot_target_ns) |target_ns| {
if (std.time.nanoTimestamp() >= target_ns) {
window.takeScreenshot(screenshot_req.?.path);
screenshot_target_ns = null;
// Exit cleanly so CI / agent flows that use the
// screenshot as their signal don't have to wait for
// a separate `--timeout` to fire.
break;
}
}
There was a problem hiding this comment.
Declining: the per-frame work is one float div + cmp once screenshot_req is null (the common case), which the optimizer keeps near-free. The current code's units-in-seconds reads more cleanly against the after_sec field on the request struct, and tracking loop_start_ns makes the "since main-loop entry" semantics explicit in the variable name. Happy to revisit if profiling ever shows this matters.
| var screenshot_req: ?@import("labelle-engine").ScreenshotRequest = null; | ||
| var screenshot_start_ns: i128 = 0; | ||
| var screenshot_initialized: bool = false; |
There was a problem hiding this comment.
Pre-calculate the target timestamp in nanoseconds instead of tracking the start time and calculating elapsed seconds on every frame. This avoids float conversions, divisions, and runtime @intCast checks on every frame.
var screenshot_req: ?@import("labelle-engine").ScreenshotRequest = null;
var screenshot_target_ns: ?i128 = null;
var screenshot_initialized: bool = false;
There was a problem hiding this comment.
Declining: the per-frame work is one float div + cmp once screenshot_req is null (the common case), which the optimizer keeps near-free. The current code's units-in-seconds reads more cleanly against the after_sec field on the request struct, and tracking loop_start_ns makes the "since main-loop entry" semantics explicit in the variable name. Happy to revisit if profiling ever shows this matters.
| if (!screenshot_initialized) { | ||
| screenshot_req = @import("labelle-engine").requestedScreenshot(); | ||
| screenshot_start_ns = std.time.nanoTimestamp(); | ||
| screenshot_initialized = true; | ||
| } | ||
| if (screenshot_req) |req| { | ||
| const now_ns: i128 = std.time.nanoTimestamp(); | ||
| const elapsed_sec: f32 = @as(f32, @floatFromInt(@as(i64, @intCast(now_ns - screenshot_start_ns)))) / 1_000_000_000.0; | ||
| if (elapsed_sec >= req.after_sec) { | ||
| window.takeScreenshot(req.path); | ||
| screenshot_req = null; | ||
| window.requestQuit(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the pre-calculated screenshot_target_ns to check if the screenshot delay has elapsed. This simplifies the per-frame check to a simple integer comparison.
if (!screenshot_initialized) {
screenshot_req = @import("labelle-engine").requestedScreenshot();
if (screenshot_req) |req| {
screenshot_target_ns = std.time.nanoTimestamp() + @as(i128, @intFromFloat(req.after_sec * 1_000_000_000.0));
}
screenshot_initialized = true;
}
if (screenshot_target_ns) |target_ns| {
if (std.time.nanoTimestamp() >= target_ns) {
window.takeScreenshot(screenshot_req.?.path);
screenshot_target_ns = null;
window.requestQuit();
}
}
There was a problem hiding this comment.
Declining: the per-frame work is one float div + cmp once screenshot_req is null (the common case), which the optimizer keeps near-free. The current code's units-in-seconds reads more cleanly against the after_sec field on the request struct, and tracking loop_start_ns makes the "since main-loop entry" semantics explicit in the variable name. Happy to revisit if profiling ever shows this matters.
| std.debug.print( | ||
| "labelle: screenshot requested but not supported on sokol backend yet ({s})\n", | ||
| .{path}, | ||
| ); |
There was a problem hiding this comment.
Use standard logging facilities (std.log.warn) instead of std.debug.print for user-facing warnings, as per the project's general rules.
std.log.warn(
"screenshot requested but not supported on sokol backend yet ({s})",
.{path},
);
References
- In CLI tools, use standard logging facilities (e.g.,
std.log.warn) or write tostderrfor user-facing warnings, rather than using debug-specific print functions (e.g.,std.debug.print).
There was a problem hiding this comment.
Addressed in 2eec524 — swapped to std.log.warn so the message respects the configured log level instead of always firing through std.debug.print.
Zig 0.16 removed std.time.nanoTimestamp; the engine helper now exposes `nowNs()` which calls libc clock_gettime under the hood. Use it in both the raylib and sokol screenshot timing blocks.
|
@copilot review |
Please share the specific change(s) you want me to make in this PR (file/behavior), and I’ll update it right away. |
Gemini flagged std.debug.print as not matching project convention for user-facing warnings. Swap to std.log.warn so the message lands on the standard log channel and respects the configured log level (debug.print always fires, even in release modes where logs are filtered). The message body is unchanged modulo the trailing newline (std.log adds its own line break).
Summary
backends/raylib/templates/desktop.txt,backends/sokol/templates/desktop.txt).window.takeScreenshotstub that prints "not yet supported" — real sokol-gfx readback (Metal blit / GLglReadPixels/ D3D11 staging) is a follow-up; the template wiring lands now so the CLI flag + engine helper can ship together.window.takeScreenshotshim that calls raylib'sTakeScreenshot(PNG/BMP/TGA by extension).Behavior
LABELLE_SCREENSHOT_PATHis unset, the new block is a singlenulltest the optimizer collapses — no change to generatedmain.zigshape for normal runs.window.takeScreenshot(req.path)exactly once afterreq.after_secwall-clock elapses (measured from main-loop entry), then exits cleanly so CI / agent flows don't need a separate--timeout.Test plan
zig buildokzig build test— exit 0 (template files are runtime-loaded; specs cover codegen + harness)labelle run --screenshot=/tmp/x.png --after=2s)Depends on
engine.requestedScreenshot)