Opus dual-lens review of the three VM6 commits (0930c35d, 39e8408c,
6cc5e183) found two blockers and two should-fix issues; all landed here
along with the review's nits and documentation corrections.
Blockers:
- A1: the procedural-scenery classifier tested bit 31 alone instead of
the full top nibble (0xF000_0000 == 0x8000_0000), so it also matched
LandblockStaticEntityIdAllocator's 0xC... namespace (fences/gates/
building shells with a cutout subset), the 0xDA11_D0xx paperdoll id,
and the 0xFFFF_FF01 portal-tunnel id as procedural scenery — all
three would have swayed. ProceduralSceneryIdAllocator.IsInNamespace
now does the exact top-nibble test; FoliageWindClassification
delegates to it.
- A2: GroupKey (the receiver's instance-batching key) did not carry
FoliageFlags while the caster's dedup key already did, so a scenery
instance and a non-scenery instance sharing a mesh subset coalesced
into one receiver InstanceGroup whose flags were last-writer-wins —
disagreeing with the correctly-keyed caster. GroupKey now carries
FoliageFlags, computed before key construction and set exactly once
at group creation; the imperative re-stamp is gone, and CachedBatch's
now-redundant FoliageFlags field is removed.
Should-fix:
- A3: the world receiver pass bound UniformAtmosphericFrame only by
accident (leftover from the caster pass, which runs first each
frame, since Vulkan binding state isn't reset between passes).
DirectionalShadowFrameBinding now carries the caster's exact
AtmosphericFrameBufferBinding and BindDirectionalShadowReceiver binds
it explicitly.
- A4: a Setup-composed tree's opaque trunk part never got the trunk
flag because HasCutoutSubset is cached per GfxObj part, not per
entity. FoliageWindClassification.ComputeEntityHasCutoutSubset now
ORs HasCutoutSubset across an entity's resolved sibling parts once
per entity, threaded into ClassifyBatches/AddDirectionalShadowBatches
via a new optional override parameter.
Nits: A5 hashes the per-vertex flutter seed relative to the instance
origin instead of absolute world XY (fp32 sin() precision loss at far
landblock corners), mirrored in both foliage_wind.glsl and
FoliageWindModel; A7 documents the max(maxHeight, 0.5) divide-guard as
a deliberate pseudocode divergence; A8 switches FoliageWindExclusions'
construction to ToFrozenSet() and softens the "never stale" doc
comment to "no slower than one frame behind."
Tests added: top-nibble classification (0xFFFFFFFFu now correctly
false), GroupKey inequality across entity-driven scenery/landblock-
static classification, a caster-batch test proving the same pairing
never coalesces, ComputeEntityHasCutoutSubset unit + end-to-end
two-part-Setup tests, the caster→receiver AtmosphericFrame binding
carry-through, flutter-hash translation invariance relative to
instance origin, and a Storm-wind mid-height displacement floor
guarding against a "no motion" regression.
Docs: plan VM6 body corrected to the five-row WeatherKind table, "bits
1 and 2", "all four" caster shaders, and top-nibble wording throughout;
the owner gate checklist's Rain/Storm step; the stale v1-only shader-
interface compatibility entry; semantic-bindings-v1.md's v2 members
folded into the main 192-byte block; the IA-25 register row's top-
nibble wording; AtmosphericFrameInputs.cs's ABI size reference.
foliage_wind.glsl's A5 change recompiled exactly the five shaders that
include it (mesh_atmospheric.vert, the four directional_shadow_world_*
casters) plus the manifest; no other .spv changed.
Verify: Release build 0 warnings/0 errors. App hermetic-lane filter
6,041/0 failed (no environment-specific failures this run).
RenderPackValidator 30/30. Full hermetic-filtered solution: 15,269/0
failed across 15 projects.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AtmosphericFrame (set 3/binding 5) grows additively from 160 to 192 bytes:
two appended vec4 members, uAtmosphereClockWind and uAtmosphereWindAmplitude,
carry the foliage-wind clock/weather and amplitude inputs VM6b's shader
displacement will read. RenderPackShaderAbi renames the old constant to
AtmosphericFrameSizeBytesV1 (160), adds AtmosphericFrameSizeBytesV2 (192),
keeps AtmosphericFrameSizeBytes pointing at the current (v2) size, and adds
ShaderAbiVersion = 2. RenderPackSpirvValidator.ValidateAtmosphericFrame
accepts either the v1 (seven-member, 160-byte) or v2 (nine-member, 192-byte)
shape and rejects anything else naming both — this is why the frozen
external sample packs under samples/*/Shaders/*.spv, whose GLSL sources are
not in this tree, need no rebuild: a v1 shader bound to the 192-byte buffer
still reads correctly, since a bound range only needs to be >= the block's
own declared size.
DirectionalSunShadowRenderer's caster pass now binds AtmosphericFrame too
(both the multiview and per-cascade sites), through a new
AtmosphericFrameBufferBinding the graph owns and supplies via
DirectionalSunShadowRenderInput. AtmosphericPostProcessGraph.RenderDirectionalShadows
builds its own 192-byte ring allocation for this, separate from the world
receiver's frame block, because the caster pass runs before RenderPostProcess
constructs that block within the same frame. The four world caster pipeline
variants (opaque/cutout, base/multiview) are now allowed to declare binding
5 in the validator; terrain casters are untouched.
This commit is plumbing only: the two new members are always written but
never read by any shader yet (zero placeholders), so pack-on and pack-off
output are both pixel-identical to before. VM6b wires the real weather-driven
values and the shader-side displacement.
App hermetic filter: 5972/5974 (2 pre-existing failures unrelated to this
change, confirmed against the unmodified baseline). Core.Tests hermetic:
4697/4697. RenderPackValidator.Tests: 30/30. VulkanShaderManifestTests
(retail oracle set): 7/7, byte-identical.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closes review finding F4 (docs/research/2026-08-22-campaign-ar-review.md):
retail's main-world colour, sun rays and volumetric shafts are all
gamma-encoded display-space values (the 2013 client has no linear
lighting pipeline), but bloom thresholding, ACES (Narkowicz fit),
Rec.709 luma saturation, the contrast pivot and the vignette were all
operating directly on those gamma values, then writing the result to
the UNORM swapchain without re-encoding.
- atmospheric_common.glsl gains acdreamDecodeDisplay/acdreamEncodeDisplay
(pow(c, 2.2) / pow(c, 1/2.2)). 2.2 is the retail-era CRT/early-LCD
display-gamma assumption, deliberately not the sRGB piecewise curve,
which would claim a precision retail's authoring pipeline never had.
uAtmosphereSunColor's comment is corrected from "authored linear rgb"
to "authored display-space rgb (retail has no linear pipeline)".
- atmospheric_bloom_downsample.frag, atmospheric_filmic.frag (both the
fused-Low and non-fused paths) decode every world/ray/volumetric read
before summing/thresholding; atmospheric_bloom_blur.frag is unchanged
(it already reads the now-linear bloom buffer); atmospheric_sun_rays.frag
and atmospheric_volumetric.frag are documented as writing display-space
colour that the consumers decode.
- The contrast pivot moves from 0.5 (a gamma-space midpoint) to 0.18
(linear mid-grey, the standard 18%-grey-card exposure convention).
The final filmic output is clamped in linear, then re-encoded before
the UNORM write.
- Bloom threshold/knee are re-derived for linear light: the pre-VM3
gamma-space pair was threshold 1.0 / knee 0.45, i.e. a soft range of
[0.55, 1.0] in gamma. Decoding both ends with the same 2.2 assumption
gives decode(1.0) = 1.0 (threshold unchanged) and
decode(0.55) = 0.55^2.2 ~= 0.27, so linear knee = 1.0 - 0.27 ~= 0.73.
Replaced the inline 0.45f literals with named constants
BloomThresholdLinear = 1f / BloomKneeLinear = 0.73f on
AtmosphericPostProcessGraph. bloom-strength's 0.65 default is
untouched.
- Exposure stays at its accepted 0.80 default: in linear,
encode(acesFitted(0.80 * decode(0.46))) ~= 0.50, reproducing the same
accepted midtone the old gamma-space pipeline produced as 0.51 for the
same 0.46 input (0.46 * 0.80 fed straight into acesFitted, no
decode/encode). Highlights now retain more (gamma 0.9 input moves from
~0.74 to ~0.85 through the full pipeline) and blacks deepen slightly
(gamma 0.1 moves from ~0.09 to ~0.05) — the owner's visual gate judges.
- Added AtmosphericColorPipeline, a CPU mirror of the GLSL decode/encode/
ACES/grade/filmic math (line-for-line, with a header comment requiring
it stay mirrored), and AtmosphericColorPipelineTests: neutral-preset
identity within half an 8-bit step for a 0..255 grey sweep (proving the
neutral preset is numerically the pack-off image), decode/encode
round-trip within 1e-6, monotonic-in-exposure, the pinned midtone/
highlight/shadow numbers above, and the bloom-knee derivation.
- Added a shader-source pinning test so a future edit cannot silently
drop the colour-space conversions: atmospheric_filmic.frag must
contain exactly one acdreamEncodeDisplay( call in main()'s output,
atmospheric_bloom_downsample.frag must contain at least three
acdreamDecodeDisplay( calls.
- Regenerated SPIR-V (tools/compile-shaders.ps1, glslc from the
installed Vulkan SDK). Only atmospheric_bloom_downsample.frag.spv and
atmospheric_filmic.frag.spv changed in bytes; every other pack shader
that includes atmospheric_common.glsl recompiled to a byte-identical
binary (the new decode/encode helpers are unreferenced dead code for
them). VulkanShaderManifestTests' retail-oracle SHA-256 set
(mesh_modern, terrain_modern, mesh_detail, etc.) is untouched and
still passes — the retail default path did not change.
- Docs: noted the linear-light move in the AR plan's Slice 1 section,
and added a "Colour space" section to the render-pack ABI doc
(docs/render-packs/semantic-bindings-v1.md) naming which inputs are
display-space and pointing at atmospheric_common.glsl as the
reference implementation. No ABI version bump — the binding layout
is unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik