Follow-up fixes from PR #508 review#509
Merged
Merged
Conversation
cf_app_set_msaa() recreated the default canvas via cf_app_recreate_default_canvas_if_needed(), which no-ops once the canvas is pinned via cf_app_set_canvas_size(). That left app->sample_count updated but the actual canvas untouched, so MSAA changes silently had no effect for pinned-canvas apps even though the function reported success. Recreate the canvas directly at its current size instead, which respects pinning while still applying the new sample count.
s_refresh_pixel_scale() unconditionally overwrote app->pixel_scale from SDL_GetWindowPixelDensity() on display/pixel-size change events, even when the app was created with CF_APP_OPTIONS_NO_HIGH_DPI_BIT. That contradicted cf_app_get_pixel_scale()'s documented guarantee to always return 1.0f when HiDPI is opted out, and could unexpectedly recreate the default canvas on a display change. Skip the refresh entirely when the flag is set.
cf_app_get_size, cf_app_get_width, and cf_app_get_height all point readers to cf_app_get_pixel_scale for converting logical points to physical pixels, but none listed it in @related, making it harder to navigate to. Add it to all three for consistency.
(int)(window_w * pixel_scale) truncates for fractional pixel scales (common with Windows/Linux fractional display scaling), leaving the pinned canvas a pixel or more smaller than the actual swapchain and reintroducing the stretch blit the pinning was meant to avoid. Round instead, matching cf_app_recreate_default_canvas_if_needed()'s own use of CF_ROUNDF.
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up fixes to the HiDPI rendering work from PR #508, focusing on making pixel-scale/MSAA behavior consistent (especially for pinned canvases) and aligning docs/samples with the intended pixel-scale model.
Changes:
- Prevent pixel-scale refresh from mutating
app->pixel_scalewhenCF_APP_OPTIONS_NO_HIGH_DPI_BITis set. - Recreate the offscreen canvas on MSAA changes even for pinned-canvas apps.
- Improve HiDPI correctness/documentation via rounding in the
window_resizingsample and@relatedcross-references incute_app.h.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/cute_input.cpp | Adds an early-out to keep pixel_scale stable in NO_HIGH_DPI mode. |
| src/cute_app.cpp | Changes MSAA handling to recreate the offscreen canvas at the current size. |
| samples/window_resizing.cpp | Uses rounding (vs truncation) when pinning canvas size based on pixel scale. |
| include/cute_app.h | Adds cf_app_get_pixel_scale to related-links for window size getters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
757
to
760
| if (supported && app->sample_count != sample_count) { | ||
| app->sample_count = sample_count; | ||
| cf_app_recreate_default_canvas_if_needed(); | ||
| s_canvas(app->canvas_w, app->canvas_h); | ||
| } |
Comment on lines
507
to
+511
| // either can fire without the other depending on platform/monitor setup, so | ||
| // both are handled the same way and this is idempotent when both fire together. | ||
| static void s_refresh_pixel_scale() | ||
| { | ||
| if (app->options & CF_APP_OPTIONS_NO_HIGH_DPI_BIT) return; |
cf_app_set_msaa() unconditionally recreated the canvas via s_canvas(app->canvas_w, app->canvas_h). For an unpinned canvas, app->canvas_w/h can be stale if cf_app_set_size() was just called -- it updates app->w/h immediately but only recreates the canvas on the next WINDOW_RESIZED event. Recreating from the stale canvas_w/h would briefly bake in the old size. Only use the fixed size when the canvas is pinned; otherwise call cf_app_recreate_default_canvas_if_needed() so it recomputes from the current app->w/h and pixel_scale.
The comment atop s_refresh_pixel_scale() described it as always updating pixel_scale/recreating the canvas, but didn't mention the early return added for CF_APP_OPTIONS_NO_HIGH_DPI_BIT, making the no-op behavior easy to miss when debugging pixel-scale issues in that mode.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses review feedback left on #508 (HiDPI rendering) after it merged.
cf_app_set_msaa()now recreates the canvas at its current size instead of going throughcf_app_recreate_default_canvas_if_needed(), which no-ops oncecf_app_set_canvas_size()has pinned the canvas. Previously MSAA changes silently had no effect for pinned-canvas apps. (noted independently by @chatgpt-codex-connector and Copilot)NO_HIGH_DPI_BITin pixel scale refresh —s_refresh_pixel_scale()no longer overwritesapp->pixel_scalewhen the app opted out of HiDPI, matchingcf_app_get_pixel_scale()'s documented guarantee to always return1.0fin that case. (Copilot)cf_app_get_pixel_scalefrom window-size docs — added it to@relatedoncf_app_get_size,cf_app_get_width, andcf_app_get_heightfor consistency. (Copilot)window_resizingsample —(int)(window_w * pixel_scale)truncated for fractional pixel scales, undersizing the pinned canvas and reintroducing blur; now usesCF_ROUNDFlikecf_app_recreate_default_canvas_if_needed()does internally. (Copilot)One additional comment from #508 (claiming
WINDOW_RESIZEDdoesn't recreate the canvas on pixel-size-only changes) wasn't addressed — the same PR already added acf_app_recreate_default_canvas_if_needed()call to theWINDOW_RESIZEDhandler, so that case is already covered.Test plan
cmake --build build --target cute— builds clean (only pre-existing unrelated warnings)cmake --build build --target windowresizing— builds and links