Add solid leaf shadows in the shadow pass

[MichaelFisher1997]

What

Fixes the long-standing "Residual angle-dependent edge aliasing on direct sun shadows" item in ROADMAP.md (Shadows section).

Leaves are cutout blocks, so the shadow pass rasterized their silhouette by sampling the texture-array alpha. At shadow-map texel densities that alpha is mip-filtered, so the binary cutout edge dithered frame to frame as the sun and view angle changed — the canopy shadow edge visibly crawled.

world/block.gdshader now tests the engine built-in IN_SHADOW_PASS and, for leaves only, writes opaque coverage:

float leaf = step(0.99, COLOR.a);
float shadow_proxy = leaf * solid_leaf_shadows * (IN_SHADOW_PASS ? 1.0 : 0.0);
ALPHA = max(tex.a, shadow_proxy);

• The visible pass is unchanged: leaves keep their cutout silhouette.
• Leaves are the only blocks the mesher gives a full COLOR.a wind weight (ChunkMesher._build_light_tables), so COLOR.a doubles as the leaf marker. Glass and cross foliage are unaffected.
• The tradeoff is a player choice: Advanced Graphics → Shadows → "Solid Leaf Shadows" (default on, present in all three presets, applied in Main._apply_graphics()). Off restores the old dappled cutout shadows.

Why not alpha_hash

The earlier alpha_hash attempt (reverted) only re-dithered mip-filtered texels; base-mip leaf alpha is binary, so it added per-frame noise without stabilizing the silhouette. Changing the silhouette in the shadow pass attacks the root cause.

Verification

• redot --editor --headless --path . --quit — clean, 0 errors.

• tools/shadow_proxy_verify.gd (headless) — PASS. Pins the mesher's leaf wind marker (uniqueness across the block table), the name-derived leaf/FLAG_LEAVES set, the shader's IN_SHADOW_PASS / solid_leaf_shadows / ALPHA = max(tex.a, shadow_proxy) wiring, and that the new graphics key exists in every preset and has a UI row.

• tools/shadow_proxy_measure.gd (display) — flat-world canopy A/B, fixed camera, day/night clock frozen, 12 frames per phase at exact 0.1° sun steps driven through DayNightCycle.set_time(), close-up shadow-edge temporal MAD, off → off control → on, four runs:

  	off	off (control)	on
  no TAA	0.0060–0.0074	0.0060–0.0074	0.0037–0.0042
  TAA	0.0038–0.0040	0.0037–0.0039	0.0015–0.0033

  Without TAA the control tracks off every run and the proxy lands ~45% lower; with TAA the sampler already smooths the edge, so it gains little and the control is noisier. Close-up captures show the ragged speckled edge replaced by a clean straight one, with the visible-pass canopy top identical off and on.

• weather_verify.gd, ui_flow_verify.gd, ui_scale_verify.gd, frame_pacing_verify.gd — all PASS (the graphics toggle rides the existing settings flow; the wind wiring is unchanged).

Notes for reviewers

• An earlier revision of this tool drove the sun by writing _sun.rotation_degrees directly. That never rendered: DayNightCycle._process calls _apply() every frame and rewrites the rotation from time_hours. The tool now freezes auto_advance and uses set_time(), and the numbers above are from the corrected tool.
• IN_SHADOW_PASS is a spatial-shader built-in on all renderers (Forward+, mobile, GLES3); this project is Forward+.

[github-actions]

📋 Summary

