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

fix(render): VM3 review fixes - pin the filmic decodes, re-derive vignette for linear, name the authored sun colour

Browse source 
Fix round from Opus review of 87677f9c (APPROVE WITH FIXES):

1. (A1/A3) Hardened the atmospheric_filmic.frag / atmospheric_bloom_
   downsample.frag shader-source pinning test: asserts the exact decode
   call count (6 - three in lowFusedScene, three in main's non-fused
   branch), that the 2.2 display-gamma exponent and 1/2.2 inverse in
   atmospheric_common.glsl are formatted FROM AtmosphericColorPipeline
   .DisplayGamma (so shader literal and CPU-tested value cannot drift),
   that the 0.18 contrast pivot in atmospheric_filmic.frag is formatted
   from AtmosphericColorPipeline.LinearMidGrey, and pins
   AtmosphericPostProcessGraph.BloomKneeLinear/BloomThresholdLinear as
   exact literals (0.73f / 1f). Also removed a stray trailing "()" from
   an existing comment in atmospheric_filmic.frag that was inflating the
   decode-call count to 7.

2. (B1) Re-derived the "vignette-strength" default for the linear-light
   post stack. The vignette multiply now happens on linear colour before
   the final encode, so a corner factor of (1 - strength) displays as
   (1 - strength)^(1/2.2), not (1 - strength) directly. The accepted
   look was strength 0.12 under the OLD gamma-space pipeline: a 12%
   on-screen corner darkening. Under strength 0.12 in the new linear
   pipeline that same 0.88 corner multiplier would only display as
   0.88^(1/2.2) ~= 0.9435 (5.6% darkening - visibly weaker). Solving
   (1 - strength)^(1/2.2) = 0.88 gives strength = 1 - 0.88^2.2 ~= 0.245,
   which reproduces the accepted 12% corner darkening. Since
   RenderPackSettingValueCodec requires every declared default to be
   step-aligned from the minimum and 0.245 is not a multiple of the old
   0.01 step, the step also moves to 0.005 (a finer slider, not
   coarser) so the exact derived default is a valid grid point -
   verified by running the ExternalTierTwoPackCanRenameEveryOwnedId
   AndShaderAsset validation test, which failed with "invalid default
   value" before this correction. Also updated the matching fallback in
   AtmosphericPostProcessGraph.FromDescriptor (0.12f -> 0.245f) for
   consistency, and added
   AtmosphericColorPipelineTests.VignetteDefaultReproducesTheAccepted
   TwelvePercentCornerDarkening pinning encode(1-0.245) ~= 0.88.

3. (A4) Renamed VolumetricShaftFrameParameters.LinearSunColor ->
   AuthoredSunColor in VolumetricShaftQuality.cs (internal, 2 references,
   both in that file - safe). Left LightSource.ColorLinear unrenamed:
   grep shows 13 files depend on it (GlobalLightPacker, SceneLightingUbo,
   LightBake, LightManager, EnvCellRenderer, RenderingDiagnostics, and
   several Core tests) across the shared retail default-path lighting
   UBO pipeline - renaming it is out of VM3's pack-only scope and would
   touch the mandatory-unchanged default path. Added a pointer comment
   on the field in LightSource.cs (and a one-line note at its
   WorldRenderFrameBuilder.cs call site) documenting the same
   display-space-not-linear fact and explaining why the rename is
   deferred to its own default-path colour-space pass.

4. (B4) Added a citation beside acesFitted in both atmospheric_filmic
   .frag and its C# mirror (AtmosphericColorPipeline.AcesFitted):
   Krzysztof Narkowicz, "ACES Filmic Tone Mapping Curve" (2016). The fit
   takes linear scene light in and returns linear display light in
   [0,1] - it does not itself gamma-encode. Evidence: acesFitted(0.80 *
   decode(0.46)) = 0.2064 un-encoded versus the accepted 0.51 on screen.

5. (B2) Rewrote the VM3 section of docs/plans/2026-08-22-visualmaster-
   campaign.md with the shipped truth in place of the pre-implementation
   guess: exposure stays 0.80 (at exposure 1.0 the linear pipeline maps
   gamma-0.5 to 0.6017, essentially the same 0.6163 the owner called too
   bright), bloom threshold stays 1.0 (a fixed point of both exponents),
   knee moves 0.45 -> 0.73, vignette-strength moves 0.12 -> 0.245. Added
   the old-vs-new curve table at exposure 0.80 across ten gamma inputs.
   Replaced the acceptance criteria's "new automated test on the
   recording RHI" with the CPU mirror + shader-source pins actually
   used, and recorded that the real-frame masked capture WAS run
   (retail vs High-with-every-effect-neutral, artifacts/vm3):
   independently re-verified by re-running the pixel diff against the
   checked-in screenshots - 110,561 px at |delta|=1 and exactly 95
   pixels at |delta|>=5, confined to foliage-canopy silhouette edges
   against sky with nothing on any ground/building/water surface. Noted
   the Stage-1 luminance table re-capture is still owed at the owner
   gate.

6. (B3) Corrected docs/plans/2026-08-21-atmospheric-rendering.md's VM3
   summary sentence: the bloom intermediate is already linear after
   extraction (no separate "bloom read" decode), and the neutral-preset
   claim is now phrased as a measured numerical identity (<=1 LSB on a
   real frame) rather than an unqualified "is" statement.

