fix(render): honour render_scale from the first frame on every host#88
Conversation
`Renderer::new_impl` sizes the depth/HDR/G-buffer targets to the full surface, but the pass chain runs at `render_extent()` (surface * render_scale, 0.5 by default). `resize()` is the only thing that reconciles the two, and until now the only caller that ran it at boot was the Windows crate, which patched itself in-place. Every other host boots through `attach_engine` (macOS, iOS/iPadOS, tvOS, Linux, Android, visionOS) and never resizes on frame 1: those platforms poll the OS size and only resize when it *changes*, which is false on a plain windowed launch. They rendered against targets that ignored render_scale — post-FX silently dropped out, and the depth-snapshot copy spans a partial depth texture, which wgpu rejects outright once a scene-reading translucent material is in view (the fatal first-frame validation error the Windows fix was chasing). Verified on an M1 Max: before this change the macOS boot path rendered visibly differently from the resized Windows path (mean 1.076/255, max 191, bloom/glow highlights missing); render_extent reported 128x128 against 256x256 targets. Fix it at the root instead: `Renderer::new_impl` now runs `resize()` before returning, so construction-time targets are always correct and every platform inherits it. The Windows band-aid is removed — it was masking the bug for one platform and hiding it for the rest. The golden suite structurally cannot catch this: its harness calls `set_taa_enabled(false)` right after construction, which internally resizes and repairs the invariant before anything renders. So add tests/render_targets.rs, which asserts render_target_extent() == render_extent() straight out of the constructor (the real frame-1 state) and that it survives set_render_scale / resize / set_taa_enabled. Needs one new accessor, `Renderer::render_target_extent()`. All 7 goldens pass unchanged, which is the evidence this is pixel-neutral wherever a resize already happened and only changes behaviour where it was genuinely broken. Also files EN-024: iOS/iPadOS/tvOS report pixels as the logical size (macOS reports points), which makes the physical-resolution glyph work inert on Apple mobile and diverges game-facing coordinates. Left as a decision, not folded in here — both resolutions are breaking.
📝 WalkthroughWalkthroughRenderer construction now synchronizes render targets with the effective render extent, Windows initialization uses this behavior, GPU tests verify sizing invariants, and documentation adds an Apple pixel-convention decision ticket. ChangesRenderer target synchronization
Apple pixel convention decision
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
native/shared/src/renderer/mod.rs (1)
6533-6547: 🚀 Performance & Scalability | 🔵 TrivialVerify boot-time cost of the now-duplicated render-target allocation.
This correctly fixes the render-scale bug, but note every render-resolution target (depth/HDR/G-buffer/bloom/SSAO/hiz/SSR/SSGI/probe history, plus the surface reconfigure / headless-target rebuild) is now allocated twice at construction: once at full surface size inside the constructor body, then again at
render_extent()size via thisresize()call. This is one-time-per-boot cost on every platform (previously only Windows paid an equivalent cost), so it's worth confirming it doesn't introduce a noticeable startup-latency regression on resource-constrained targets (mobile GPUs on iOS/Android), since the PR notes compile validation for those platforms but not boot-time profiling.🤖 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 `@native/shared/src/renderer/mod.rs` around lines 6533 - 6547, Verify boot-time allocation cost for the renderer initialization followed by renderer.resize, profiling representative iOS and Android devices and comparing startup latency against the prior implementation. Confirm that the duplicate render-target allocation does not cause a noticeable regression; if it does, initialize render targets at render_extent() during construction so the first resize does not rebuild them.
🤖 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 `@native/shared/src/renderer/mod.rs`:
- Around line 6533-6547: Verify boot-time allocation cost for the renderer
initialization followed by renderer.resize, profiling representative iOS and
Android devices and comparing startup latency against the prior implementation.
Confirm that the duplicate render-target allocation does not cause a noticeable
regression; if it does, initialize render targets at render_extent() during
construction so the first resize does not rebuild them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd239e9e-adf3-4457-b252-3c9b8bbd47a1
📒 Files selected for processing (4)
docs/tickets.mdnative/shared/src/renderer/mod.rsnative/shared/tests/render_targets.rsnative/windows/src/lib.rs
PR #87/#88's web work pushed native/web/src/lib.rs to 2034 lines (limit 2000). Pure code motion, same pattern as input_ffi.rs: the Phase-1c material-system FFI section (compile/params/draw submission, instance buffers, texture arrays, reflection probes) moves to material_ffi.rs. lib.rs 2034 -> 1546; cargo check for wasm32-unknown-unknown green; validate-ffi passes.
The bug
Renderer::new_implsizes the depth/HDR/G-buffer targets to the full surface, but the pass chain runs atrender_extent()(surface * render_scale, 0.5 by default).resize()is the only thing that reconciles the two — and the only caller that ran it at boot was the Windows crate, which patched itself in place (native/windows/src/lib.rs:678).Every other host boots through
attach_engine— macOS, iOS/iPadOS, tvOS, Linux, Android, visionOS — and never resizes on frame 1: those platforms poll the OS size and resize only when it changes, which is false on a plain windowed launch. Nothing in the engine's TS boot path callssetTaaEnabled/setRenderScaleeither, so a default game stays in the mismatched state.Consequences on those platforms:
Evidence (M1 Max / Metal)
Before this change, the macOS boot path reported
render_extent128×128 against 256×256 targets, and rendering the same scene through the macOS boot path vs. the (resized) Windows boot path differed by mean 1.076/255, max 191, with the bloom/glow highlights missing entirely on macOS.The fix
Do it at the root:
Renderer::new_implnow runsresize()before returning, so construction-time targets always honourrender_scaleand every platform inherits it. The Windows band-aid is removed — it was fixing one platform and hiding the bug for the other six.Tests
The golden suite structurally cannot catch this: its harness calls
set_taa_enabled(false)immediately after construction, which internally resizes and repairs the invariant before anything renders.So this adds
native/shared/tests/render_targets.rs, assertingrender_target_extent() == render_extent()straight out of the constructor (the real frame-1 state), and that the invariant survivesset_render_scale/resize/set_taa_enabled. Needs one new accessor,Renderer::render_target_extent().macos,ios,tvoscrates and the shared WASM target all compile;validate-fficlean.Also
Files EN-024: iOS/iPadOS/tvOS report pixels as the logical size while macOS reports points. That makes the physical-resolution glyph work (60ba704) inert on Apple mobile (
dpi = physical / logicalis always 1.0 there) and diverges game-facing coordinates across Apple targets. Left as a decision rather than folded in here — both resolutions are breaking changes.Note: pre-existing CI red on
maintools/check-file-lines.jsalready fails on cleanmain, independently of this PR:native/shared/src/renderer/mod.rs: 12118 lines vs. its grandfathered 11985 baselinenative/shared/src/renderer/material_system.rs: 2054 lines vs. the 2000 limitThis PR adds 23 lines to
mod.rs(already over). Fixing that debt is a split/refactor that doesn't belong in a bug fix — flagging it so it isn't mistaken for fallout from this change.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation