From 3418f6546235726caeff70bf77499f28a9317f21 Mon Sep 17 00:00:00 2001 From: Erik Date: Sat, 9 May 2026 09:15:51 +0200 Subject: [PATCH] fix(N.5b T6): index-length validation + document VertsPerLandblock %6 invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review (Important #1): AddLandblock validated Vertices.Length but not Indices.Length. The indices loop indexes meshData.Indices[0..383] unconditionally — out-of-range input would throw IndexOutOfRangeException instead of the clearer ArgumentException the vertex check raises. Today LandblockMesh.Build always produces 384/384, so this is defensive forward-compat for future mesh sources. Code review (Important #2): The shader (terrain_modern.vert:gl_VertexID % 6) only correctly picks the cell-corner index because we bake `slot * VertsPerLandblock` into indices and 384 is a multiple of 6. That invariant is now documented in a comment near the constant — anyone changing it must audit the shader. Build green: 0 errors / 0 warnings. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/AcDream.App/Rendering/TerrainModernRenderer.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/AcDream.App/Rendering/TerrainModernRenderer.cs b/src/AcDream.App/Rendering/TerrainModernRenderer.cs index efa54ea..e70a955 100644 --- a/src/AcDream.App/Rendering/TerrainModernRenderer.cs +++ b/src/AcDream.App/Rendering/TerrainModernRenderer.cs @@ -17,7 +17,14 @@ namespace AcDream.App.Rendering; /// public sealed unsafe class TerrainModernRenderer : IDisposable { - private const int VertsPerLandblock = LandblockMesh.VerticesPerLandblock; // 384 + // VertsPerLandblock MUST stay divisible by 6 — terrain_modern.vert uses + // `gl_VertexID % 6` to pick the cell-corner index (BL/BR/TR/TL), and + // because we bake `slot * VertsPerLandblock` into indices CPU-side and + // pass BaseVertex=0 to MultiDrawElementsIndirect, gl_VertexID becomes + // `slot * VertsPerLandblock + local_index`. The shader's modulo-6 only + // reduces to `local_index % 6` because 384 is a multiple of 6. Changing + // either constant without auditing the shader will silently mis-render. + private const int VertsPerLandblock = LandblockMesh.VerticesPerLandblock; // 384 (= 64 cells * 6 verts) private const int IndicesPerLandblock = VertsPerLandblock; private const int VertexSize = 40; // sizeof(TerrainVertex) private const int IndexSize = sizeof(uint); @@ -89,6 +96,10 @@ public sealed unsafe class TerrainModernRenderer : IDisposable throw new ArgumentException( $"Expected {VertsPerLandblock} vertices, got {meshData.Vertices.Length}", nameof(meshData)); + if (meshData.Indices.Length != IndicesPerLandblock) + throw new ArgumentException( + $"Expected {IndicesPerLandblock} indices, got {meshData.Indices.Length}", + nameof(meshData)); if (_idToSlot.ContainsKey(landblockId)) RemoveLandblock(landblockId);