Linked issues: The PR body references the open ROADMAP.md Shadows item ("Residual angle-dependent edge aliasing on direct sun shadows") rather than a GitHub issue (Fixes #N). The roadmap item explicitly called the shadow-only solid proxy "the untried option," and this PR implements exactly that and ticks the item [x] with full measurement provenance — not a silent completion.

This PR adds a shadow-pass-only leaf proxy to world/block.gdshader (12 lines): leaves — uniquely identified by the mesher's full COLOR.a wind weight — write opaque coverage when IN_SHADOW_PASS is true, so the shadow map rasterizes a solid geometric silhouette instead of mip-filtered binary alpha. It ships a headless wiring verifier, a display-only A/B measurement tool, and updates AGENTS.md/ROADMAP.md. The shipped-code change is minimal, well-isolated from the worker threads (mesher untouched), and correctly contracts with existing systems.

I verified the wiring against the full sources: _wind[id] = 1.0 only for FLAG_LEAVES (world/chunk_mesher.gd:425-430), all constants referenced by the verifier exist in world/block_registry.gd:10-50, _init builds the tables eagerly (world/chunk_mesher.gd:143-146) so ChunkMesher.new(blocks) headless works (precedent: tools/worldgen_cave_verify.gd:23), the registry material is the ShaderMaterial from res://world/block.gdshader (world/block_registry.gd:188,357-358), clouds use their own StandardMaterial3D (world/cloud_field.gd:32) so they cannot inherit the proxy, the measure scene's node paths match game/main.tscn (World/WorldEnvironment/Sun/Player), player.camera exists (player/player.gd:45), and DirAccess.make_dir_recursive_absolute("user://…") is the established pattern (game/shadow_capture.gd:30). No previous automated reviews exist, so there is nothing to re-check or acknowledge.

📌 Review Metadata

• Reviewed Commit SHA: d0ad7abfd12166e0250ee8c5b83e39d90a1a3623
• Reviewed PR: Add solid leaf shadows in the shadow pass #18

The one substantive finding is in the measurement tooling, not the shipped shader.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified. The shader change is correct: in the visible pass IN_SHADOW_PASS is false so shadow_proxy is 0 and ALPHA = tex.a is byte-identical to before; non-leaf cutout blocks (glass weight 0, cross foliage 0.6) fail step(0.99, COLOR.a) and are untouched; the vertex wind sway runs in the shadow pass too, so the solid silhouette tracks the swaying canopy instead of diverging from it; the uniform defaults to 1.0 on the single shared ShaderMaterial, so no per-chunk setup is needed. IN_SHADOW_PASS is a spatial-shader built-in in Redot's Godot 4.x base, and the author's local display run confirms it compiles and visibly changes the edge — the only residual risk is that CI cannot run the engine to re-confirm (parse check does not validate shaders), which the PR acknowledges.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] tools/shadow_proxy_measure.gd:127 - Manual sun steps are overwritten every frame by DayNightCycle, so the documented "0.1° sun steps" never render
Confidence: High
Description: DayNightCycle._process ends with an unconditional _apply() (world/day_night_cycle.gd:133-141), and _apply() sets _sun.rotation_degrees from time_hours every frame (world/day_night_cycle.gd:183). The measure coroutine sets _sun.rotation_degrees after frame_post_draw resumes, i.e. before the next frame's _process batch — so every captured frame shows the day-cycle-driven sun, never the manual stepped pose. auto_advance is true and day_length_seconds is 1200, so the actual sweep is ~0.3°/s (~0.005°/frame at 60 fps), roughly 20× slower than the claimed 0.1°/frame. It happens to frame correctly only because start_hour = 8.0 with sunrise at 6 yields elevation (8−6)/24·360 = 30°, matching SUN_ELEVATION_START. _sun.light_energy/light_color grading also drifts continuously during the run.
Impact: The comparative A/B conclusion still holds (all three phases share the same continuous sweep, and the control phase bounds run-to-run noise, which is why off→on dropped below both off runs), but the absolute MAD values and the "12 frames per phase at 0.1° sun steps" methodology recorded in ROADMAP.md:39 and the PR body do not describe what was actually rendered. Anyone re-running the tool with a different start_hour or day length, or tuning SUN_ELEVATION_STEP, will silently measure something else than intended.
Suggested Fix: Pin the cycle in _run() after acquiring _sun (note: auto_advance = false alone is insufficient because _apply() still runs every _process). Either stop the node entirely:

var day_night := _main.get_node("DayNight")
day_night.set_process(false)

or drive the hour per step instead of the rotation, which also freezes the energy/color grading consistently:

# in _measure(), replace the rotation assignment:
_day_night.set_time(SUNRISE_HOUR + (SUN_ELEVATION_START + index * SUN_ELEVATION_STEP) * 24.0 / 360.0)

Then re-record the ROADMAP numbers with the intended step size, or amend the methodology text to say the sweep is game-time driven.

ℹ️ Low Priority Suggestions (Optional)