7. (A5) Corrected toolchain attribution: tools/compile-shaders.ps1 used
   the managed Silk.NET.Shaderc path (shaderc_shared.dll) to compile in
   both this round and the original VM3 commit - a Vulkan SDK glslc was
   detected and its path recorded, but the managed compiler is what
   actually ran. Regenerating this round only changed the atmospheric_
   filmic frag stage's manifest hash (comment-only edits); the compiled
   .spv bytes are unchanged, and every retail-oracle shader
   (mesh_modern, terrain_modern, mesh_detail, etc.) remains untouched.

8. Replaced an invented motive in the atmospheric_filmic.frag contrast-
   pivot comment ("rounded up for a stronger gamma-space contrast
   feel") with the actual reason: the previous 0.5 was simply the [0,1]
   midpoint of the standard contrast formula, not a deliberately chosen
   value; in linear the perceptual mid-grey is 0.18.

Verify: Release build 0 warnings / 0 errors. App hermetic-filter tests:
5970 passed / 0 failed / 0 skipped. VulkanShaderManifestTests: 7/7 pass
(retail-oracle SPIR-V byte-identical; only the atmospheric_filmic frag
manifest hash changed, no .spv bytes changed).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-22 23:40:48 +02:00
parent 87677f9c4f
commit 51178f7c77
12 changed files with 151 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

17
tests/AcDream.App.Tests/Rendering/Packs/AtmosphericColorPipelineTests.cs
View file

@ -140,4 +140,21 @@ public sealed class AtmosphericColorPipelineTests
Assert.Equal(AtmosphericPostProcessGraph.BloomKneeLinear, derivedKnee, 2);
Assert.Equal(1f, AtmosphericPostProcessGraph.BloomThresholdLinear);
}
[Fact]
public void VignetteDefaultReproducesTheAcceptedTwelvePercentCornerDarkening()
{
// BuiltInAtmosphericRenderPack's "vignette-strength" default is
// 0.245 = 1 - 0.88^2.2, chosen so the corner's linear multiplier
// (1 - strength) encodes back to the accepted 0.88 (a 12% on-screen
// darkening) instead of the weaker 0.88^(1/2.2) ~= 0.9435 a
// strength of 0.12 would now produce. See the "vignette-strength"
// comment in BuiltInAtmosphericRenderPack.cs for the full
// derivation.
const float VignetteStrengthDefault = 0.245f;
float cornerMultiplier = 1f - VignetteStrengthDefault;
Vector3 displayed = AtmosphericColorPipeline.Encode(
new Vector3(cornerMultiplier, cornerMultiplier, cornerMultiplier));
Assert.InRange(displayed.X, 0.88f - 0.005f, 0.88f + 0.005f);
}
}

21
tests/AcDream.App.Tests/Rendering/Packs/AtmosphericPostProcessGraphTests.cs
View file

@ -1,3 +1,4 @@
using System.Globalization;
using System.Numerics;
using System.Runtime.InteropServices;
using AcDream.App.Plugins;
@ -1180,6 +1181,7 @@ public sealed class AtmosphericPostProcessGraphTests
"AcDream.App",
"Rendering",
"Shaders");
string common = File.ReadAllText(Path.Combine(shaderRoot, "atmospheric_common.glsl"));
string filmic = File.ReadAllText(Path.Combine(shaderRoot, "atmospheric_filmic.frag"));
string downsample = File.ReadAllText(
Path.Combine(shaderRoot, "atmospheric_bloom_downsample.frag"));
@ -1193,10 +1195,29 @@ public sealed class AtmosphericPostProcessGraphTests
filmic,
StringComparison.Ordinal);
// Three in lowFusedScene (A/B/C) plus three in main's non-fused
// branch (A/C/D — sampleBloom's B is already linear).
Assert.Equal(6, CountOccurrences(filmic, "acdreamDecodeDisplay("));
int decodesInDownsample = CountOccurrences(downsample, "acdreamDecodeDisplay(");
Assert.True(
decodesInDownsample >= 3,
$"expected at least 3 acdreamDecodeDisplay( calls in atmospheric_bloom_downsample.frag, found {decodesInDownsample}");
// The contrast pivot and the decode/encode exponent are formatted
// from the C# mirror's own constants so the shader literal and the
// CPU-tested value cannot silently drift apart.
string gamma = AtmosphericColorPipeline.DisplayGamma.ToString(CultureInfo.InvariantCulture);
Assert.Contains($"vec3({gamma})", common, StringComparison.Ordinal);
Assert.Contains($"vec3(1.0 / {gamma})", common, StringComparison.Ordinal);
string pivot = AtmosphericColorPipeline.LinearMidGrey.ToString(CultureInfo.InvariantCulture);
Assert.Contains($"const float LinearMidGrey = {pivot}", filmic, StringComparison.Ordinal);
// The linear bloom threshold/knee the graph writes into Params0/2 are
// pinned by their exact literal values here too (AtmosphericColorPipelineTests
// separately proves they derive correctly from the pre-VM3 gamma pair).
Assert.Equal(0.73f, AtmosphericPostProcessGraph.BloomKneeLinear);
Assert.Equal(1f, AtmosphericPostProcessGraph.BloomThresholdLinear);
}
private static int CountOccurrences(string haystack, string needle)