From 92c503b253df5cc9bdae7741fa8adcd75d83bf75 Mon Sep 17 00:00:00 2001 From: Julian Date: Tue, 17 Mar 2026 02:18:37 +0000 Subject: [PATCH] refactor: address PR review feedback on meshlet LOD system - Remove WORK_SUMMARY.md - Use Debug.Assert for stride validation in ClodBuilder - Fix ClodBuilder.Build return value after cluster disposal - Update ClodPartition to accept AllocationHandle for return collections - Standardize on camelCase for public fields in ClodConfig, ClodMesh, ClodBounds, etc. - Remove redundant Resize calls where capacity suffices or Add is used - Enforce stack allocator usage for internal temporary collections - Ensure proper allocator propagation for collections returned from methods --- WORK_SUMMARY.md | 62 ------------------- .../Meshlet/ClodBoundsHelper.cs | 5 +- .../Ghost.Graphics/Meshlet/ClodBuilder.cs | 56 +++++++---------- .../Meshlet/ClodInternal_Partition.cs | 13 ++-- .../Ghost.Graphics/Meshlet/ClodMesh.cs | 41 +++++++++--- 5 files changed, 64 insertions(+), 113 deletions(-) delete mode 100644 WORK_SUMMARY.md diff --git a/WORK_SUMMARY.md b/WORK_SUMMARY.md deleted file mode 100644 index 873579b..0000000 --- a/WORK_SUMMARY.md +++ /dev/null @@ -1,62 +0,0 @@ -# ClusterLOD C# Translation — Work Summary - -## ✅ Completed Work - -I've successfully translated the C++ `clusterlod` library to C# using your GhostEngine infrastructure: - -### Files Created - -1. **ClodConfig.cs** — Configuration struct with fields (`camelCase`) and properties (`PascalCase`) -2. **ClodMesh.cs** — Unsafe mesh data structure (pointers for high-performance access) -3. **ClodBounds.cs** — Bounds representation with center, radius, and error -4. **ClodBuilder.cs** — Main API entry point with full `clodBuild` implementation -5. **ClodInternal.cs** — Clusterization using `meshopt_buildMeshlets*` bindings -6. **ClodInternal_Partition.cs** — Spatial partitioning via `meshopt_partitionClusters` -7. **ClodInternal_Boundary.cs** — Boundary locking to prevent mesh seams -8. **ClodBoundsHelper.cs** — Bounds computation and merging -9. **ClodSimplify.cs** — Full simplification pipeline (permissive + sloppy fallbacks) -10. **AGENT_GUIDELINES.md** — Your naming conventions and architecture guide - -### Features Implemented - -✅ High-performance memory via `UnsafeList` from `Misaki.HighPerformance` -✅ Full cluster LOD hierarchy generation -✅ Attribute-aware simplification -✅ Error monotonicity tracking -✅ Boundary preservation across simplification -✅ Proper allocator handling and cleanup -✅ Edge-length error limiting -✅ Fallback simplification (permissive & sloppy modes) -✅ C# naming conventions (fields camelCase, props PascalCase, consts UPPER_SNAKE_CASE) - -### Changes to Native Bindings - -- Renamed `NvttApi` → `MeshOptApi` in `meshopt.json` config -- Re-ran code generator to produce `MeshOptApi.nativegen.cs` and mesh-related wrapper files - -### Local Commits - -``` -301a6d1 feat: translate clusterlod to C# and restructure to Ghost.Graphics.Meshlet -``` - -## 📤 Next Steps (You) - -The commit is ready locally. To push and create a PR: - -```bash -cd projects/GhostEngine -git push origin develop -``` - -Then create a PR from `Julian/GhostEngine:develop` → `Misaki/GhostEngine:develop`. - -## 🎯 What's Ready for Testing - -- Full clusterization pipeline -- Spatial/flex meshlet building -- Simplification with all fallback modes -- Bounds computation and hierarchy tracking -- Ready to integrate with your graphics pipeline - -Let me know if you'd like me to refine anything or add documentation! diff --git a/src/Runtime/Ghost.Graphics/Meshlet/ClodBoundsHelper.cs b/src/Runtime/Ghost.Graphics/Meshlet/ClodBoundsHelper.cs index a352d2a..d0045cc 100644 --- a/src/Runtime/Ghost.Graphics/Meshlet/ClodBoundsHelper.cs +++ b/src/Runtime/Ghost.Graphics/Meshlet/ClodBoundsHelper.cs @@ -21,12 +21,11 @@ internal static class ClodBoundsHelper public static ClodBounds MergeBounds(UnsafeList clusters, UnsafeList group) { using var scope = AllocationManager.CreateStackScope(); - var boundsList = new UnsafeList(group.Length, scope.AllocationHandle); - boundsList.Resize((nuint)group.Length); + var boundsList = new UnsafeList((nuint)group.Length, scope.AllocationHandle); for (int j = 0; j < (int)group.Length; j++) { - boundsList[j] = clusters[group[j]].bounds; + boundsList.Add(clusters[group[j]].bounds); } var merged = MeshOptApi.ComputeSphereBounds( diff --git a/src/Runtime/Ghost.Graphics/Meshlet/ClodBuilder.cs b/src/Runtime/Ghost.Graphics/Meshlet/ClodBuilder.cs index da405df..8e4367b 100644 --- a/src/Runtime/Ghost.Graphics/Meshlet/ClodBuilder.cs +++ b/src/Runtime/Ghost.Graphics/Meshlet/ClodBuilder.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics; using System.Numerics; using Ghost.MeshOptimizer; using Misaki.HighPerformance; @@ -16,24 +17,19 @@ internal struct Cluster public unsafe static class ClodBuilder { - private const float CONST_SIMPLIFY_RATIO_DEFAULT = 0.5f; - private const float CONST_SIMPLIFY_THRESHOLD_DEFAULT = 0.85f; - private const float CONST_SIMPLIFY_ERROR_MERGE_PREVIOUS_DEFAULT = 1.0f; - private const float CONST_SIMPLIFY_ERROR_MERGE_ADDITIVE_DEFAULT = 0.0f; - private const float CONST_SIMPLIFY_ERROR_FACTOR_SLOPPY_DEFAULT = 2.0f; - - public static nuint Build(ClodConfig config, ClodMesh mesh, void* outputContext, ClodOutputDelegate outputCallback, Allocator allocator = Allocator.Persistent) + public static nuint Build(ClodConfig config, ClodMesh mesh, void* outputContext, ClodOutputDelegate outputCallback) { - if (mesh.vertexAttributesStride % (nuint)sizeof(float) != 0) - throw new ArgumentException("vertexAttributesStride must be a multiple of sizeof(float)"); + Debug.Assert(mesh.vertexAttributesStride % (nuint)sizeof(float) == 0, "vertexAttributesStride must be a multiple of sizeof(float)"); - var locks = new UnsafeList((int)mesh.vertexCount, allocator); + using var scope = AllocationManager.CreateStackScope(); + + var locks = new UnsafeList(mesh.vertexCount, scope.AllocationHandle); locks.Resize(mesh.vertexCount); for (int i = 0; i < (int)mesh.vertexCount; i++) locks[i] = 0; // Generate position-only remap - var remap = new UnsafeList((int)mesh.vertexCount, allocator); + var remap = new UnsafeList(mesh.vertexCount, scope.AllocationHandle); remap.Resize(mesh.vertexCount); MeshOptApi.GeneratePositionRemap(remap.GetUnsafePtr(), mesh.vertexPositions, mesh.vertexCount, mesh.vertexPositionsStride); @@ -58,7 +54,7 @@ public unsafe static class ClodBuilder } // Initial clusterization - var clusters = ClodInternal.Clusterize(config, mesh, mesh.indices, mesh.indexCount, allocator); + var clusters = ClodInternal.Clusterize(config, mesh, mesh.indices, mesh.indexCount, Allocator.Persistent); // Compute initial bounds for (int i = 0; i < (int)clusters.Length; i++) @@ -66,8 +62,8 @@ public unsafe static class ClodBuilder clusters[i].bounds = ClodBoundsHelper.ComputeBounds(mesh, clusters[i].indices, 0.0f); } - var pending = new UnsafeList((int)clusters.Length, allocator); - pending.Resize((nuint)clusters.Length); + var pending = new UnsafeList(clusters.Length, scope.AllocationHandle); + pending.Resize(clusters.Length); for (int i = 0; i < (int)clusters.Length; i++) pending[i] = i; @@ -75,7 +71,7 @@ public unsafe static class ClodBuilder while (pending.Length > 1) { - var groups = ClodPartition.Partition(config, mesh, clusters, pending, remap); + var groups = ClodPartition.Partition(config, mesh, clusters, pending, remap, scope.AllocationHandle); pending.Clear(); @@ -84,7 +80,7 @@ public unsafe static class ClodBuilder for (int i = 0; i < (int)groups.Length; i++) { - var merged = new UnsafeList(groups[i].Length * (int)config.maxTriangles * 3, allocator); + var merged = new UnsafeList((nuint)groups[i].Length * config.maxTriangles * 3, scope.AllocationHandle); for (int j = 0; j < (int)groups[i].Length; j++) { var clusterIndices = clusters[groups[i][j]].indices; @@ -102,15 +98,13 @@ public unsafe static class ClodBuilder if (simplified.Length > (nuint)(merged.Length * config.simplifyThreshold)) { bounds.error = float.MaxValue; - OutputGroup(config, mesh, clusters, groups[i], bounds, depth, outputContext, outputCallback, allocator); - merged.Dispose(); - simplified.Dispose(); + OutputGroup(config, mesh, clusters, groups[i], bounds, depth, outputContext, outputCallback); continue; } bounds.error = Math.Max(bounds.error * config.simplifyErrorMergePrevious, error) + error * config.simplifyErrorMergeAdditive; - int refined = OutputGroup(config, mesh, clusters, groups[i], bounds, depth, outputContext, outputCallback, allocator); + int refined = OutputGroup(config, mesh, clusters, groups[i], bounds, depth, outputContext, outputCallback); // Discard old clusters for (int j = 0; j < (int)groups[i].Length; j++) @@ -119,7 +113,7 @@ public unsafe static class ClodBuilder } // Clusterize simplified mesh - var split = ClodInternal.Clusterize(config, mesh, simplified.GetUnsafePtr(), simplified.Length, allocator); + var split = ClodInternal.Clusterize(config, mesh, simplified.GetUnsafePtr(), simplified.Length, Allocator.Persistent); for (int j = 0; j < (int)split.Length; j++) { split[j].refined = refined; @@ -129,8 +123,6 @@ public unsafe static class ClodBuilder } split.Dispose(); - merged.Dispose(); - simplified.Dispose(); } // Cleanup groups @@ -146,18 +138,17 @@ public unsafe static class ClodBuilder var cluster = clusters[pending[0]]; var bounds = cluster.bounds; bounds.error = float.MaxValue; - OutputGroup(config, mesh, clusters, pending, bounds, depth, outputContext, outputCallback, allocator); + OutputGroup(config, mesh, clusters, pending, bounds, depth, outputContext, outputCallback); } + nuint finalClusterCount = clusters.Length; + // Cleanup for (int i = 0; i < (int)clusters.Length; i++) clusters[i].indices.Dispose(); clusters.Dispose(); - locks.Dispose(); - remap.Dispose(); - pending.Dispose(); - return (nuint)clusters.Length; + return finalClusterCount; } private static int OutputGroup( @@ -168,11 +159,11 @@ public unsafe static class ClodBuilder ClodBounds simplified, int depth, void* outputContext, - ClodOutputDelegate outputCallback, - Allocator allocator + ClodOutputDelegate outputCallback ) { - var groupClusters = new UnsafeList(group.Length, allocator); + using var scope = AllocationManager.CreateStackScope(); + var groupClusters = new UnsafeList(group.Length, scope.AllocationHandle); groupClusters.Resize((nuint)group.Length); for (int i = 0; i < (int)group.Length; i++) @@ -189,12 +180,11 @@ public unsafe static class ClodBuilder dstCluster.vertexCount = srcCluster.vertices; } - var clodGroup = new ClodGroup { Depth = depth, Simplified = simplified }; + var clodGroup = new ClodGroup { depth = depth, simplified = simplified }; int result = outputCallback != null ? outputCallback(outputContext, clodGroup, (ClodCluster*)groupClusters.GetUnsafePtr(), (nuint)groupClusters.Length) : -1; - groupClusters.Dispose(); return result; } } diff --git a/src/Runtime/Ghost.Graphics/Meshlet/ClodInternal_Partition.cs b/src/Runtime/Ghost.Graphics/Meshlet/ClodInternal_Partition.cs index 0d6a8c6..2fb0c3f 100644 --- a/src/Runtime/Ghost.Graphics/Meshlet/ClodInternal_Partition.cs +++ b/src/Runtime/Ghost.Graphics/Meshlet/ClodInternal_Partition.cs @@ -6,19 +6,18 @@ namespace Ghost.Graphics.Meshlet; internal static class ClodPartition { - public static UnsafeList> Partition(ClodConfig config, ClodMesh mesh, UnsafeList clusters, UnsafeList pending, UnsafeList remap) + public static UnsafeList> Partition(ClodConfig config, ClodMesh mesh, UnsafeList clusters, UnsafeList pending, UnsafeList remap, AllocationHandle allocator) { if (pending.Length <= (int)config.partitionSize) { - using var scope = AllocationManager.CreateStackScope(); - var partitions = new UnsafeList>(1, scope.AllocationHandle); + var partitions = new UnsafeList>(1, allocator); partitions.Add(pending); return partitions; } using var stackScope = AllocationManager.CreateStackScope(); var clusterIndices = new UnsafeList(1024, stackScope.AllocationHandle); - var clusterCounts = new UnsafeList(pending.Length, stackScope.AllocationHandle); + var clusterCounts = new UnsafeList((nuint)pending.Length, stackScope.AllocationHandle); nuint totalIndexCount = 0; for (int i = 0; i < pending.Length; i++) @@ -42,7 +41,7 @@ internal static class ClodPartition offset += (nuint)cluster.indices.Length; } - var clusterPart = new UnsafeList(pending.Length, stackScope.AllocationHandle); + var clusterPart = new UnsafeList((nuint)pending.Length, stackScope.AllocationHandle); clusterPart.Resize((nuint)pending.Length); nuint partitionCount = MeshOptApi.PartitionClusters( @@ -57,10 +56,10 @@ internal static class ClodPartition config.partitionSize ); - var partitions = new UnsafeList>(partitionCount, stackScope.AllocationHandle); + var partitions = new UnsafeList>(partitionCount, allocator); for (nuint i = 0; i < partitionCount; i++) { - partitions.Add(new UnsafeList((nuint)(config.partitionSize + config.partitionSize / 3), stackScope.AllocationHandle)); + partitions.Add(new UnsafeList((nuint)(config.partitionSize + config.partitionSize / 3), allocator)); } for (int i = 0; i < pending.Length; i++) diff --git a/src/Runtime/Ghost.Graphics/Meshlet/ClodMesh.cs b/src/Runtime/Ghost.Graphics/Meshlet/ClodMesh.cs index 55984bd..e552d89 100644 --- a/src/Runtime/Ghost.Graphics/Meshlet/ClodMesh.cs +++ b/src/Runtime/Ghost.Graphics/Meshlet/ClodMesh.cs @@ -1,16 +1,41 @@ +using System; +using Ghost.MeshOptimizer; +using Misaki.HighPerformance; + namespace Ghost.Graphics.Meshlet; public unsafe struct ClodMesh { + public float* vertexPositions; + public nuint vertexCount; + public nuint vertexPositionsStride; + + public float* vertexAttributes; + public nuint vertexAttributesStride; + public float* attributeWeights; + public nuint attributeCount; + + public uint* indices; + public nuint indexCount; + + public byte* vertexLock; + public uint attributeProtectMask; +} + +public struct ClodGroup +{ + public int depth; + public ClodBounds simplified; +} + +public unsafe struct ClodCluster +{ + public int refined; + public ClodBounds bounds; + public uint* indices; public nuint indexCount; public nuint vertexCount; - public float* vertexPositions; - public nuint vertexPositionsStride; - public float* vertexAttributes; - public nuint vertexAttributesStride; - public byte* vertexLock; - public float* attributeWeights; - public nuint attributeCount; - public uint attributeProtectMask; } + +public unsafe delegate int ClodOutputDelegate(void* context, ClodGroup group, ClodCluster* clusters, nuint clusterCount);