[LOW] world/block.gdshader:13 - solid_leaf_shadows has no player-facing toggle
Confidence: High
Description: The dappled→solid canopy shadow tradeoff is real and the default flips the game's look, but the only way back is editing a shader parameter externally. The PR deliberately defers a setting ("Say the word if you'd rather expose it").
Impact: Players who preferred dappled shadows cannot restore them from Settings.
Suggested Fix: If desired later, wire it into Advanced Graphics → Shadows; per AGENTS.md, a new graphics key must then be added to every GRAPHICS_PRESETS entry (existing user configs pick up the default automatically).

[LOW] tools/shadow_proxy_verify.gd:75-82 - Hardcoded leaf family lets a future leaf block silently miss the proxy
Confidence: Medium
Description: _check_leaf_blocks_independent pins six named BLOCK_*_LEAVES constants, but _check_mesher_leaf_marker's scan only sees blocks that already carry FLAG_LEAVES. A newly added leaf block (e.g. cherry) that forgets the flag would pass both checks.
Impact: The regression pin decays as the block table grows.
Suggested Fix: Derive the expected set from the block table names instead of a constant list:

for id in blocks._names.size():
    if blocks.get_block_name(id).to_lower().contains("leaves"):
        _expect(blocks.has_flag(id, BlockRegistry.FLAG_LEAVES), "block %d ('%s') looks like a leaf but lacks FLAG_LEAVES" % [id, blocks.get_block_name(id)])

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9	Shader owns the visual proxy; tools are single-purpose; mesher and registry untouched
Open/Closed	8	Data-driven via existing COLOR.a marker + a uniform escape hatch; no mesher changes needed for the new behavior
Liskov Substitution	8	No inheritance introduced; contracts preserved (visible-pass output identical for non-leaf blocks)
Interface Segregation	8	Small focused scripts; measure and verify tools are cleanly split headless vs display
Dependency Inversion	7	The shader↔mesher COLOR.a convention is implicit cross-module coupling, but it is documented in AGENTS.md and pinned by shadow_proxy_verify.gd
Average	8.0

🎯 Final Assessment

Overall Confidence Score: 82%

Rate your confidence in this PR being ready to merge (0-100%).
How to interpret:

• 0-30%: Major concerns, do not merge without significant rework
• 31-60%: Moderate concerns, several issues need addressing
• 61-80%: Minor concerns, mostly ready with some fixes
• 81-100%: High confidence, ready to merge or with trivial fixes

Confidence Breakdown:

