erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

chore(sky): post-merge cleanups — CullFace save/restore + stale comments

Browse source 
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.
This commit is contained in:
Erik 2026-04-27 23:34:21 +02:00
parent f7c9e88b6a
commit e4bc6de7ba
2 changed files with 20 additions and 7 deletions
Download patch file Download diff file Expand all files Collapse all files

4
src/AcDream.App/Rendering/Shaders/sky.frag
View file

@ -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 ×

23
src/AcDream.App/Rendering/Sky/SkyRenderer.cs
View file

@ -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
/// <summary>
/// <c>Surface.Translucency</c> float (0..1) carried through from
/// <see cref="GfxObjSubMesh.SurfTranslucency"/>. Passed to the
/// sky fragment shader as <c>uSurfTranslucency</c>; the shader
/// multiplies output alpha by <c>(1 - x)</c>. 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 <c>curr_alpha</c> derivation in
/// <c>D3DPolyRender::SetSurface</c> at <c>0x59c767</c>.
/// sky fragment shader as <c>uSurfTranslucency</c> and used
/// DIRECTLY as opacity (NOT <c>1 - x</c>). Retail's
/// <c>D3DPolyRender::SetSurface</c> at <c>0x59c7a6</c>
/// (decomp lines 425255-425260) computes
/// <c>curr_alpha = _ftol2(translucency × 255)</c> 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.
/// </summary>
public float SurfTranslucency;
}