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() {