fix(ci): green up ffi-parity + macOS clustered-lights golden#84
Merged
Conversation
Two checks were red on main since the round-2 branch (pre-dating the lag/flicker work); this makes both green. ffi-parity: - Regenerate native/watchos/src/ffi_stubs.rs from the manifest (gen_stubs.js). The round-2 FFI additions (numeric profiler ABI, present-mode, sharpen, scene setters, splat, scratch buffers) were added to package.json but the watchOS no-op stubs were never regenerated, so name-coverage failed for 14 functions. - Add the 16 web-absent functions to the documented WEB_GAP_ALLOWLIST, grouped by reason: profiler (needs TIMESTAMP_QUERY, absent on WebGPU), present-mode (browser owns vsync), sharpen (post-FX group, same as the existing bloom_set_bloom_intensity gap), scene setters + scratch buffers (same Perry-WASM linear-memory bridge TODO as the existing mesh-scratch / scene_set_lod gaps), and splat_impulse. shared-tests (macOS/Metal): - golden_many_point_lights_clustered_scene diverges ~4.5/255 mean on Metal (0 outliers, max 19) vs the Vulkan/DX reference golden, which Linux/Windows match under 2.0 — a uniform fp/accumulation-order difference in the froxel clustered light loop, not a broken region. Add a per-test mean tolerance (6.0 for this scene) while keeping the strict OUTLIER_FRACTION gate global, so a real regression (localized >32/255 patch) still fails. Regenerating a Metal golden retires it. Verified locally: bun tools/validate-ffi.js -> 0 failures; the golden test compiles and passes on the Windows backend.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates golden-image tolerance handling, adds missing watchOS FFI stubs plus matching web allowlist entries, and moves renderer GI bake logic into a new submodule with clipmap, WSRC, and per-mesh SDF baking routines. ChangesGolden Render Tolerance
watchOS FFI Stubs and Web Allowlist
Renderer GI Bake Split
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant SceneGraph
participant wgpuCommandEncoder as wgpu::CommandEncoder
Renderer->>Renderer: maybe_invalidate_sdf_clipmap()
Renderer->>SceneGraph: bake_scene_sdf_clipmap(scene, encoder)
Renderer->>wgpuCommandEncoder: encode_clipmap_bake_slices(job, encoder)
Renderer->>Renderer: maybe_invalidate_wsrc()
Renderer->>wgpuCommandEncoder: bake_wsrc(encoder)
Renderer->>SceneGraph: bake_pending_sdfs(scene, encoder)
Renderer->>wgpuCommandEncoder: copy_texture_to_buffer for sdf cache readback
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The round-2 + lag/flicker work grew renderer/mod.rs to 12560 lines, past its grandfathered 11985 baseline in the file-line guard (this surfaced only after the ffi-parity coverage fix let the check-file-lines step run). Move the scene-SDF-clipmap / WSRC / per-mesh-SDF bake methods (current_camera_world_pos, maybe_invalidate_sdf_clipmap, bake_scene_sdf_clipmap, encode_clipmap_bake_slices, maybe_invalidate_wsrc, bake_wsrc, bake_pending_sdfs) into a new gi_bake.rs `impl Renderer` block, following the existing scene_pass.rs / shadow_pass.rs / postfx_chain.rs split pattern. Pure code move, no behaviour change; the five methods called from end_frame_with_scene are now pub(super). mod.rs drops to 11956 (under baseline); gi_bake.rs is 614 lines. shared crate builds clean.
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.
Greens up the two checks that were red on main since the round-2 branch (they pre-date the lag/flicker session; surfaced on main via the round-2 consolidation merge #83).
ffi-parity
native/watchos/src/ffi_stubs.rsfrom the manifest viagen_stubs.js. The round-2 FFI additions (numeric profiler ABI, present-mode, sharpen, scene setters, splat, scratch buffers) were added topackage.jsonbut the watchOS no-op stubs were never regenerated -> 14 name-coverage failures.WEB_GAP_ALLOWLIST, grouped by reason (profiler needs TIMESTAMP_QUERY absent on WebGPU; present-mode is browser-owned; sharpen joins the existing post-FX gap; scene setters + scratch buffers share the existing Perry-WASM linear-memory TODO; splat_impulse not yet wired).shared-tests (macOS / Metal)
golden_many_point_lights_clustered_scenediverges ~4.5/255 mean on Metal (0 outliers, max 19) from the Vulkan/DX reference golden that Linux/Windows match under 2.0 — a uniform fp/accumulation-order difference in the froxel clustered light loop, not a broken region. Added a per-test mean tolerance (6.0 for this scene); the strict globalOUTLIER_FRACTIONgate is unchanged, so a real regression (localized >32/255 patch) still fails. Regenerating a Metal golden retires the override.Verified locally:
bun tools/validate-ffi.js-> 0 failures; golden test compiles + passes on the Windows backend. Leaving this open for CI (incl. macOS Metal) to confirm green before merge.Summary by CodeRabbit