fix: material-path shadow/IBL wiring + multi-primitive skin pose sharing#86
Merged
Merged
Conversation
added 3 commits
July 11, 2026 05:04
- impulse_field.wgsl: clamp the accumulated field to 1.0 (and square the falloff) so repeated wade splats at one spot no longer latch a full-strength splat for seconds of decay. - planar_reflection.rs: probe passes now carry dummy material/velocity/ albedo attachments so opaque-profile material pipelines (4-target layout) validate against the probe pass. - material_system.rs: skip translucent-profile and reads_scene materials when dispatching with the reflection pipeline - single-target pipelines and missing group-4 binds were a wgpu validation panic.
All three cascade passes are encoded before the queue submits, and queue.write_buffer executes at submit - so sharing one uniform region meant every cascade rendered with the LAST cascade's light_vp + model matrices. Near receivers sample cascades 0/1, whose depth content was garbage, so the player and enemies never had shadows; distant trees kept theirs because cascade 2's write happened to be the surviving one. The uniform buffer is now NUM_CASCADES regions with per-cascade base offsets for both the writes and the dynamic bind offsets.
…rimitives
Two root causes behind 'no shadows ever' and the mangled marauder:
1. The material system's per-view bind group was created at init with
1x1 stubs on the env/BRDF/shadow slots ('Phase 2 wires them' - it
never did). wgpu zero-inits textures, so sample_sun_shadow read
stored depth 0.0 everywhere: every in-frustum fragment on a material
surface (terrain/grass/water) compared occluded and the whole
ground rendered sunless and shadow-blind, while the planar-probe
path - which builds a live bind group per frame - showed correct
shadows in water reflections. New refresh_material_per_view_bg()
mirrors the probe group (real env, BRDF LUT, cascade depth views),
runs on first end_frame and re-arms when load_env_from_hdr swaps
the panorama.
2. bloom_draw_model popped one staged skin pose per skinned PRIMITIVE,
but updateModelAnimation stages one per MODEL. On two-primitive
GLBs (marauder, tyrant) the second primitive found the FIFO empty,
fell back to joint offset 0 - the player's pose block - and
rendered as a giant mangled blob glued to the player. The FFI now
pops once per drawModel (take_staged_skin_offset) and every
primitive shares the offset via the new *_with_joints variants;
the old per-mesh entry points remain as wrappers.
Also folds in the round-5 cached path for bloom_draw_model_rotated
(draw_model_cached_rotated) so rotated static models get cutout,
material maps, and cutout shadows instead of the immediate batch.
📝 WalkthroughWalkthroughThe renderer refreshes live material bindings, restructures planar reflections into two passes, supports staged skinning and rotated cached models, isolates shadow uniforms per cascade, clamps impulse-field shader output, and updates a Metal golden tolerance. ChangesRenderer pipeline updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant PlanarReflectionProbe
participant UserMaterials
participant CachedModels
Renderer->>PlanarReflectionProbe: obtain reflection, depth, and auxiliary views
Renderer->>UserMaterials: execute Pass A with multiple color attachments
Renderer->>CachedModels: execute Pass B using Pass A depth
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@native/shared/src/renderer/mod.rs`:
- Around line 8717-8763: Update refresh_material_per_view_bg to prefer
procedural_sky_equirect_full_view and procedural_env_diffuse_view when
available, falling back to sky_texture and env_diffuse_texture/default views
otherwise. In set_procedural_sky and bake_procedural_ibl, invalidate
material_per_view_bg_live after changing or baking procedural resources so the
next end_frame rebuilds the bind group.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fd3ed42-b63c-4ca7-b11b-ef47a8484876
📒 Files selected for processing (7)
native/shared/shaders/impulse_field.wgslnative/shared/src/ffi_core/models.rsnative/shared/src/renderer/material_system.rsnative/shared/src/renderer/mod.rsnative/shared/src/renderer/planar_reflection.rsnative/shared/src/renderer/shadow_pass.rsnative/shared/src/shadows.rs
Comment on lines
+8717
to
+8763
| /// Rebuild the material system's per-view bind group with the LIVE | ||
| /// env / BRDF / shadow resources. The group created at material-system | ||
| /// init binds 1×1 stubs ("Phase 2 wires them" — this is that wiring): | ||
| /// the zero-init stub depth views read 0.0 at every texel, so any | ||
| /// in-frustum fragment compared as "occluded" and the whole material | ||
| /// path (terrain, grass, water) rendered sunless and shadowless while | ||
| /// the planar-probe path — which builds a live group per frame — | ||
| /// showed correct shadows in reflections. Mirrors that probe group | ||
| /// exactly (mod.rs "planar_probe_per_view_bg_live"). | ||
| fn refresh_material_per_view_bg(&mut self) { | ||
| let sky_view_owned: Option<wgpu::TextureView> = self.sky_texture | ||
| .as_ref() | ||
| .map(|t| t.create_view(&Default::default())); | ||
| let env_view: &wgpu::TextureView = sky_view_owned | ||
| .as_ref() | ||
| .unwrap_or(&self.scene_env_default_view); | ||
| let diffuse_view_owned: Option<wgpu::TextureView> = self.env_diffuse_texture | ||
| .as_ref() | ||
| .map(|t| t.create_view(&Default::default())); | ||
| let env_diffuse_view: &wgpu::TextureView = diffuse_view_owned | ||
| .as_ref() | ||
| .unwrap_or(&self.scene_env_default_view); | ||
|
|
||
| let bg = self.device.create_bind_group(&wgpu::BindGroupDescriptor { | ||
| label: Some("material_per_view_bg_live"), | ||
| layout: &self.material_system.layouts.per_view, | ||
| entries: &[ | ||
| wgpu::BindGroupEntry { binding: 0, resource: self.material_system.per_view_buffer.as_entire_binding() }, | ||
| wgpu::BindGroupEntry { binding: 1, resource: wgpu::BindingResource::TextureView(env_view) }, | ||
| wgpu::BindGroupEntry { binding: 2, resource: wgpu::BindingResource::Sampler(&self.env_sampler) }, | ||
| wgpu::BindGroupEntry { binding: 3, resource: wgpu::BindingResource::TextureView(env_diffuse_view) }, | ||
| wgpu::BindGroupEntry { binding: 4, resource: wgpu::BindingResource::TextureView(&self.brdf_lut_view) }, | ||
| wgpu::BindGroupEntry { binding: 5, resource: wgpu::BindingResource::Sampler(&self.brdf_lut_sampler) }, | ||
| wgpu::BindGroupEntry { binding: 6, resource: wgpu::BindingResource::TextureView(&self.shadow_map.depth_views[0]) }, | ||
| wgpu::BindGroupEntry { binding: 7, resource: wgpu::BindingResource::TextureView(&self.shadow_map.depth_views[1]) }, | ||
| wgpu::BindGroupEntry { binding: 8, resource: wgpu::BindingResource::TextureView(&self.shadow_map.depth_views[2]) }, | ||
| wgpu::BindGroupEntry { binding: 9, resource: wgpu::BindingResource::Sampler(&self.shadow_map.sampler) }, | ||
| ], | ||
| }); | ||
| self.material_system.per_view_bg = bg; | ||
| self.material_per_view_bg_live = true; | ||
| } | ||
|
|
||
| pub fn end_frame(&mut self) { | ||
| if !self.material_per_view_bg_live { | ||
| self.refresh_material_per_view_bg(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and symbols first.
ast-grep outline native/shared/src/renderer/mod.rs --view expanded > /tmp/mod_outline.txt || true
printf '\n=== outline excerpt ===\n'
sed -n '1,220p' /tmp/mod_outline.txt
printf '\n=== symbol search ===\n'
rg -n "refresh_material_per_view_bg|set_procedural_sky|bake_procedural_ibl|material_per_view_bg_live|procedural_sky_equirect_full_view|procedural_env_diffuse_view|lighting_bg_is_procedural|load_env_from_hdr|compile_material|submit_material_draw|lighting_bind_group" native/shared/src/renderer/mod.rs
printf '\n=== surrounding code: refresh_material_per_view_bg / end_frame ===\n'
sed -n '8690,8785p' native/shared/src/renderer/mod.rs
printf '\n=== surrounding code: set_procedural_sky / bake_procedural_ibl / load_env_from_hdr ===\n'
for sym in set_procedural_sky bake_procedural_ibl load_env_from_hdr; do
line=$(rg -n "$sym" native/shared/src/renderer/mod.rs | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-25)); [ "$start" -lt 1 ] && start=1
end=$((line+120))
echo "--- $sym @ line $line ---"
sed -n "${start},${end}p" native/shared/src/renderer/mod.rs
fi
done
printf '\n=== find material-system bind group creation / use sites ===\n'
rg -n "per_view_bg|material_system\.per_view_bg|set_bind_group\(.*per_view|set_bind_group\(.*material" native/shared/src/renderer/mod.rsRepository: Bloom-Engine/engine
Length of output: 40401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the procedural sky transitions and the material draw path.
for sym in set_procedural_sky bake_procedural_ibl load_env_from_hdr submit_material_draw compile_material; do
line=$(rg -n "pub fn $sym|fn $sym" native/shared/src/renderer/mod.rs | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-35)); [ "$start" -lt 1 ] && start=1
end=$((line+220))
echo "=== $sym @ line $line ==="
sed -n "${start},${end}p" native/shared/src/renderer/mod.rs
echo
fi
done
printf '\n=== call sites for material per-view refresh and lighting bg updates ===\n'
rg -n "material_per_view_bg_live|refresh_material_per_view_bg|lighting_bg_is_procedural|lighting_bind_group =|make_lighting_bind_group" native/shared/src/renderer/mod.rsRepository: Bloom-Engine/engine
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm how MaterialSystem uses its per-view bind group.
fd -a "material_system.rs" native/shared/src/renderer || true
rg -n "per_view_bg|set_bind_group\(|per_view_buffer|layouts\.per_view|submit_draw|submit_draw_instanced" native/shared/src/renderer/material_system.rs native/shared/src/renderer/material_system -S
# Read only the relevant slices.
line=$(rg -n "per_view_bg|submit_draw|submit_draw_instanced" native/shared/src/renderer/material_system.rs | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-80)); [ "$start" -lt 1 ] && start=1
end=$((line+220))
sed -n "${start},${end}p" native/shared/src/renderer/material_system.rs
fiRepository: Bloom-Engine/engine
Length of output: 3896
Material per-view bind group needs a procedural-sky path
refresh_material_per_view_bg still binds self.sky_texture / self.env_diffuse_texture only, so material draws never pick up self.procedural_sky_equirect_full_view / self.procedural_env_diffuse_view. set_procedural_sky and bake_procedural_ibl also leave material_per_view_bg_live unchanged, so toggling procedural sky can leave the material path stuck on the panorama/default env while the scene pipeline uses the procedural IBL. Rebuild or invalidate this bind group when switching sky sources.
🤖 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 8717 - 8763, Update
refresh_material_per_view_bg to prefer procedural_sky_equirect_full_view and
procedural_env_diffuse_view when available, falling back to sky_texture and
env_diffuse_texture/default views otherwise. In set_procedural_sky and
bake_procedural_ibl, invalidate material_per_view_bg_live after changing or
baking procedural resources so the next end_frame rebuilds the bind group.
…n headroom - renderer/mod.rs crossed its ratcheting line-limit baseline (12175 > 11985): the cached-model draw commands and the immediate-mode mesh path (incl. the skin-pose plumbing) move to renderer/model_draw.rs, same split pattern as shadow_pass.rs. mod.rs is back to 11869 lines. - golden_many_point_lights_clustered_scene: the live env/BRDF wiring uniformly brightened material surfaces and nudged the documented Metal fp-divergence from ~6.0 to 6.24 mean (still 0 outliers). Per-test Metal headroom raised 6.0 -> 8.0; Linux/Windows stay under 2.0 and pass locally (7/7 goldens, 100/100 lib tests).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
native/shared/src/renderer/model_draw.rs (1)
16-43: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist loop-invariant matrix math out of the per-mesh loop.
model_matrix(Lines 24-28) andmodel_mvp(Line 28) don't depend onmesh_idx, so recomputing them for every primitive is wasted work. Notedraw_model_cached_rotatedalready hoistsmodel_matrixbut still recomputesmodel_mvpinside its loop (Line 85) — worth aligning both.♻️ Proposed hoist
+ // Compute model MVP once: VP * translate(position) * scale(s) + let model_matrix = mat4_multiply( + mat4_translate(IDENTITY_MAT4, position), + mat4_scale(IDENTITY_MAT4, [scale, scale, scale]), + ); + let model_mvp = mat4_multiply(self.current_vp_matrix, model_matrix); + for mesh_idx in 0..mesh_count { let slot = self.next_model_uniform_slot; self.next_model_uniform_slot += 1; // Grow uniform pool if needed self.ensure_model_uniform_slot(slot); - // Compute model MVP: VP * translate(position) * scale(s) - let model_matrix = mat4_multiply( - mat4_translate(IDENTITY_MAT4, position), - mat4_scale(IDENTITY_MAT4, [scale, scale, scale]), - ); - let model_mvp = mat4_multiply(self.current_vp_matrix, model_matrix); - // Write uniform for this draw self.queue.write_buffer(🤖 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/model_draw.rs` around lines 16 - 43, Hoist the loop-invariant matrix calculations out of the per-mesh loop in the model drawing method: compute model_matrix and model_mvp once before iterating over mesh_idx, then reuse them for uniform writes and CachedModelDraw entries. Apply the same optimization in draw_model_cached_rotated by moving model_mvp calculation outside its mesh loop while retaining the existing model_matrix hoist.
🤖 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.
Inline comments:
In `@native/shared/tests/golden_render.rs`:
- Around line 306-311: Scope the relaxed tolerance in compare_or_update_tol for
“many_point_lights_clustered_scene” to Metal only; retain the previous tolerance
on Linux and Windows. Use the platform-detection mechanism already present in
native/shared/tests/golden_render.rs and ensure non-Metal backends continue
enforcing the stricter regression threshold.
---
Nitpick comments:
In `@native/shared/src/renderer/model_draw.rs`:
- Around line 16-43: Hoist the loop-invariant matrix calculations out of the
per-mesh loop in the model drawing method: compute model_matrix and model_mvp
once before iterating over mesh_idx, then reuse them for uniform writes and
CachedModelDraw entries. Apply the same optimization in
draw_model_cached_rotated by moving model_mvp calculation outside its mesh loop
while retaining the existing model_matrix hoist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ff6dd03-649a-42d9-bfc7-62755ea7e1da
📒 Files selected for processing (3)
native/shared/src/renderer/mod.rsnative/shared/src/renderer/model_draw.rsnative/shared/tests/golden_render.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- native/shared/src/renderer/mod.rs
Comment on lines
+306
to
+311
| // 2026-07: wiring the material path's live env/BRDF resources | ||
| // (refresh_material_per_view_bg) uniformly brightened material | ||
| // surfaces and pushed Metal from ~6.0 to 6.24 (still 0 outliers, | ||
| // max 22) — headroom raised 6.0 → 8.0. Regenerate a Metal golden | ||
| // (BLOOM_UPDATE_GOLDEN=1 on macOS) to retire this override. | ||
| compare_or_update_tol("many_point_lights_clustered_scene", w, h, &rgba, 8.0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope the relaxed tolerance to Metal.
Line 311 applies 8.0 on every platform, although the rationale identifies only Metal as needing extra headroom and says Linux/Windows remain below 2.0. This weakens regression detection for non-Metal backends and can let clustered-lighting failures pass. Keep the previous tolerance for other platforms, or use platform-specific goldens.
Proposed fix
- compare_or_update_tol("many_point_lights_clustered_scene", w, h, &rgba, 8.0);
+ let mean_tol = if cfg!(target_os = "macos") { 8.0 } else { 6.0 };
+ compare_or_update_tol("many_point_lights_clustered_scene", w, h, &rgba, mean_tol);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 2026-07: wiring the material path's live env/BRDF resources | |
| // (refresh_material_per_view_bg) uniformly brightened material | |
| // surfaces and pushed Metal from ~6.0 to 6.24 (still 0 outliers, | |
| // max 22) — headroom raised 6.0 → 8.0. Regenerate a Metal golden | |
| // (BLOOM_UPDATE_GOLDEN=1 on macOS) to retire this override. | |
| compare_or_update_tol("many_point_lights_clustered_scene", w, h, &rgba, 8.0); | |
| // 2026-07: wiring the material path's live env/BRDF resources | |
| // (refresh_material_per_view_bg) uniformly brightened material | |
| // surfaces and pushed Metal from ~6.0 to 6.24 (still 0 outliers, | |
| // max 22) — headroom raised 6.0 → 8.0. Regenerate a Metal golden | |
| // (BLOOM_UPDATE_GOLDEN=1 on macOS) to retire this override. | |
| let mean_tol = if cfg!(target_os = "macos") { 8.0 } else { 6.0 }; | |
| compare_or_update_tol("many_point_lights_clustered_scene", w, h, &rgba, mean_tol); |
🤖 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/tests/golden_render.rs` around lines 306 - 311, Scope the
relaxed tolerance in compare_or_update_tol for
“many_point_lights_clustered_scene” to Metal only; retain the previous tolerance
on Linux and Windows. Use the platform-detection mechanism already present in
native/shared/tests/golden_render.rs and ensure non-Metal backends continue
enforcing the stricter regression threshold.
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
1 participant
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
Fixes the two remaining user-visible rendering bugs in the shooter, plus the round-5/6 engine work that was still uncommitted.
1. No shadows on anything, ever (material path sampled stub textures)
The material system's per-view bind group was built once at init with 1×1 stub textures in the env/BRDF/shadow slots — the "Phase 2 wires them from the existing renderer state" comment never got its Phase 2. wgpu zero-initializes textures, so
sample_sun_shadowread stored depth 0.0 at every texel: any in-frustum fragment on a material surface (terrain, grass, water — i.e. the ground everything stands on) compared as occluded, and the sun/shadow term was effectively dead scene-wide. Water reflections still showed tree shadows because the planar-probe pass builds a live bind group per frame — that asymmetry was the tell.Fix:
Renderer::refresh_material_per_view_bg()mirrors the probe's live group (real env panorama, BRDF LUT, and the three cascade depth views), installed on firstend_frameand re-armed wheneverload_env_from_hdrswaps the panorama.2. Two-primitive skinned models rendered as a giant blob glued to the player
bloom_draw_modelpopped one staged skin pose per skinned primitive, butupdateModelAnimationstages one per model. On 2-primitive GLBs (the shooter's marauder and tyrant) the second primitive found the FIFO empty, fell back to joint offset 0 — the player's pose block — and rendered as a huge mangled blob tracking the player. The FFI now pops once perdrawModel(take_staged_skin_offset()) and all primitives share the offset; the per-mesh entry points remain as compatibility wrappers.Also included (previously uncommitted round-5/6 work)
bloom_draw_model_rotated(cutout, material maps, cutout shadows for rotated statics).reads_sceneskip (wgpu validation panics).Verification
Batch-run F12 captures on the shooter (Windows, 4K): player and all five alien kinds cast posed shadows on grass; tree + building shadows across the terrain; marauder/tyrant/dragoon render correctly shaped and scaled at melee range. Shadow-term and stored-vs-ref depth visualizations on the terrain material confirmed cascade content and receiver projection agree after the fix.
Summary by CodeRabbit