• Code Quality: 90% (minimal, well-commented shader delta; tools match repo conventions exactly — SceneTree _initialize pattern, .uid files, user:// output via make_dir_recursive_absolute)
• Completeness: 90% (implements precisely the roadmap's named option; docs updated on both AGENTS.md and ROADMAP.md; headless + display verification provided)
• Risk Level: 85% (no worker-thread, determinism, streaming, or water invariants touched; only residual runtime risk is that CI cannot compile the shader — locally verified per the PR)
• Verification: 70% (shadow_proxy_verify.gd should be run headless and weather_verify.gd re-run; neither can execute on this runner — treated as residual risk per policy. The display measurement's methodology has the Medium finding above, though its control-phase design keeps the conclusion sound)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• No unresolved worker-thread, determinism, or data-loss risk

Verdict:

MERGE WITH FIXES

The shader change is correct, well-contracted, and properly documented; fix the measurement tool's day-cycle override (or amend the recorded methodology) so the ROADMAP numbers describe what actually rendered, and run shadow_proxy_verify.gd locally before merging.

Machine Readable Verdict

{
  "reviewed_sha": "d0ad7abfd12166e0250ee8c5b83e39d90a1a3623",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 82,
  "recommendation": "MERGE_WITH_FIXES"
}

opencode session  |  github run

[MichaelFisher1997]

Review feedback addressed — 4b8037e

MEDIUM — measurement sun was overwritten by DayNightCycle (valid, fixed).
DayNightCycle._process calls _apply() every frame and rewrites _sun.rotation_degrees from time_hours, so the manual rotation writes never rendered — the actual sweep was the auto-advancing clock (~20× slower than claimed), and the recorded methodology was wrong. Fixed by freezing auto_advance and driving the sun through DayNightCycle.set_time() (_set_sun_elevation inverts the engine's elevation = (time_hours - SUNRISE_HOUR) / 24 * 360 mapping). Also:

• settle the sculpted chunks (commit queue) before sampling,
• flush TAA history for 0.5 s after each solid_leaf_shadows toggle, since TAA was bleeding the previous phase into the next,
• drop the now-unused SUN_YAW / _shadow_center param.

Re-ran the corrected A/B four times, control included:

	off	off (control)	on
no TAA	0.0060–0.0074	0.0060–0.0074	0.0037–0.0042
TAA	0.0038–0.0040	0.0037–0.0039	0.0015–0.0033

Without TAA the control tracks off every run and the proxy lands ~45% lower; with TAA the sampler already smooths the edge, so it gains little and the control is noisier. ROADMAP.md and the tool header now describe the real methodology and these numbers.

LOW — no player-facing toggle (addressed).
Added Advanced Graphics → Shadows → Solid Leaf Shadows (default on, key in all three GRAPHICS_PRESETS, applied in Main._apply_graphics() via the shared ShaderMaterial). Dappled shadows are restorable from Settings. shadow_proxy_verify.gd now asserts the key exists in every preset and has a section row.

LOW — hardcoded leaf list (addressed).
_check_leaf_blocks_independent now derives the expected set from BlockRegistry.get_block_name() (*leaves*) instead of six named constants, so a future leaf block that forgets FLAG_LEAVES fails the check. It also asserts at least six leaf blocks exist.

Local verification after the changes: parse check 0 errors; shadow_proxy_verify, weather_verify, ui_flow_verify, ui_scale_verify, frame_pacing_verify all PASS.

[github-actions]

All verification complete. Here is my review:

📋 Summary

Linked issues: No GitHub issues referenced (Fixes #N absent). The PR body targets the open ROADMAP.md Shadows item "Residual angle-dependent edge aliasing on direct sun shadows" and ticks it [x] with full measurement provenance — not a silent completion. The implementation matches what the roadmap item named as the untried option.

This commit (4b8037e, "Address shadow proxy review feedback") is a direct response to the previous automated review: it re-drives the measurement sun through DayNightCycle.set_time() with auto_advance frozen, exposes the dappled/solid tradeoff as an Advanced Graphics → Shadows "Solid Leaf Shadows" toggle wired through the standard preset/UI/Main._apply_graphics() pipeline, and makes the verifier's leaf-set check name-derived. I verified every fix against the full sources: the set_time() inversion is exact against _apply()'s elevation = (time_hours - SUNRISE_HOUR) / 24.0 * 360.0 (world/day_night_cycle.gd:149-151,182-183); the graphics key exists in all three presets and load_settings() (autoload/game_config.gd:505-515) grants old configs the preset default; the "check" row rides the identical soft_shadows flow (graphics_section_panel.gd:124-126 → Main._on_setting_changed("graphics") → _apply_graphics(), game/main.gd:348-353); the runtime GameConfig fetch in the verifier copies the proven frame_pacing_verify.gd:19-40 pattern; and world.get_registry() is valid whenever _apply_graphics() runs (children ready first; voxel_world.gd:115-116; the is ShaderMaterial guard covers the registry's StandardMaterial3D fallback, world/block_registry.gd:226,360,365). No worker-thread, determinism, streaming, or water surface was touched in either commit.

📌 Review Metadata

• Reviewed Commit SHA: 4b8037ebbe2b93359342da114640ff7d0fd36fc9
• Reviewed PR: Add solid leaf shadows in the shadow pass #18

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified. ✅ All previously reported critical issues have been resolved (none were reported).

⚠️ High Priority Issues (Should Fix)

None identified. ✅ All previously reported high priority issues have been resolved (none were reported).

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: manual sun steps in tools/shadow_proxy_measure.gd were overwritten every frame by DayNightCycle._process, so the documented "0.1° sun steps" never rendered. The fix takes the stronger of the two suggested options: auto_advance = false (tools/shadow_proxy_measure.gd:54) plus _set_sun_elevation() → _day_night.set_time(SUNRISE_HOUR + elevation * 24.0 / 360.0) (tools/shadow_proxy_measure.gd:112-113), which is the exact inverse of _apply()'s hour→elevation mapping (world/day_night_cycle.gd:182-183), so each captured frame renders the precise stepped pose and the sun energy/color grading follows the same pose instead of drifting. ROADMAP.md:39 was re-recorded with the corrected methodology and new four-run numbers (no-TAA control 0.0060-0.0074 vs proxy 0.0037-0.0042, ~45% reduction). Verified correct.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: solid_leaf_shadows had no player-facing toggle. Now exposed end-to-end: key in every GRAPHICS_PRESETS entry (autoload/game_config.gd:113,136,159 — satisfying the AGENTS.md all-presets rule, with load_settings()'s existing-key filter auto-migrating old configs), a "check" row in the SHADOWS section (ui/graphics_sections.gd:15) using the same row-type flow as soft_shadows, application in Main._apply_graphics() (game/main.gd:348-353) with null/fallback-material guards, verifier coverage (_check_graphics_toggle), and AGENTS.md documentation. Verified correct.

✅ [FIXED] Previous issue: hardcoded leaf family let a future leaf block silently miss FLAG_LEAVES. _check_leaf_blocks_independent (tools/shadow_proxy_verify.gd:100-115) now derives the expected set from block names via is_valid_id + get_block_name().to_lower().contains("leaves") — exactly the suggested fix — and additionally pins named_leaves >= 6 and glass/tall-grass exclusions. All helper methods exist (world/block_registry.gd:244,248,258) and constants BLOCK_GLASS/BLOCK_TALL_GRASS are real (block_registry.gd:12,39).

[LOW] PR description (Verification section) - Measurement table is stale relative to commit 2
Confidence: High
Description: The PR body still shows the first commit's numbers (off 0.0031 → control 0.0027 → on 0.0018) and the pre-fix methodology, which the second commit's own message states were measured with the buggy game-clock sweep (~20× slower than 0.1°/frame). The corrected numbers and methodology live only in ROADMAP.md:39 and the commit message.
Impact: Future readers quoting the PR body will cite numbers that do not describe what the fixed tool measures.
Suggested Fix: Update the PR description's Verification table to the re-recorded ROADMAP numbers. No code change needed.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9	Proxy isolated in the shader; toggle flows through the established graphics preset/UI/apply pipeline; tools remain single-purpose
Open/Closed	8	Behavior keyed off the existing COLOR.a data channel + uniform; new setting is data-driven through GRAPHICS_PRESETS/SECTIONS
Liskov Substitution	8	No inheritance introduced; visible-pass output and all existing signal/material contracts preserved
Interface Segregation	8	Headless verify vs display measure cleanly split; new check functions are focused
Dependency Inversion	7	The shader↔mesher COLOR.a convention is still implicit cross-module coupling, but it is documented in AGENTS.md, and the verifier now guards both sides (marker uniqueness + name-derived leaf set)
Average	8.0

🎯 Final Assessment

Overall Confidence Score: 88%

Rate your confidence in this PR being ready to merge (0-100%).
How to interpret:

• 0-30%: Major concerns, do not merge without significant rework
• 31-60%: Moderate concerns, several issues need addressing
• 61-80%: Minor concerns, mostly ready with some fixes
• 81-100%: High confidence, ready to merge or with trivial fixes

Confidence Breakdown:

• Code Quality: 92% (all three review findings addressed with the stronger fix option; toggle wiring matches repo conventions exactly, including the frame_pacing_verify.gd autoload-fetch precedent)
• Completeness: 95% (roadmap item implemented and ticked with provenance; docs, presets, UI, application, and regression pinning all present)
• Risk Level: 88% (main-thread-only shader parameter and settings plumbing; no worker, determinism, streaming, or water invariants touched; material guard covers the non-shader fallback)
• Verification: 75% (shadow_proxy_verify.gd should be re-run headless locally — its new _check_graphics_toggle postdates the PR body's "PASS" claim — and weather_verify.gd remains valid since commit 2 did not touch wind wiring; the engine binary is unavailable on this runner, which is residual risk per policy, not a blocker)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• No unresolved worker-thread, determinism, or data-loss risk

Verdict:

MERGE

All previously reported issues are fixed and verified against the full sources; the only remaining nit is the stale PR-description table, which affects no shipped code.

Machine Readable Verdict

{
  "reviewed_sha": "4b8037ebbe2b93359342da114640ff7d0fd36fc9",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 88,
  "recommendation": "MERGE"
}

opencode session  |  github run