From c7f5f251f83ed8f960cdaced29e8a1182df7bb88 Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 27 Jul 2026 21:11:48 +0200 Subject: [PATCH] feat(render): add integer vertex attributes and the terrain tiling binding A scouting pass over V4d stopped before writing code and reported three gaps between terrain and the pinned contract. All three verified against source. The load-bearing one: terrain_modern.vert declares locations 2-5 as uvec4 and TerrainModernRenderer feeds them with glVertexAttribIPointer, but GpuVertexFormat had no integer format and the encoder only issued glVertexAttribPointer. GL leaves an integer shader input undefined if it arrives through the float path, and Vulkan needs the format named as R8G8B8A8_UINT rather than _UNORM, so UByte4Normalized cannot stand in for it. Those packed bytes carry terrain-type, road and split-direction codes that drive every blend decision, so normalising them would have produced garbage rather than an approximation. Adds GpuVertexFormat.UByte4UInt and an integer branch in the encoder. Also adds a uniform binding for terrain's 36-float per-layer tiling array, which at 144 bytes cannot ride in the 96-byte push-constant block or Vulkan's guaranteed 128-byte ceiling, and has no uniform-array verb to reach it otherwise. Corrects two V4d plan rows: TerrainAtlas belongs to V4t with the rest of the texture stack, and terrain has no GPU timer to port since its diagnostics use a CPU stopwatch. The uView/uProjection convergence gets its own pixel-gated sub-commit because it moves a matrix product from per-vertex GPU evaluation to a CPU multiply, and that rounding effect should be attributable on its own. Files #250: two zero-allocation tests fail about one run in three on an unchanged tree, independent of this campaign. That noise trains everyone to re-run until green, which is how a real regression gets waved through. Co-Authored-By: Claude Fable 5 --- docs/ISSUES.md | 43 +++++++++++++++++++ docs/plans/2026-07-27-vulkan-campaign.md | 2 +- .../Rendering/Gpu/Gl/GlEnumMapping.cs | 15 ++++++- .../Rendering/Gpu/Gl/GlGpuPassEncoder.cs | 28 +++++++++--- .../Rendering/Gpu/GpuBindingModel.cs | 11 +++++ .../Rendering/Gpu/GpuPipelineDescription.cs | 18 ++++++++ .../Rendering/Gpu/GpuContractTests.cs | 28 ++++++++++++ 7 files changed, 136 insertions(+), 9 deletions(-) diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 2a59ed93..1633705a 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -97,6 +97,49 @@ Copy this block when adding a new issue: --- +## #250 — Zero-allocation tests fail intermittently, roughly 1 run in 3 + +**Status:** OPEN +**Severity:** MEDIUM (undermines every "tests green" gate) +**Filed:** 2026-07-27 +**Component:** tests / allocation assertions + +**Description:** Two tests fail non-deterministically on an otherwise unchanged +tree: + +- `AcDream.App.Tests.UI.UiDatFontTests.InstanceMeasureWidth_ReusesGlyphTableWithoutAllocating` +- `AcDream.App.Tests.Rendering.RenderFrameProductTests.WarmProductBuildAndBorrowAllocateNothing` + +Measured over six consecutive Release runs of the App suite on an unmodified +tree: four passed 3,846/3, and two failed with exactly one failure — a different +one of the pair each time. So the observed rate is about one run in three, and +it is not specific to either test. + +Both assert that a warmed code path allocates zero managed bytes. That +measurement is inherently sensitive to anything else the runtime does on the +thread — tiered JIT recompilation and background GC bookkeeping can both attribute +bytes to the measured window. + +**Why it matters now:** Campaign V's acceptance criteria include 0 B/frame +steady-state managed allocation, and these are the tests that guard it. A gate +that fails a third of the time for unrelated reasons trains everyone to re-run +until green, which is exactly how a real regression gets waved through — see the +V4a revert, where a failing gate was rationalised rather than investigated. + +**Provenance:** first observed during Campaign V slice V2 and dismissed as +unrelated flakiness; seen again independently by the V4c and V4d agents; then +reproduced deliberately here. Not caused by the campaign. + +**Fix direction:** warm the path harder before measuring (force tiered +promotion), take the best of N samples rather than a single one, or measure with +`GC.TryStartNoGCRegion`. Whichever is chosen, the assertion should stay strict — +the goal is to remove the measurement noise, not to loosen the bound. + +**Acceptance:** twenty consecutive Release runs of the App suite with zero +failures. + +--- + ## #249 — Bindless handles stay resident after their table slot is released **Status:** OPEN diff --git a/docs/plans/2026-07-27-vulkan-campaign.md b/docs/plans/2026-07-27-vulkan-campaign.md index 179d8469..96ca65fa 100644 --- a/docs/plans/2026-07-27-vulkan-campaign.md +++ b/docs/plans/2026-07-27-vulkan-campaign.md @@ -517,7 +517,7 @@ because sample positions are not specified across implementations. | **V4b** | `GlobalMeshBuffer` + `ObjectMeshManager` onto `IGpuBuffer`; arena, LRU and ledger logic untouched. | pixel gate | | **V4c** | **The large one.** `WbDrawDispatcher` + `EnvCellRenderer`: per-frame uploads → rings, MDI brackets → pipelines + `MultiDrawIndexedIndirect`, loose uniforms → push constants, timer scopes. `RetailAlphaQueue` and all bucketing untouched. **Narrowed after the V4c scouting report — see §5.3.** | pixel gate at several checkpoints + connected lifecycle | | **V4t** | **World texture stack** (added 2026-07-27, see §5.3): `TextureCache`, `CompositeTextureArrayCache`, `ManagedGLTextureArray`, `TerrainAtlas` and `ObjectMeshManager`'s material path onto `IGpuTexture`/`IGpuSampler`; retype `GroupKey`, `CachedBatch` and `ObjectRenderBatch` from `ulong` bindless handle to `GpuTextureSlot`; retire the interim per-renderer handle tables for V4c, V4d and V4e at once. | pixel gate | -| **V4d** | `TerrainModernRenderer` + `TerrainAtlas`. | pixel gate | +| **V4d** | `TerrainModernRenderer` only — **`TerrainAtlas` belongs to V4t** with the rest of the texture stack. Two sub-commits: first the `uView`/`uProjection` → `uViewProjection` shader convergence on its own pixel gate (it moves a matrix product from per-vertex GPU to a CPU multiply, so its rounding effect must be attributable alone), then the plumbing. Terrain has no GPU timer to port — its diagnostics use a CPU `Stopwatch`. | pixel gate per sub-commit | | **V4e** | `ParticleRenderer` (after V4c — shared alpha-queue contract). | pixel gate (particle-heavy checkpoint) | | **V4f** | `SkyRenderer` + weather. | pixel gate (dawn/dusk, day group pinned) | | **V4g** | `PrivateEntityViewportRenderer` → `IGpuRenderTarget`; `PortalDepthMaskRenderer` + `PortalTunnelPresentation` → stencil/depth-mask pipelines. | pixel gate incl. paperdoll and portal transit | diff --git a/src/AcDream.App/Rendering/Gpu/Gl/GlEnumMapping.cs b/src/AcDream.App/Rendering/Gpu/Gl/GlEnumMapping.cs index a2da95f7..997f513a 100644 --- a/src/AcDream.App/Rendering/Gpu/Gl/GlEnumMapping.cs +++ b/src/AcDream.App/Rendering/Gpu/Gl/GlEnumMapping.cs @@ -3,7 +3,17 @@ using Silk.NET.OpenGL; namespace AcDream.App.Rendering.Gpu.Gl; /// One vertex attribute's GL shape: component count, element type, and whether integer values normalize to [0,1]/[-1,1]. -internal readonly record struct GlVertexAttributeShape(int ComponentCount, VertexAttribPointerType Type, bool Normalized); +/// +/// How one vertex attribute reaches GL. selects +/// glVertexAttribIPointer over glVertexAttribPointer: GL requires +/// the integer entry point for an integer shader input (uvec4 and friends) +/// and leaves the value undefined otherwise. +/// +internal readonly record struct GlVertexAttributeShape( + int ComponentCount, + VertexAttribPointerType Type, + bool Normalized, + bool Integer = false); /// /// Pure, GL-context-free mappings from the RHI's backend-neutral enums to @@ -21,6 +31,9 @@ internal static class GlEnumMapping GpuVertexFormat.Float3 => new GlVertexAttributeShape(3, VertexAttribPointerType.Float, false), GpuVertexFormat.Float4 => new GlVertexAttributeShape(4, VertexAttribPointerType.Float, false), GpuVertexFormat.UByte4Normalized => new GlVertexAttributeShape(4, VertexAttribPointerType.UnsignedByte, true), + // Integer attributes carry Integer = true; the encoder must route them + // through glVertexAttribIPointer, not the normalized float path. + GpuVertexFormat.UByte4UInt => new GlVertexAttributeShape(4, VertexAttribPointerType.UnsignedByte, false, Integer: true), _ => throw new NotSupportedException($"No GL vertex attribute shape for {format}."), }; diff --git a/src/AcDream.App/Rendering/Gpu/Gl/GlGpuPassEncoder.cs b/src/AcDream.App/Rendering/Gpu/Gl/GlGpuPassEncoder.cs index 50c7cd0a..09e33cd1 100644 --- a/src/AcDream.App/Rendering/Gpu/Gl/GlGpuPassEncoder.cs +++ b/src/AcDream.App/Rendering/Gpu/Gl/GlGpuPassEncoder.cs @@ -110,13 +110,27 @@ internal sealed class GlGpuPassEncoder : IGpuPassEncoder { GlVertexAttributeShape shape = GlEnumMapping.VertexShapeOf(attribute.Format); nint attributeOffset = (nint)(offsetBytes + attribute.OffsetBytes); - _gl.VertexAttribPointer( - attribute.Location, - shape.ComponentCount, - shape.Type, - shape.Normalized, - layout.StrideBytes, - (void*)attributeOffset); + if (shape.Integer) + { + // An integer shader input (uvec4) must come through the I-form. + // Supplying it via glVertexAttribPointer leaves the value undefined. + _gl.VertexAttribIPointer( + attribute.Location, + shape.ComponentCount, + (VertexAttribIType)shape.Type, + layout.StrideBytes, + (void*)attributeOffset); + } + else + { + _gl.VertexAttribPointer( + attribute.Location, + shape.ComponentCount, + shape.Type, + shape.Normalized, + layout.StrideBytes, + (void*)attributeOffset); + } } GLHelpers.ThrowOnResourceError(_gl, $"bind vertex buffer '{buffer.Name}'"); _gl.BindBuffer(GLEnum.ArrayBuffer, 0); diff --git a/src/AcDream.App/Rendering/Gpu/GpuBindingModel.cs b/src/AcDream.App/Rendering/Gpu/GpuBindingModel.cs index 2543be4b..196d7fa3 100644 --- a/src/AcDream.App/Rendering/Gpu/GpuBindingModel.cs +++ b/src/AcDream.App/Rendering/Gpu/GpuBindingModel.cs @@ -78,6 +78,17 @@ internal static class GpuBindingModel /// public const uint UniformSceneLighting = 1; + /// + /// Terrain per-layer texture tiling factors — 36 floats. + /// + /// Added at slice V4d. These live in a uniform float[36] today, which + /// is 144 bytes: too large for the 96-byte push-constant block (and for + /// Vulkan's guaranteed 128-byte ceiling), and there is no RHI verb for setting + /// a uniform array. A small uniform buffer is the Vulkan-legal home. Binding 2 + /// is taken by the terrain clip block, so this is 3. + /// + public const uint UniformTerrainTiling = 3; + /// Set index carrying every uniform buffer. public const uint UniformSet = 1; diff --git a/src/AcDream.App/Rendering/Gpu/GpuPipelineDescription.cs b/src/AcDream.App/Rendering/Gpu/GpuPipelineDescription.cs index 66e7f7c9..f631b615 100644 --- a/src/AcDream.App/Rendering/Gpu/GpuPipelineDescription.cs +++ b/src/AcDream.App/Rendering/Gpu/GpuPipelineDescription.cs @@ -9,7 +9,25 @@ internal enum GpuVertexFormat Float2, Float3, Float4, + + /// Four unsigned bytes scaled to [0,1] floats — a shader vec4 input. UByte4Normalized, + + /// + /// Four unsigned bytes delivered as INTEGERS — a shader uvec4 input. + /// + /// Distinct from in kind, not just in scaling: + /// GL requires glVertexAttribIPointer for an integer shader input and + /// leaves the value undefined if it arrives through the float path, and Vulkan + /// needs the format named as R8G8B8A8_UINT rather than _UNORM. + /// + /// Added at slice V4d, which found `terrain_modern.vert` declares locations 2–5 + /// as uvec4 and feeds them with glVertexAttribIPointer. Those + /// packed bytes carry terrain-type, road and split-direction codes that drive + /// every blend decision, so normalising them would not be an approximation — + /// it would be garbage. + /// + UByte4UInt, } /// One vertex attribute, matching a layout(location = N) in declaration. diff --git a/tests/AcDream.App.Tests/Rendering/Gpu/GpuContractTests.cs b/tests/AcDream.App.Tests/Rendering/Gpu/GpuContractTests.cs index 28cfc37b..b4286ca5 100644 --- a/tests/AcDream.App.Tests/Rendering/Gpu/GpuContractTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Gpu/GpuContractTests.cs @@ -86,6 +86,34 @@ public sealed class GpuContractTests Assert.Contains(GpuBlendMode.InverseAlpha, Enum.GetValues()); } + [Fact] + public void IntegerVertexAttributesAreRepresentableDistinctlyFromNormalizedOnes() + { + // terrain_modern.vert declares locations 2-5 as uvec4 and the CPU feeds + // them with glVertexAttribIPointer. GL leaves an integer shader input + // undefined if it arrives through the float path, and Vulkan needs the + // format named as R8G8B8A8_UINT rather than _UNORM — so the two cannot be + // the same contract value. Those packed bytes carry terrain-type, road and + // split-direction codes, so normalising them would produce garbage, not an + // approximation. + Assert.NotEqual(GpuVertexFormat.UByte4Normalized, GpuVertexFormat.UByte4UInt); + Assert.Contains(GpuVertexFormat.UByte4UInt, Enum.GetValues()); + } + + [Fact] + public void UniformBindingsDoNotCollide() + { + uint[] uniformBindings = + [ + GpuBindingModel.UniformSceneLighting, + GpuBindingModel.UniformTerrainTiling, + ]; + + Assert.Equal(uniformBindings.Length, uniformBindings.Distinct().Count()); + // Binding 2 is the terrain clip block; the tiling array must not take it. + Assert.NotEqual(2u, GpuBindingModel.UniformTerrainTiling); + } + [Fact] public void UnassignedTextureSlotIsNeverAValidIndex() {