From e4bc6de7babeaf2bf96e2aff96f28b82be145fb0 Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 27 Apr 2026 23:34:21 +0200 Subject: [PATCH] =?UTF-8?q?chore(sky):=20post-merge=20cleanups=20=E2=80=94?= =?UTF-8?q?=20CullFace=20save/restore=20+=20stale=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small hygiene items flagged by external code-review reports during the sky/weather investigation: 1. CullFace state leak in SkyRenderer.RenderPass. Disabled CullFace at the start of the sky pass without restoring it on exit. Benign today — the global convention in this codebase is CullFace=off and subsequent renderers (InstancedMeshRenderer, StaticMeshRenderer) explicitly enable on entry / disable on exit — but a future caller assuming culling stays on across the sky pass would have silently broken. Wrap with an IsEnabled save / Enable restore using TextRenderer.cs's pattern. 2. Stale comment in SubMeshGpu.SurfTranslucency doc. Said "the shader multiplies output alpha by (1 - x)". After commit 97fc1b5 the shader uses translucency DIRECTLY as opacity per retail D3DPolyRender::SetSurface at 0x59c7a6 (decomp 425255-425260). Updated to reflect the current formula. 3. Stale comment in sky.frag header. Said "fragment.a = texture.a × (1 - uTransparency) × (1 - uSurfTranslucency)". Updated to "× uSurfTranslucency" with citation. Not addressed: Report 2's "uLuminosity declared but never referenced" claim. Verified false — the uniform was already removed; the only remaining uLuminosity references are in comments documenting the historical removal (sky.frag header line 13-14 explicitly says "removed 2026-04-26"). Report 2 was reading stale content. 1314 tests pass. --- src/AcDream.App/Rendering/Shaders/sky.frag | 4 +++- src/AcDream.App/Rendering/Sky/SkyRenderer.cs | 23 +++++++++++++++----- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/AcDream.App/Rendering/Shaders/sky.frag b/src/AcDream.App/Rendering/Shaders/sky.frag index e492f6e..c704467 100644 --- a/src/AcDream.App/Rendering/Shaders/sky.frag +++ b/src/AcDream.App/Rendering/Shaders/sky.frag @@ -3,7 +3,9 @@ // D3D fixed-function: // // fragment.rgb = texture.rgb × vTint + lightning_flash -// fragment.a = texture.a × (1 - uTransparency) × (1 - uSurfTranslucency) +// fragment.a = texture.a × (1 - uTransparency) × uSurfTranslucency +// (uSurfTranslucency is OPACITY directly per retail's +// D3DPolyRender::SetSurface at 0x59c7a6, NOT 1-x) // // vTint arrives from the vertex shader with retail's per-vertex // lighting formula baked in (Emissive + lightAmbient + lightDiffuse × diff --git a/src/AcDream.App/Rendering/Sky/SkyRenderer.cs b/src/AcDream.App/Rendering/Sky/SkyRenderer.cs index 1aa9550..c593950 100644 --- a/src/AcDream.App/Rendering/Sky/SkyRenderer.cs +++ b/src/AcDream.App/Rendering/Sky/SkyRenderer.cs @@ -186,6 +186,14 @@ public sealed unsafe class SkyRenderer : IDisposable // Save + override GL state. _gl.DepthMask(false); _gl.Disable(EnableCap.DepthTest); + // Save + disable CullFace for the sky pass; restore at the end. + // Mirrors TextRenderer.cs's save/restore pattern. Without this the + // sky pass left CullFace disabled regardless of its prior state, + // which is benign today (the global convention in this codebase is + // off and subsequent renderers manage their own CullFace) but + // would break the moment any future caller assumes back-face + // culling stays on across the sky pass. + bool wasCullFace = _gl.IsEnabled(EnableCap.CullFace); _gl.Disable(EnableCap.CullFace); _gl.Enable(EnableCap.Blend); // Default blend — overridden per-submesh inside the inner loop. @@ -406,6 +414,7 @@ public sealed unsafe class SkyRenderer : IDisposable _gl.Disable(EnableCap.Blend); _gl.DepthMask(true); _gl.Enable(EnableCap.DepthTest); + if (wasCullFace) _gl.Enable(EnableCap.CullFace); _gl.BindVertexArray(0); } @@ -737,12 +746,14 @@ public sealed unsafe class SkyRenderer : IDisposable /// /// Surface.Translucency float (0..1) carried through from /// . Passed to the - /// sky fragment shader as uSurfTranslucency; the shader - /// multiplies output alpha by (1 - x). For the rain - /// surface 0x080000C5 this is 0.5 → opacity 0.5 → rain streaks - /// contribute at half intensity under the additive blend, matching - /// retail's curr_alpha derivation in - /// D3DPolyRender::SetSurface at 0x59c767. + /// sky fragment shader as uSurfTranslucency and used + /// DIRECTLY as opacity (NOT 1 - x). Retail's + /// D3DPolyRender::SetSurface at 0x59c7a6 + /// (decomp lines 425255-425260) computes + /// curr_alpha = _ftol2(translucency × 255) and writes that + /// as vertex.color.alpha — i.e. translucency is opacity directly. + /// For non-Translucent surfaces the GfxObjMesh.Build() path keeps + /// this at 1.0 so they stay fully opaque. /// public float SurfTranslucency; }