feat(sdl): conform window to the canonical contract (#386) - #437
Conversation
First step of extracting the SDL backend out-of-tree (epic #386): make backends/sdl/src/window.zig satisfy labelle-core's canonical window contract (core.assertWindow), mirroring the raylib backend's prior conformance. This is purely ADDITIVE and byte-identical for generated output: the generated SDL run-loop templates still call the legacy names (windowShouldClose, getScreenWidth via gfx, beginDrawing, ...). The new decls are thin aliases/wrappers over SDL's existing internals: - width()/height() -> gfx.getScreenWidth()/getScreenHeight() - frameDuration() -> seconds since last call from a dedicated SDL_GetPerformanceCounter baseline (SDL stores no frame-time; the FPS limiter discards its delta) - requestQuit() -> latches the existing should_close flag - shouldQuit() -> alias of windowShouldClose() (its presence marks SDL as a loop-model backend) Also resets should_close and the frameDuration baseline at the top of initWindow so a close->reopen starts clean, and renames initWindow's width/height params to width_px/height_px to avoid shadowing the new module-level width()/height() decls. Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe SDL backend window module gains a canonical window contract: new exported functions width(), height(), frameDuration(), requestQuit(), and shouldQuit(). A frame_dur_last timing baseline is added and initialized alongside the existing FPS limiter, and initWindow() now clears should_close on reinitialization. ChangesSDL Window Contract
Sequence Diagram(s)sequenceDiagram
participant App
participant WindowModule
participant gfx
App->>WindowModule: initWindow(width_px, height_px, title)
WindowModule->>WindowModule: reset should_close, last_frame_time, frame_dur_last
loop Frame loop
App->>WindowModule: frameDuration()
WindowModule-->>App: elapsed seconds
App->>WindowModule: width() / height()
WindowModule->>gfx: query size
gfx-->>WindowModule: dimensions
App->>WindowModule: shouldQuit()
WindowModule-->>App: bool
end
App->>WindowModule: requestQuit()
WindowModule->>WindowModule: set should_close = true
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the canonical window contract for the SDL backend in backends/sdl/src/window.zig, adding functions such as width(), height(), frameDuration(), requestQuit(), and shouldQuit(). The review feedback identifies a critical issue with the current implementation of frameDuration(): because it mutates the baseline state (frame_dur_last) on every call, it is not idempotent and will return incorrect values if queried multiple times within the same frame. The reviewer suggests caching the calculated frame duration once per frame (e.g., in beginDrawing()) and returning this cached value to ensure consistent timing across all subsystems.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var should_close: bool = false; | ||
| var target_fps_val: i32 = 60; | ||
| var last_frame_time: u64 = 0; | ||
| var frame_dur_last: u64 = 0; // baseline for the canonical frameDuration() dt source |
There was a problem hiding this comment.
To make frameDuration() idempotent and safe to call multiple times per frame, we should cache the calculated frame duration in a state variable. Declare frame_dur_seconds here, which will be updated once per frame (e.g., in beginDrawing()).
var frame_dur_last: u64 = 0; // baseline for the canonical frameDuration() dt source
var frame_dur_seconds: f64 = 0.0; // cached frame duration for idempotent queries
| const now = c.SDL_GetPerformanceCounter(); | ||
| last_frame_time = now; | ||
| frame_dur_last = now; // reset the frameDuration() baseline too |
There was a problem hiding this comment.
Reset the cached frame_dur_seconds to 0.0 during window initialization to ensure a clean state on re-initialization.
const now = c.SDL_GetPerformanceCounter();
last_frame_time = now;
frame_dur_last = now; // reset the frameDuration() baseline too
frame_dur_seconds = 0.0;
| pub fn frameDuration() f64 { | ||
| const freq = c.SDL_GetPerformanceFrequency(); | ||
| const now = c.SDL_GetPerformanceCounter(); | ||
| const elapsed = now - frame_dur_last; | ||
| frame_dur_last = now; | ||
| if (freq == 0) return 0; | ||
| return @as(f64, @floatFromInt(elapsed)) / @as(f64, @floatFromInt(freq)); | ||
| } |
There was a problem hiding this comment.
Non-idempotent frameDuration()
The current implementation of frameDuration() mutates frame_dur_last on every call. If multiple subsystems or scripts query frameDuration() in the same frame, only the first call returns the correct elapsed time, while subsequent calls return approximately 0.0 seconds. This can cause severe timing and physics bugs.
Solution
- Simplify
frameDuration()to return a cachedframe_dur_secondsvariable. - Update
frame_dur_secondsonce per frame at the start ofbeginDrawing()(which is outside the modified diff hunks, so you will need to add this manually):
pub fn beginDrawing() void {
const freq = c.SDL_GetPerformanceFrequency();
const now = c.SDL_GetPerformanceCounter();
const elapsed = now - frame_dur_last;
frame_dur_last = now;
frame_dur_seconds = if (freq == 0) 0 else @as(f64, @floatFromInt(elapsed)) / @as(f64, @floatFromInt(freq));
input.newFrame();
// ... rest of beginDrawing()
}pub fn frameDuration() f64 {
return frame_dur_seconds;
}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb5e21342c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const freq = c.SDL_GetPerformanceFrequency(); | ||
| const now = c.SDL_GetPerformanceCounter(); | ||
| const elapsed = now - frame_dur_last; | ||
| frame_dur_last = now; |
There was a problem hiding this comment.
When any SDL consumer queries frameDuration() more than once during the same frame, this assignment advances the baseline on the first read, so the second read reports only the time between the two calls instead of the last frame's duration. Other backends expose this as a query of the last-frame dt source, so using SDL through the canonical window contract can make one subsystem receive the real dt while another gets near-zero dt; update/cache the value once per frame instead of consuming it inside the getter.
Useful? React with 👍 / 👎.
|
|
||
| /// Current framebuffer width (physical px). | ||
| pub fn width() i32 { | ||
| return gfx.getScreenWidth(); |
There was a problem hiding this comment.
When the SDL drawable size changes, for example after setFullscreen(true) switches the window to desktop fullscreen, this still returns the init-time gfx cache because gfx.setScreenSize is only called during initWindow and no resize/fullscreen path refreshes it. The canonical window API is meant to report the current physical framebuffer dimensions, so external/shared loops using window.width()/height() will keep layout or render targets at the old size; query SDL for the current output size or update the cache on size changes.
Useful? React with 👍 / 👎.
…nDrawing) (#444) The conformance's frameDuration() advanced its baseline on every call, so a second query within a frame returned ~0 (flagged by gemini/codex on #437). Compute the dt once per frame in beginDrawing, cache it, and return the cache — idempotent. Not exercised by current templates (fixed dt), but correct for when the extracted backend's manifest template wires window.frameDuration() as dt.
…445) sdl extraction step (window conformed in #437). Opts the sdl DESKTOP build into the manifest-splice path. Loop-style, desktop-only, NO params; EMPTY link beyond the single blank the enum `.link_sdl` emitted (SDL2 is linked by the backend's own build.zig). The backend_dep fragment carries the input core-diamond override (underscore `labelle_core` key, #258). Byte-identical to the enum path (diffed a generated baseline → 0 diff). zig build test green.
* feat(sdl): add backend.manifest.zon — manifest-splice codegen (#386) sdl extraction step (window conformed in #437). Opts the sdl DESKTOP build into the manifest-splice path. Loop-style, desktop-only, NO params; EMPTY link beyond the single blank the enum `.link_sdl` emitted (SDL2 is linked by the backend's own build.zig). The backend_dep fragment carries the input core-diamond override (underscore `labelle_core` key, #258). Byte-identical to the enum path (diffed a generated baseline → 0 diff). zig build test green. * feat(manifest): emit pack/feature manifest sidecar (#442) Extend the generated catalog into the RFC §7 pack/feature manifest, written to <game>/.labelle/manifest.json alongside flow_catalog.json. Two-tier, realm-structured + sliceable: - index (always loaded): contracts (events/enums) + a realm map (game root + each plugin) with owns / depends_on / exposes (commands vs queries, derived from FlowNode void-ness) / recipes. - per-realm detail: game realm gets full component field schemas + save policy + event payloads (light AST pass over components/ and events/); plugin realms surface event + flow-node names (payloads already live in flow_catalog.json). Everything except recipes is derived from already-scanned data, so it can't drift. Additive: a failure is logged, never fatal (same contract as the flow catalog). Deferred and called out in the schema: recipes (empty, share the scaffold source — cli #271), event emitted_by/subscribed_by cross-refs (empty arrays), and component visibility (needs pack.labelle). Part of #651, implements #442. * fix(manifest): address review findings on pack/feature manifest sidecar (#442) - parseStructFile: deinit the parsed Ast (per-file leak); propagate OOM from extractSavePolicy instead of swallowing it via `catch null`. - parseStructDir: propagate OutOfMemory; degrade non-fatal read failures and no-match files to a name-only StructDecl (file-stem Pascal name, empty fields) so the manifest still lists what the registries import. - parseStructDir: emit only the file-stem Pascal decl per file, so helper containers (`const Options`/`pub const Clip`) no longer leak as phantom components/events. - contracts.events: realm-qualify entries (`engine.tick`, `box2d.collision_begin`, `game.<Event>`) to match the toolchain's dotted qualified form and avoid cross-realm name collisions. - Emit a dedicated `engine` realm (index + detail) when engine lifecycle events are discovered, so `engine.<event>` contract entries resolve to a realm that actually appears. - Game-realm exposes: script-qualify FlowNode commands/queries (`flows.hit_counter.spawn`) via the scriptModuleLabel dotted form so two scripts exposing the same bare name stay distinct. - root.zig: use std.log.warn (not std.debug.print) for the manifest sidecar emission-failed warning. - Tests: name-only fallback + helper-container exclusion (parseStructDir); engine realm emission + realm-qualified contracts + script-qualified exposes (writeManifestJson). Claude-Session: https://claude.ai/code/session_017pW3ifKf9wgxNg4viy6okw
1 participant
First step of extracting SDL out-of-tree (#386):
backends/sdl/src/window.zigsatisfies labelle-core'sassertWindow. Additive + byte-identical (templates still call legacy names). Adds width/height (gfx size), frameDuration (SDL_GetPerformanceCounter delta), requestQuit (latches should_close), shouldQuit (alias → loop-style); initWindow params renamed*_px; state reset on re-init.backends/sdl+ repo-root tests green.Summary by CodeRabbit
New Features
Bug Fixes