From f7344758f8b67a57526b103841c87edb06ad856c Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 28 Jul 2026 13:31:13 +0200 Subject: [PATCH] =?UTF-8?q?fix(render):=20Campaign=20V=20slice=20V6i-2=20c?= =?UTF-8?q?ommit=201=20=E2=80=94=20the=20terrain=20clip=20block=20reaches?= =?UTF-8?q?=20set=201?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plan §5.5.12 finding 2, measured on the committed SPIR-V rather than inferred: terrain_modern.vert declared layout(std140, binding = 2) uniform TerrainClip { ... } with no ACDREAM_UBO_SET, so under the Vulkan dialect the block landed in set 0 binding 2 — which set 0's layout declares as a STORAGE buffer. Any terrain pipeline built against the shared pipeline layout was therefore malformed. Nothing had caught it: GL expands the macro to nothing and keeps its UBO and SSBO namespaces separate, the shader compiled cleanly for both backends, and no terrain pipeline has ever been created on Vulkan. sky.vert declares the SAME block correctly and is the precedent, so this is a one-word omission, not a numbering question. spirv-dis on spv/terrain_modern.vert.spv, before and after: before %372 = OpVariable %_ptr_Uniform__struct_370 Uniform OpDecorate %372 DescriptorSet 0 / Binding 2 after OpDecorate %372 DescriptorSet 1 / Binding 2 with %_struct_370 = OpTypeStruct %int %_arr_v4float_uint_8 — TerrainClip's { int uTerrainClipCount; vec4 uTerrainClipPlanes[8]; } — in both. The same commit closes §5.5.8's second recorded gap. Set 1's layout declared only bindings 1 and 3, so it was missing BOTH the terrain clip block and UniformSkyParams at binding 4, which sky.vert and sky.frag have compiled to SPIR-V since V6e. Both are now declared, all four dynamic, which is half Vulkan's guaranteed maxDescriptorSetUniformBuffersDynamic of 8 and is asserted by the capability gate as before. Membership and ORDER now come from one predicate — IsDeclaredUniformBinding — that the layout, the descriptor writes and vkCmdBindDescriptorSets's dynamic-offset array are all built from, the same shape V6g gave set 0. The three had been restated separately, which is exactly how a fifth binding would have gone wrong the same way. Both gaps were found by hand, months apart, and neither could fail on the shipping backend. VulkanShaderDescriptorContractTests reads the committed .spv and asserts the partition instead: every uniform block at a declared set-1 binding, every storage block inside set 0's declared range, every sampled resource in the one texture table. Checked out against the pre-fix .spv, two of its four tests fail. Gates: Release build; App tests 4,090 / 3 skips (4,086 baseline plus four); strict GL offline pixel gate vs 0ca802cd 3.02e-05 (17 px of 563,200, inside the documented 9–31 px control band, 33x under threshold) — expected, since GL executes not one changed statement; one Vulkan composition-host run with VK_LAYER_KHRONOS_validation proven inserted by the loader at zero errors, zero warnings and no [shutdown] diagnostic on either stream. Co-Authored-By: Claude Fable 5 --- .../Rendering/Gpu/Vk/VulkanFrameBindings.cs | 42 ++-- .../Rendering/Gpu/Vk/VulkanPipelineLayouts.cs | 96 ++++++-- .../Shaders/spv/shaders.manifest.json | 2 +- .../Shaders/spv/terrain_modern.vert.spv | Bin 9824 -> 9824 bytes .../Rendering/Shaders/terrain_modern.vert | 8 +- .../Vk/VulkanShaderDescriptorContractTests.cs | 223 ++++++++++++++++++ 6 files changed, 335 insertions(+), 36 deletions(-) create mode 100644 tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanShaderDescriptorContractTests.cs diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs index 12638d35..f1f6fbea 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs @@ -75,15 +75,22 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable return [.. bindings]; } - /// Bindings 0..3 of set 1; only 1 (SceneLighting) and 3 (terrain tiling) are used. - internal const int UniformBindingCount = 4; + /// + /// Bindings 0..4 of set 1. Slice V6i-2 raised this from 4 when the layout + /// gained binding 4 (sky params); binding 0 remains unused and is counted + /// only so the bookkeeping arrays stay index-aligned with the binding number. + /// Which of them the layout DECLARES is + /// . + /// + internal const int UniformBindingCount = 5; /// /// How many of set 1's bindings the layout actually declares, all dynamic. /// Asserted against maxDescriptorSetUniformBuffersDynamic by the /// capability gate; Vulkan guarantees 8, so this is comfortable. /// - internal const uint DynamicUniformBindingCount = 2; + internal static uint DynamicUniformBindingCount { get; } = + (uint)VulkanPipelineLayouts.DeclaredUniformBindings.Length; /// /// Widest range any single binding may address. Dynamic descriptors take a @@ -179,14 +186,15 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable sets[1] = _sets[slot].Uniform; sets[2] = device.TextureTable.Set; - int dynamicCount = DynamicStorageBindings.Length + 2; + uint[] declaredUniforms = VulkanPipelineLayouts.DeclaredUniformBindings; + int dynamicCount = DynamicStorageBindings.Length + declaredUniforms.Length; uint* offsets = stackalloc uint[dynamicCount]; // Dynamic offsets are ordered by set, then by binding number, and only // the DYNAMIC descriptors have a slot at all. for (int i = 0; i < DynamicStorageBindings.Length; i++) offsets[i] = _arena.StorageOffset(DynamicStorageBindings[i]); - offsets[DynamicStorageBindings.Length + 0] = _arena.UniformOffset(GpuBindingModel.UniformSceneLighting); - offsets[DynamicStorageBindings.Length + 1] = _arena.UniformOffset(GpuBindingModel.UniformTerrainTiling); + for (int i = 0; i < declaredUniforms.Length; i++) + offsets[DynamicStorageBindings.Length + i] = _arena.UniformOffset(declaredUniforms[i]); _vk.CmdBindDescriptorSets( commands, @@ -212,18 +220,16 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable VulkanPipelineLayouts.IsDynamicStorageBinding(binding)); } - // Only the two bindings the layout declares exist; the rest of the array - // is bookkeeping so the offsets stay index-aligned. - WriteUniform( - pair.Uniform, - GpuBindingModel.UniformSceneLighting, - new Silk.NET.Vulkan.Buffer(_arena.UniformBuffer(GpuBindingModel.UniformSceneLighting)), - _arena.UniformRange(GpuBindingModel.UniformSceneLighting)); - WriteUniform( - pair.Uniform, - GpuBindingModel.UniformTerrainTiling, - new Silk.NET.Vulkan.Buffer(_arena.UniformBuffer(GpuBindingModel.UniformTerrainTiling)), - _arena.UniformRange(GpuBindingModel.UniformTerrainTiling)); + // Only the bindings the layout declares exist; the rest of the array is + // bookkeeping so the offsets stay index-aligned with the binding number. + foreach (uint binding in VulkanPipelineLayouts.DeclaredUniformBindings) + { + WriteUniform( + pair.Uniform, + binding, + new Silk.NET.Vulkan.Buffer(_arena.UniformBuffer(binding)), + _arena.UniformRange(binding)); + } } private (DescriptorSet Storage, DescriptorSet Uniform) AllocatePair() diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanPipelineLayouts.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanPipelineLayouts.cs index a4ac876f..40c0c62c 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanPipelineLayouts.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanPipelineLayouts.cs @@ -184,29 +184,93 @@ internal static unsafe class VulkanPipelineLayouts return layout; } - /// Set 1 — the SceneLighting and terrain-tiling uniform blocks. + /// + /// The terrain screen-space clip block's uniform binding. + /// + /// does not name this number — it only + /// records, twice, that "binding 2 is taken by the terrain clip block" while + /// explaining why terrain tiling is 3 and sky params are 4. The number itself + /// has been pinned by terrain_modern.vert and sky.vert since + /// Phase U.3. Restating it here rather than promoting it into the frozen + /// binding model keeps slice V6i-2 out of the pinned contract; a later slice + /// entitled to change §3.3 should move it. + /// + internal const uint UniformTerrainClip = 2; + + /// + /// Which of set 1's bindings the layout declares — the uniform-side twin of + /// , and for the same reason: the + /// layout, the descriptor writes and vkCmdBindDescriptorSets's + /// dynamic-offset array must agree on both membership and ORDER, and getting + /// either wrong is a validation error. Deriving all three from one predicate + /// is what stops them drifting. + /// + /// Binding 0 is unused — + /// is 1 so that GL's separate UBO/SSBO namespaces keep their existing numbers. + /// It is still counted by + /// so the bookkeeping arrays stay index-aligned with the binding number. + /// + /// Slice V6i-2 added 2 (terrain clip) and 4 (sky params). Both blocks + /// have compiled to SPIR-V declaring set 1 for some time — plan §5.5.8 + /// recorded the sky gap and §5.5.12 measured the terrain one — but neither + /// binding existed in the layout, so a pipeline built for either shader was + /// malformed. Nothing caught it because no terrain or sky pipeline has ever + /// been created on Vulkan. + /// + internal static bool IsDeclaredUniformBinding(uint binding) => binding switch + { + GpuBindingModel.UniformSceneLighting => true, + UniformTerrainClip => true, + GpuBindingModel.UniformTerrainTiling => true, + GpuBindingModel.UniformSkyParams => true, + _ => false, + }; + + /// + /// Set 1's declared bindings in ascending order — the order + /// vkCmdBindDescriptorSets requires its dynamic offsets in. + /// + internal static uint[] DeclaredUniformBindings { get; } = BuildDeclaredUniformBindings(); + + private static uint[] BuildDeclaredUniformBindings() + { + var bindings = new List(VulkanFrameBindings.UniformBindingCount); + for (uint binding = 0; binding < VulkanFrameBindings.UniformBindingCount; binding++) + { + if (IsDeclaredUniformBinding(binding)) + bindings.Add(binding); + } + + return [.. bindings]; + } + + /// + /// Set 1 — the SceneLighting, terrain-clip, terrain-tiling and sky-params + /// uniform blocks. All four are dynamic: each is fed from the per-frame ring, + /// so its offset moves every frame and a dynamic descriptor is exactly what + /// spares the write. Four is half Vulkan's guaranteed + /// maxDescriptorSetUniformBuffersDynamic of 8, and the capability gate + /// asserts it. + /// internal static DescriptorSetLayout CreateUniformSetLayout(Silk.NET.Vulkan.Vk vk, Device device) { - DescriptorSetLayoutBinding* bindings = stackalloc DescriptorSetLayoutBinding[2]; - bindings[0] = new DescriptorSetLayoutBinding + uint[] declared = DeclaredUniformBindings; + DescriptorSetLayoutBinding* bindings = stackalloc DescriptorSetLayoutBinding[declared.Length]; + for (int i = 0; i < declared.Length; i++) { - Binding = GpuBindingModel.UniformSceneLighting, - DescriptorType = DescriptorType.UniformBufferDynamic, - DescriptorCount = 1, - StageFlags = ShaderStageFlags.VertexBit | ShaderStageFlags.FragmentBit, - }; - bindings[1] = new DescriptorSetLayoutBinding - { - Binding = GpuBindingModel.UniformTerrainTiling, - DescriptorType = DescriptorType.UniformBufferDynamic, - DescriptorCount = 1, - StageFlags = ShaderStageFlags.VertexBit | ShaderStageFlags.FragmentBit, - }; + bindings[i] = new DescriptorSetLayoutBinding + { + Binding = declared[i], + DescriptorType = DescriptorType.UniformBufferDynamic, + DescriptorCount = 1, + StageFlags = ShaderStageFlags.VertexBit | ShaderStageFlags.FragmentBit, + }; + } var create = new DescriptorSetLayoutCreateInfo { SType = StructureType.DescriptorSetLayoutCreateInfo, - BindingCount = 2, + BindingCount = (uint)declared.Length, PBindings = bindings, }; VulkanInterop.Check( diff --git a/src/AcDream.App/Rendering/Shaders/spv/shaders.manifest.json b/src/AcDream.App/Rendering/Shaders/spv/shaders.manifest.json index f6a65f17..6ca80364 100644 --- a/src/AcDream.App/Rendering/Shaders/spv/shaders.manifest.json +++ b/src/AcDream.App/Rendering/Shaders/spv/shaders.manifest.json @@ -105,7 +105,7 @@ "stages": [ { "stage": "vert", - "sourceSha256": "336880b293e95c9ba13dce7a616a15e941191afec8bdaabe04a4993182eb6811", + "sourceSha256": "a3f8592d482793622f7a69c08f8ba828a4e071f2fb5e1763f865be84797bceb7", "compiled": true }, { diff --git a/src/AcDream.App/Rendering/Shaders/spv/terrain_modern.vert.spv b/src/AcDream.App/Rendering/Shaders/spv/terrain_modern.vert.spv index 6c4f766ee9149938c312cf968b009145422e1724..bbf61fadf9cbd6320b6199294f6814dd6acd69ef 100644 GIT binary patch delta 14 VcmaFh^T20=3JW9SW>pqzB>*b+1Z)5R delta 14 VcmaFh^T20=3JW8{W>pqzB>*b$1Zw~Q diff --git a/src/AcDream.App/Rendering/Shaders/terrain_modern.vert b/src/AcDream.App/Rendering/Shaders/terrain_modern.vert index 254b948e..dbf24f40 100644 --- a/src/AcDream.App/Rendering/Shaders/terrain_modern.vert +++ b/src/AcDream.App/Rendering/Shaders/terrain_modern.vert @@ -47,7 +47,13 @@ layout(std140, ACDREAM_UBO_SET binding = 1) uniform SceneLighting { // UBO, leaving binding=2 free. uTerrainClipCount == 0 (the U.3 default) ungates // terrain entirely (the second loop sets all 8 distances to +1.0). Uploaded by // ClipFrame.UploadShared each frame; TerrainModernRenderer binds it before draw. -layout(std140, binding = 2) uniform TerrainClip { +// +// Campaign V slice V6i-2: ACDREAM_UBO_SET is what puts this in set 1 under the +// Vulkan dialect and expands to nothing under GL. Omitting it left the block at +// set 0 binding 2, which the storage layout declares as a STORAGE buffer — see +// plan §5.5.12 finding 2, measured on the committed SPIR-V rather than inferred. +// sky.vert declares the SAME block correctly and is the precedent. +layout(std140, ACDREAM_UBO_SET binding = 2) uniform TerrainClip { int uTerrainClipCount; vec4 uTerrainClipPlanes[8]; }; diff --git a/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanShaderDescriptorContractTests.cs b/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanShaderDescriptorContractTests.cs new file mode 100644 index 00000000..53d3c5c0 --- /dev/null +++ b/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanShaderDescriptorContractTests.cs @@ -0,0 +1,223 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using AcDream.App.Rendering.Gpu; +using AcDream.App.Rendering.Gpu.Vk; + +namespace AcDream.App.Tests.Rendering.Gpu.Vk; + +/// +/// Campaign V slice V6i-2: every compiled SPIR-V module's descriptor decorations +/// must match the descriptor set layouts the backend creates. +/// +/// Why this test exists. Slice V6i measured that +/// terrain_modern.vert's TerrainClip block was declared +/// layout(std140, binding = 2) with no ACDREAM_UBO_SET, so under +/// the Vulkan dialect it landed in set 0 binding 2 — which set 0's layout +/// declares as a STORAGE buffer. A terrain pipeline built against the shared +/// layout was therefore malformed. Nothing caught it: GL expands the macro to +/// nothing and keeps its UBO and SSBO namespaces separate, the shader compiled +/// cleanly for both backends, and no terrain pipeline had ever been created on +/// Vulkan. Plan §5.5.8 recorded a second instance of the same class — sky's +/// UniformSkyParams at binding 4 was absent from the layout — found the +/// same way, by hand, months apart. +/// +/// A one-word omission in a shader that cannot fail to compile, cannot +/// fail on the shipping backend, and is only wrong on a backend nothing has run +/// yet, is exactly the shape of defect that needs a test rather than an audit. +/// Reading the committed .spv is what makes the assertion about what the +/// GPU will actually be handed, not about what the GLSL looks like. +/// +public sealed class VulkanShaderDescriptorContractTests +{ + private static string RepositoryRoot() + { + var directory = new DirectoryInfo(AppContext.BaseDirectory); + while (directory is not null && !File.Exists(Path.Combine(directory.FullName, "AcDream.slnx"))) + directory = directory.Parent; + return directory?.FullName + ?? throw new InvalidOperationException("Could not locate the repository root from the test binary."); + } + + private static string SpirvDirectory() => + Path.Combine(RepositoryRoot(), "src", "AcDream.App", "Rendering", "Shaders", "spv"); + + /// One interface variable's set, binding and storage class. + private readonly record struct ShaderResource( + string Module, + uint Id, + SpirvStorageClass StorageClass, + uint? Set, + uint? Binding); + + private enum SpirvStorageClass : uint + { + UniformConstant = 0, + Uniform = 2, + StorageBuffer = 12, + } + + /// + /// The smallest SPIR-V reader that can answer this question: walk the + /// instruction stream collecting OpDecorate DescriptorSet/Binding and + /// the storage class of every OpVariable. No type or name recovery is + /// needed, so no dependency is either. + /// + private static IReadOnlyList ReadResources(string path) + { + byte[] bytes = File.ReadAllBytes(path); + Assert.True(bytes.Length >= 20 && bytes.Length % 4 == 0, $"{path} is not a SPIR-V module."); + var words = new uint[bytes.Length / 4]; + Buffer.BlockCopy(bytes, 0, words, 0, bytes.Length); + Assert.Equal(0x07230203u, words[0]); + + const uint OpDecorate = 71; + const uint OpVariable = 59; + const uint DecorationBinding = 33; + const uint DecorationDescriptorSet = 34; + + var sets = new Dictionary(); + var bindings = new Dictionary(); + var variables = new List<(uint Id, SpirvStorageClass StorageClass)>(); + + int index = 5; + while (index < words.Length) + { + uint header = words[index]; + int wordCount = (int)(header >> 16); + uint opcode = header & 0xFFFF; + Assert.True(wordCount > 0, $"{path} has a zero-length instruction at word {index}."); + + if (opcode == OpDecorate && wordCount >= 4) + { + uint target = words[index + 1]; + uint decoration = words[index + 2]; + if (decoration == DecorationDescriptorSet) + sets[target] = words[index + 3]; + else if (decoration == DecorationBinding) + bindings[target] = words[index + 3]; + } + else if (opcode == OpVariable && wordCount >= 4) + { + variables.Add((words[index + 2], (SpirvStorageClass)words[index + 3])); + } + + index += wordCount; + } + + string module = Path.GetFileName(path); + return + [ + .. variables.Select(v => new ShaderResource( + module, + v.Id, + v.StorageClass, + sets.TryGetValue(v.Id, out uint s) ? s : null, + bindings.TryGetValue(v.Id, out uint b) ? b : null)), + ]; + } + + private static IReadOnlyList AllResources() => + [ + .. Directory + .EnumerateFiles(SpirvDirectory(), "*.spv") + .OrderBy(path => path, StringComparer.Ordinal) + .SelectMany(ReadResources), + ]; + + /// + /// The regression itself, named. TerrainClip is the only uniform block + /// terrain_modern.vert declares besides SceneLighting, so + /// asserting the module's uniform bindings as a set pins it exactly. + /// + [Fact] + public void TerrainVertexShaderDeclaresItsClipBlockInTheUniformSet() + { + uint[] uniformBindings = + [ + .. ReadResources(Path.Combine(SpirvDirectory(), "terrain_modern.vert.spv")) + .Where(r => r.StorageClass == SpirvStorageClass.Uniform) + .Select(r => + { + Assert.Equal(GpuBindingModel.UniformSet, r.Set); + return r.Binding!.Value; + }) + .Order(), + ]; + + Assert.Equal( + [GpuBindingModel.UniformSceneLighting, VulkanPipelineLayouts.UniformTerrainClip], + uniformBindings); + } + + /// + /// The general rule the specific case is an instance of: a uniform block may + /// only live at a binding + /// actually declares, in the set it declares them in. + /// + [Fact] + public void EveryUniformBlockLandsAtADeclaredUniformBinding() + { + string[] violations = + [ + .. AllResources() + .Where(r => r.StorageClass == SpirvStorageClass.Uniform) + .Where(r => + r.Set != GpuBindingModel.UniformSet + || r.Binding is null + || !VulkanPipelineLayouts.IsDeclaredUniformBinding(r.Binding.Value)) + .Select(r => + $"{r.Module}: uniform block %{r.Id} is at set {r.Set?.ToString() ?? "(none)"} " + + $"binding {r.Binding?.ToString() ?? "(none)"}; set 1 declares " + + $"[{string.Join(", ", VulkanPipelineLayouts.DeclaredUniformBindings)}]."), + ]; + + Assert.Empty(violations); + } + + /// + /// The other half of the same contract: set 0 is storage buffers only, and + /// within the range pins. A + /// uniform block straying into set 0 fails this as well as the test above, + /// which is the point — the two layouts must partition the bindings. + /// + [Fact] + public void EveryStorageBlockLandsInTheStorageSetWithinItsDeclaredRange() + { + string[] violations = + [ + .. AllResources() + .Where(r => r.StorageClass == SpirvStorageClass.StorageBuffer) + .Where(r => r.Set != 0 || r.Binding is null || r.Binding >= GpuBindingModel.StorageBindingCount) + .Select(r => + $"{r.Module}: storage block %{r.Id} is at set {r.Set?.ToString() ?? "(none)"} " + + $"binding {r.Binding?.ToString() ?? "(none)"}; set 0 declares bindings " + + $"0..{GpuBindingModel.StorageBindingCount - 1}."), + ]; + + Assert.Empty(violations); + } + + /// + /// The sampled-texture table is one variable-count descriptor array at set 2 + /// binding 0. Any other combined-image-sampler would need a layout that does + /// not exist. + /// + [Fact] + public void EverySampledTextureIsTheSharedTable() + { + string[] violations = + [ + .. AllResources() + .Where(r => r.StorageClass == SpirvStorageClass.UniformConstant && r.Set is not null) + .Where(r => r.Set != GpuBindingModel.TextureTableSet || r.Binding != GpuBindingModel.TextureTableBinding) + .Select(r => + $"{r.Module}: sampled resource %{r.Id} is at set {r.Set} binding " + + $"{r.Binding?.ToString() ?? "(none)"}; the only declared table is set " + + $"{GpuBindingModel.TextureTableSet} binding {GpuBindingModel.TextureTableBinding}."), + ]; + + Assert.Empty(violations); + } +}