Repository navigation
fix(sdl): make frameDuration() idempotent (#437 follow-up) - #444
Conversation
…nDrawing) 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.
📝 WalkthroughWalkthroughThe SDL window backend now computes frame delta time once per frame inside beginDrawing, caching it in a new frame_dur_seconds field. frameDuration() returns this cached value instead of recomputing it from a performance counter each call. initWindow resets the cache to 1/60 on (re)open. ChangesFrame duration caching
Sequence Diagram(s)sequenceDiagram
participant App
participant beginDrawing
participant frameDuration
App->>beginDrawing: start frame
beginDrawing->>beginDrawing: compute dt from perf counter delta
beginDrawing->>beginDrawing: cache frame_dur_seconds, update frame_dur_last
App->>frameDuration: query dt
frameDuration->>App: return cached frame_dur_seconds
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 refactors frame duration tracking in the SDL backend by caching the delta time once per frame in beginDrawing(), making frameDuration() idempotent. Feedback suggests resetting frame_dur_last to 0 in initWindow to prevent the first frame's delta time from being incorrectly overwritten, and adding a guard now >= frame_dur_last to avoid potential integer underflow panics from performance counter drift.
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.
| const now = c.SDL_GetPerformanceCounter(); | ||
| last_frame_time = now; | ||
| frame_dur_last = now; // reset the frameDuration() baseline too | ||
| frame_dur_seconds = 1.0 / 60.0; // and its cached value, so a re-open starts clean |
There was a problem hiding this comment.
To prevent the first frame's delta time from being overwritten to ~0 in beginDrawing, we should reset frame_dur_last to 0 here. This ensures that the first call to beginDrawing skips the elapsed time calculation and preserves the seeded 1.0 / 60.0 value.
frame_dur_last = 0; // reset baseline to 0 so the first frame uses the seeded value
frame_dur_seconds = 1.0 / 60.0; // and its cached value, so a re-open starts clean
| if (freq != 0 and frame_dur_last != 0) { | ||
| frame_dur_seconds = @as(f64, @floatFromInt(now - frame_dur_last)) / @as(f64, @floatFromInt(freq)); | ||
| } |
There was a problem hiding this comment.
Add a guard now >= frame_dur_last to prevent potential integer underflow panics in Debug or ReleaseSafe modes if the OS/hardware performance counter ever reports a slightly backward value or drifts.
if (freq != 0 and frame_dur_last != 0 and now >= frame_dur_last) {
frame_dur_seconds = @as(f64, @floatFromInt(now - frame_dur_last)) / @as(f64, @floatFromInt(freq));
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backends/sdl/src/window.zig (1)
132-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPer-frame dt computation is correctly idempotent.
Computing the delta once in
beginDrawingagainstframe_dur_last, guarding for the zero-baseline/zero-frequency edge case, and updating the baseline afterward correctly fixes the original bug where a secondframeDuration()call within the same frame returned near zero. The separation fromlast_frame_time(used only for FPS-cap delay inendDrawing, and only updated whentarget_fps_val > 0) is intentional and avoids coupling the two timers.Consider adding a regression test asserting two consecutive
frameDuration()calls within the same frame return the same value, since this was the exact bug being fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backends/sdl/src/window.zig` around lines 132 - 143, The frame delta caching in beginDrawing and frameDuration is now correct, but the fix should be protected with a regression test. Add a test around the window timing path in backends/sdl/src/window.zig that exercises beginDrawing and calls frameDuration() twice within the same frame, asserting both calls return the same value; use the existing frame_dur_last/frame_dur_seconds behavior as the target of the check so the test catches any future drift in idempotence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backends/sdl/src/window.zig`:
- Around line 132-143: The frame delta caching in beginDrawing and frameDuration
is now correct, but the fix should be protected with a regression test. Add a
test around the window timing path in backends/sdl/src/window.zig that exercises
beginDrawing and calls frameDuration() twice within the same frame, asserting
both calls return the same value; use the existing
frame_dur_last/frame_dur_seconds behavior as the target of the check so the test
catches any future drift in idempotence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f3f4bff-8b77-4a8a-8dfc-d2c1e4d9c182
📒 Files selected for processing (1)
backends/sdl/src/window.zig
![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: fa97448f64
ℹ️ 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(); | ||
| if (freq != 0 and frame_dur_last != 0) { | ||
| frame_dur_seconds = @as(f64, @floatFromInt(now - frame_dur_last)) / @as(f64, @floatFromInt(freq)); |
There was a problem hiding this comment.
When an SDL loop uses the new canonical frameDuration() as its dt source after calling beginDrawing(), the first beginDrawing() overwrites the 1/60 seed with the time since initWindow() before any frame has actually completed. If startup/setup/loading takes noticeable time, that startup interval is reported as the first frame's dt instead of the documented seed, causing an initial simulation jump or clamp; keep the seed until a completed frame has been presented/delayed, or make the first beginDrawing() only establish the baseline.
Useful? React with 👍 / 👎.
1 participant
Addresses the gemini-high/codex-P2 cluster on #437:
frameDuration()mutated its baseline on every call → a 2nd query/frame returned ~0. NowbeginDrawingcomputes the dt once per frame and caches it;frameDuration()returns the cache (idempotent). Latent today (generated templates use a fixed dt) but correct for the extracted backend's manifest template.backends/sdltests green.Summary by CodeRabbit