feat(perf): vertex-cache optimization + ACMR (Phase 6 slice C) - #498
Merged
Conversation
Builds the third optimization pillar of #261 on top of slices A/B, using the same shape so each layer (CLI / MCP / Inspector validator) gets the same hook. VertexCacheOptimizer (new) — pure-data Tom Forsyth's linear-time vertex cache optimizer plus an ACMR (Average Cache Miss Ratio) calculator. ~300 LoC, no external dep (rejected meshoptimizer because Forsyth's algorithm is ~150 lines inline and we only need the one routine). The Ogre-backed wrapper analyzeEntity() reads each SubMesh's index buffer into a unified uint32 vector, runs the optimizer, and (when rewrite=true) writes the result back through the existing 16/32-bit index path. Only writes back when the new ACMR is strictly lower — never regresses. Surfaced through: - CLI: `qtmesh vertex-cache <file> [-o <output>] [--json]`. Without -o, analyze-only (read-only). With -o, rewrite in-memory and export to a new file. JSON shape mirrors slice A/B. - MCP tool: `optimize_vertex_cache` with a `rewrite` bool arg. Returns text summary + structured `vertexCache` payload (per- submesh ACMR before/after + weighted totals). - Run Validation: the inspector checklist now includes a "Vertex cache: ACMR <n>" info row, pointing at the CLI / MCP for the actual reorder action (validation stays read-only). Tests: 14 VertexCacheOptimizerTest cases cover the byte math (ACMR on empty / single tri / strip / shuffled strip), Forsyth's behaviour (reduces ACMR ≥15% on shuffled meshes, preserves triangle set, rejects out-of-range indices), and JSON / text serialisation. MeshValidatorTest.DoValidateValidMeshReportsChecklist extended to assert the new ACMR row. Manual smoke: `qtmesh vertex-cache media/models/ninja.mesh -o ...` produced ACMR 0.932 → 0.841 (9.7% improvement) on the larger submesh while leaving the already-optimal one untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Forsyth-based vertex-cache optimizer with ACMR analysis and reporting, wired into CLI, MCP, MeshValidator (with QML action), ScanEngine/ScanConfig metric and rule plumbing, plus serializers, unit tests, and build/test updates. ChangesVertex Cache Optimizer Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant CLIPipeline
participant MeshImporter
participant VertexCacheOptimizer
participant MeshExporter
User->>CLIPipeline: invoke vertex-cache <file> [-o output] [--json]
CLIPipeline->>MeshImporter: import file -> entities
CLIPipeline->>VertexCacheOptimizer: analyzeEntity(entity, rewrite?)
VertexCacheOptimizer->>VertexCacheOptimizer: computeAcmr / forsyth
alt rewrite and improved
VertexCacheOptimizer->>MeshExporter: write reordered indices
MeshExporter->>User: exported file
end
VertexCacheOptimizer->>CLIPipeline: report (JSON/text)
CLIPipeline->>User: display or emit JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/CLIPipeline.cpp`:
- Around line 3514-3515: Replace the custom breadcrumb category used when
logging the vertex-cache command with the guideline-approved category
"ui.action": update the SentryReporter::addBreadcrumb call in CLIPipeline.cpp
(the call that currently uses "cli.vertex-cache" and constructs the message with
QString("Vertex-cache .%1%2").arg(fi.suffix(), rewrite ? " rewrite" : "
analyze")) to use "ui.action" as the first argument while leaving the message
construction unchanged so the event is recorded as a user action.
- Around line 3495-3496: The -o handler currently accepts "-o" with no value and
silently leaves outputPath empty; change the parsing in the CLI loop so that
when arg == "-o" you require a following non-option token (check i < argc and
that argv[i] does not start with '-') before assigning outputPath = argv[i++];
otherwise treat it as a CLI parse error: print usage/error and exit non-zero (or
return a parse-failure) instead of falling back to analyze-only. Update the
branch that sets outputPath and the error path handling near the filePath/arg
parsing to enforce this.
In `@src/MCPServer.cpp`:
- Around line 2809-2825: The per-entity loop calls
VertexCacheOptimizer::analyzeEntity and rewrites shared index buffers repeatedly
for instanced entities, making "before" metrics order-dependent; instead
deduplicate by mesh resource (e.g., use entity->getMesh() or mesh name/handle as
the key) and call analyze/rewrite once per unique mesh, then for aggregation
multiply each SubMeshCacheReport's metrics by the instance count of that mesh
when updating aggregate.submeshes, aggregate.totalTriangles,
aggregate.weightedAcmrBefore/After and aggregate.totalReordered; ensure you
still iterate scene nodes to count instances but perform analyze/rewrite only on
the unique-mesh set.
In `@src/MeshValidator.cpp`:
- Around line 384-387: The CLI hint in the validator message is incorrect;
update the string assignment to issue["description"] in MeshValidator.cpp so it
suggests the documented rewrite flag (-o) instead of `--rewrite` (e.g., change
the hint text "run `qtmesh vertex-cache --rewrite`" to "run `qtmesh vertex-cache
-o`" or equivalent documented flag), leaving the rest of the formatted message
and use of cacheReport.weightedAcmrBefore unchanged.
In `@src/VertexCacheOptimizer.cpp`:
- Around line 66-68: In forsyth(), reject or clamp non-positive cache sizes
before any reserve/use to avoid invalid behavior: check if cacheSize <= 0 and
either return false or set cacheSize = 1 (whichever matches project conventions)
immediately after the early-validations (the block that checks indices.empty(),
indices.size() % 3 and vertexCount) and before any use of cacheSize or calls
that rely on it (references: cacheSize, kMaxCachePos, indices, vertexCount, and
the forsyth() function itself); also keep the existing clamp to kMaxCachePos
after this new guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e18f2ab-8e19-46e9-81a4-ddf5b193a1e3
📒 Files selected for processing (12)
src/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MeshValidator.cppsrc/MeshValidator_test.cppsrc/VertexCacheOptimizer.cppsrc/VertexCacheOptimizer.hsrc/VertexCacheOptimizer_test.cppsrc/main.cpptests/CMakeLists.txt
Comment on lines
+3495
to
+3496
| if (arg == "-o" && i < argc) { outputPath = argv[i++]; continue; } | ||
| if (!arg.startsWith("-") && filePath.isEmpty()) filePath = arg; |
There was a problem hiding this comment.
Handle missing -o value as a hard CLI parse error.
If -o is provided without a path, the command currently falls back to analyze-only instead of returning usage error.
Suggested fix
- if (arg == "-o" && i < argc) { outputPath = argv[i++]; continue; }
+ if (arg == "-o" || arg == "--output") {
+ if (i >= argc) {
+ err() << "Error: -o/--output requires a value." << Qt::endl;
+ return 2;
+ }
+ outputPath = QString::fromUtf8(argv[i++]);
+ continue;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/CLIPipeline.cpp` around lines 3495 - 3496, The -o handler currently
accepts "-o" with no value and silently leaves outputPath empty; change the
parsing in the CLI loop so that when arg == "-o" you require a following
non-option token (check i < argc and that argv[i] does not start with '-')
before assigning outputPath = argv[i++]; otherwise treat it as a CLI parse
error: print usage/error and exit non-zero (or return a parse-failure) instead
of falling back to analyze-only. Update the branch that sets outputPath and the
error path handling near the filePath/arg parsing to enforce this.
Comment on lines
+3514
to
+3515
| SentryReporter::addBreadcrumb("cli.vertex-cache", | ||
| QString("Vertex-cache .%1%2").arg(fi.suffix(), rewrite ? " rewrite" : " analyze")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use a guideline-approved breadcrumb category for command invocation.
For this user-triggered action, use ui.action instead of a custom category to stay aligned with telemetry conventions.
Suggested fix
- SentryReporter::addBreadcrumb("cli.vertex-cache",
+ SentryReporter::addBreadcrumb("ui.action",
QString("Vertex-cache .%1%2").arg(fi.suffix(), rewrite ? " rewrite" : " analyze"));As per coding guidelines: “All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message) using categories: ui.action … file.import / file.export …”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SentryReporter::addBreadcrumb("cli.vertex-cache", | |
| QString("Vertex-cache .%1%2").arg(fi.suffix(), rewrite ? " rewrite" : " analyze")); | |
| SentryReporter::addBreadcrumb("ui.action", | |
| QString("Vertex-cache .%1%2").arg(fi.suffix(), rewrite ? " rewrite" : " analyze")); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/CLIPipeline.cpp` around lines 3514 - 3515, Replace the custom breadcrumb
category used when logging the vertex-cache command with the guideline-approved
category "ui.action": update the SentryReporter::addBreadcrumb call in
CLIPipeline.cpp (the call that currently uses "cli.vertex-cache" and constructs
the message with QString("Vertex-cache .%1%2").arg(fi.suffix(), rewrite ? "
rewrite" : " analyze")) to use "ui.action" as the first argument while leaving
the message construction unchanged so the event is recorded as a user action.
Comment on lines
+2809
to
+2825
| for (Ogre::SceneNode* node : Manager::getSingleton()->getSceneNodes()) { | ||
| if (!node) continue; | ||
| for (unsigned i = 0; i < node->numAttachedObjects(); ++i) { | ||
| Ogre::MovableObject* obj = node->getAttachedObject(i); | ||
| if (!obj || obj->getMovableType() != "Entity") continue; | ||
| auto* entity = static_cast<Ogre::Entity*>(obj); | ||
| const VertexCacheReport partial = | ||
| VertexCacheOptimizer::analyzeEntity(entity, rewrite); | ||
| for (const SubMeshCacheReport& sr : partial.submeshes) { | ||
| aggregate.submeshes.append(sr); | ||
| aggregate.totalTriangles += sr.triangleCount; | ||
| aggregate.weightedAcmrBefore += sr.acmrBefore * sr.triangleCount; | ||
| aggregate.weightedAcmrAfter += sr.acmrAfter * sr.triangleCount; | ||
| if (sr.reordered) ++aggregate.totalReordered; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Deduplicate by mesh resource before rewrite/aggregation.
This loop rewrites/analyzes per entity, but index buffers are shared at mesh level. With instanced entities, later iterations observe already-mutated buffers, making weighted ACMR “before” order-dependent and under-reporting improvements.
Please process each unique mesh once (for analyze/rewrite), then apply instance-count weighting in aggregation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MCPServer.cpp` around lines 2809 - 2825, The per-entity loop calls
VertexCacheOptimizer::analyzeEntity and rewrites shared index buffers repeatedly
for instanced entities, making "before" metrics order-dependent; instead
deduplicate by mesh resource (e.g., use entity->getMesh() or mesh name/handle as
the key) and call analyze/rewrite once per unique mesh, then for aggregation
multiply each SubMeshCacheReport's metrics by the instance count of that mesh
when updating aggregate.submeshes, aggregate.totalTriangles,
aggregate.weightedAcmrBefore/After and aggregate.totalReordered; ensure you
still iterate scene nodes to count instances but perform analyze/rewrite only on
the unique-mesh set.
Comment on lines
+66
to
+68
| if (indices.empty() || indices.size() % 3 != 0 || vertexCount == 0) return false; | ||
| if (cacheSize > kMaxCachePos) cacheSize = kMaxCachePos; | ||
|
|
There was a problem hiding this comment.
Reject non-positive cache sizes in forsyth().
cacheSize <= 0 is currently accepted, which can lead to invalid cache behavior. Add an early guard (or clamp) before reserve/use.
Suggested fix
bool VertexCacheOptimizer::forsyth(std::vector<uint32_t>& indices, uint32_t vertexCount,
int cacheSize)
{
if (indices.empty() || indices.size() % 3 != 0 || vertexCount == 0) return false;
+ if (cacheSize <= 0) return false;
if (cacheSize > kMaxCachePos) cacheSize = kMaxCachePos;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/VertexCacheOptimizer.cpp` around lines 66 - 68, In forsyth(), reject or
clamp non-positive cache sizes before any reserve/use to avoid invalid behavior:
check if cacheSize <= 0 and either return false or set cacheSize = 1 (whichever
matches project conventions) immediately after the early-validations (the block
that checks indices.empty(), indices.size() % 3 and vertexCount) and before any
use of cacheSize or calls that rely on it (references: cacheSize, kMaxCachePos,
indices, vertexCount, and the forsyth() function itself); also keep the existing
clamp to kMaxCachePos after this new guard.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e1d8c5a46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+385
to
+386
| "run `qtmesh vertex-cache --rewrite` or the MCP " | ||
| "optimize_vertex_cache tool to improve)") |
There was a problem hiding this comment.
The validation checklist tells users to run qtmesh vertex-cache --rewrite, but this command does not exist in cmdVertexCache (rewrite is enabled via -o <output>). Following the suggested fix path from the UI will therefore fail and block users from applying the optimization unless they discover the correct syntax elsewhere.
Useful? React with 👍 / 👎.
User feedback on slice C: the validator row reported the current ACMR but not how much a rewrite would actually save, which buried the call to action. VertexCacheOptimizer now runs Forsyth on a local copy even when rewrite=false, so the SubMeshCacheReport's `acmrAfter` always reflects what the optimized buffer would score. The `reordered` flag still only flips when bytes actually changed on disk — analyze-only mode never mutates Ogre's index buffer. MeshValidator uses the projected delta to pick the row type: - improvement >= 1% → info row "ACMR x.xxx → y.yyy (Z% improvement available — run `qtmesh vertex-cache -o <out>` …)" - improvement < 1% → ok row "ACMR x.xxx — already optimal" 1% is the cutoff so rounding noise doesn't surface a call-to- action when nothing meaningful would change. Manual smoke: `qtmesh vertex-cache ninja.mesh` (analyze-only) now prints ACMR 0.932 → 0.841 (9.7% improvement available) with "Submeshes rewritten: 0" confirming the buffer wasn't touched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User feedback: surfacing the projected ACMR improvement in the validator row is great, but the suggested fix still needs a terminal — "can we allow it to fix from the UI?". Adds an "Optimize Vertex Cache" button below the existing "Fix All" row in the Inspector's validation section. Distinct from Fix All on purpose: - Fix All re-imports through Assimp with cleanup flags (mutates geometry / topology). Shown when the row carries `fixable:true`. - Optimize Vertex Cache only rewrites index-buffer ordering via Forsyth — never touches positions / UVs / materials. Shown via a new `hasCacheOptimization` Q_PROPERTY when the validator computed a >=1% projected improvement. MeshValidator gains an `optimizeVertexCache()` Q_INVOKABLE that runs VertexCacheOptimizer::analyzeEntity(rewrite=true) on each selected entity, emits a `fixApplied` message with the actual before/after ACMR, then re-runs `validate()` so the checklist flips the row to "already optimal". The button uses the same blue (#5090d0) as the info-row glyph so the visual grouping is unambiguous — info row → blue action. Manual smoke: load ninja.mesh, click Run Validation → the cache row reports 9.7% improvement available; click Optimize Vertex Cache → row flips to "ACMR 0.841 — already optimal" and the fixApplied message confirms 1 submesh reordered. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ge (#498) User feedback: when the selection changes, the issue list already clears but the "Optimize Vertex Cache" button stays visible and the green "Reordered N submesh(es)…" feedback line lingers. Two fixes: - MeshValidator::selectionChanged handler now also resets m_cacheOptimizationAvailable so the new hasCacheOptimization Q_PROPERTY notifies false alongside hasFixableIssues — the button hides automatically. - fixFeedback Text in the validation section now listens for onIssuesChanged and clears itself whenever MeshValidator.validated is false. That covers selection-change, no-selection, and any future code path that resets the report. fixApplied / error messages still survive normal validate() runs because validated stays true through those. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ow match the row state (#498) User feedback: after slice C the vertex-cache row carries a one-click fix but the bottom context panel still shows "Findings: 0 / Fixable: No". Considered promoting the row from info → warning to make it count. Rejected: ACMR is a perf metric, not a correctness issue; promoting just this row would make the validator inconsistent with the sibling Draws / GPU info rows. Instead, two surgical fixes: 1. MeshValidator: mark the cache row `fixable: true` when the projected improvement is meaningful (>=1%). The dedicated "Optimize Vertex Cache" button reads `hasCacheOptimization` to appear, but the row's `fixable` flag is what the context panel reads to count "Fixable: Yes". To keep the red "Fix All (re-import with cleanup)" button from accidentally appearing on cache-only fixable cases (it does the Assimp re-import, not the vertex-cache reorder), `hasFixableIssues` now restricts itself to error / warning rows. Info-tier fixables have their own buttons (hasCacheOptimization for now; future slices add more). 2. BottomContextPanel: added a "Suggestions" column alongside "Findings". `Findings` still counts errors+warnings (the must-fix tier); `Suggestions` counts info rows that carry a one-click fix. Both update reactively on validate() / selection change. Visual result: validate a non-optimized mesh, the panel now reads "Findings: 0 Suggestions: 1 Fixable: Yes Status: Ready", and the "Optimize Vertex Cache" button is right there in the panel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two pieces of user feedback on slice C:
1. Context panel "Fixable" now reads Yes for the vertex-cache
suggestion. The QML reads MeshValidator.hasFixableIssues OR
hasCacheOptimization, mirroring how Suggestions counts info-tier
fixable rows separately from Findings.
2. qtmesh scan now flags poor vertex-cache locality.
- ScanConfig.maxAcmr (double, 0 = disabled). Loaded from
yml/json, scope overrides, and (via the qtmesh-cloud PR
landing alongside this) the project's `max_acmr` rule.
- AssetInfo.weightedAcmr computed via VertexCacheOptimizer::
computeAcmr on Assimp's flattened per-face triangle indices,
weighted by triangle count. Emitted in the per-asset JSON.
- evaluateRules adds a `max_acmr` warning when the asset's
weighted ACMR exceeds the configured ceiling.
- CLI flag --max-acmr <n>.
Design note (documented inline): the Assimp index order is NOT
the same as Ogre's MeshSerializer order, so the scan's ACMR runs
higher than the editor's "Run Validation" ACMR on the same asset
(e.g. ninja.mesh scans at 3.0 vs 0.93 in the editor). The scan
still catches the meshes that need a reorder; an Ogre-backed scan
backend that produces matching numbers is the deliberately-deferred
slice C2 (CLAUDE.md says ScanEngine is "lightweight metadata
extraction" on purpose, and switching wholesale costs ~500ms+ Ogre
init plus O(n) entity cleanup per file).
Manual smoke: `qtmesh scan media/models --max-acmr 0.8` warns on
the four high-ACMR fbx/mesh files and is silent on robot.mesh
(below threshold). JSON output carries `weightedAcmr` on every
mesh asset.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Quality Gate already passed; this clears the 24 issues flagged on
the new code by extracting helpers and applying mechanical const /
init-statement fixes throughout.
Structural refactors (S3776 cognitive complexity):
VertexCacheOptimizer.cpp / forsyth() — 59 → ~15
- Extracted buildTriangleAdjacency, cachePushFront,
enforceCacheCapacity, triangleScore, and findNextBestTriangle
helpers. The main loop is now a six-line state machine:
emit → push-to-cache → evict → re-score cached verts → pick
next best.
- ScoreTable's 32-entry c-array swapped for std::array<float, 33>.
- vertexScore's nested ternary on cache-position split into a
named cachePositionScore() so each branch reads independently.
- Out-of-range index check moved to a single pre-loop pass before
any allocation, dropping a branch in the hot CSR-build path.
VertexCacheOptimizer.cpp / analyzeEntity() — 52 → ~10
- Extracted readIndexBuffer / writeIndexBuffer (the 16/32-bit
index lock/copy/unlock dance) and analyzeSubMesh (per-submesh
Forsyth + writeback) into anonymous-namespace helpers.
- New public helpers mergeReport() / finalize() collapse the
multi-entity aggregation that was duplicated in MCP and CLI
callers; both call sites are now three lines.
CLIPipeline.cpp / cmdVertexCache() — 33 → ~15
- Extracted parseVertexCacheArgs, exportRewrittenMesh,
emitVertexCacheReport into anonymous-namespace helpers (same
pattern as cmdMemory in slice A). cmdVertexCache is now a thin
orchestrator.
Mechanical fixes (S5350 / S6004 / S5827 / S5276):
- const Ogre::SceneNode* / const Ogre::Entity* in scene walks.
- const Ogre::SubMesh* / IndexData* / VertexData* in the per-
submesh helper.
- Init-statement on the cache.find() in computeAcmr's inner loop.
- ScanEngine's tris cast now uses `const auto` (mirrors slice B's
pattern for static_cast assignments).
- Removed redundant `static` on the ScoreTable singleton (it's
already in an anonymous namespace, so static is redundant).
The remaining MCPServer.cpp:2798 S5817 ("should be const") is
the same intentional NOSONAR pattern as slice A's get_memory_usage
and slice B's analyze_draw_calls — ToolHandler is a non-const
member-fn pointer.
Manual smoke: qtmesh vertex-cache media/models/ninja.mesh -o out
still produces ACMR 0.932 → 0.841 (9.7%) with 1/2 submeshes
reordered. UnitTests link clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/VertexCacheOptimizer.cpp (1)
162-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
cacheSize <= 0before using it.This still accepts zero/negative cache sizes, and Line 192 later feeds that value into
reserve(...)whileenforceCacheCapacity(...)can hitresize(cacheSize). A negative input can therefore turn into a huge allocation instead of a clean failure.Suggested fix
bool VertexCacheOptimizer::forsyth(std::vector<uint32_t>& indices, uint32_t vertexCount, int cacheSize) { if (indices.empty() || indices.size() % 3 != 0 || vertexCount == 0) return false; + if (cacheSize <= 0) return false; if (cacheSize > kMaxCachePos) cacheSize = kMaxCachePos;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/VertexCacheOptimizer.cpp` around lines 162 - 163, The code currently clamps cacheSize only above kMaxCachePos but permits zero/negative values; before using cacheSize (near the existing checks that reference indices, vertexCount, and kMaxCachePos), explicitly reject non-positive cache sizes by returning false (or otherwise failing) when cacheSize <= 0 so later calls that do reserve(...) or enforceCacheCapacity(...)->resize(cacheSize) cannot receive zero/negative values; update the check around the block containing indices.empty(), vertexCount, cacheSize and kMaxCachePos to include this validation for cacheSize.src/MCPServer.cpp (1)
2808-2819:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftProcess each shared mesh once before aggregating or rewriting.
This still analyzes by entity, but Ogre entities can share the same mesh/index buffers. With
rewrite=true, the first instance mutates the shared buffer and later instances observe the already-reordered data, so the reported “before” ACMR and total improvement become scene-order dependent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MCPServer.cpp` around lines 2808 - 2819, The loop analyzes per-entity which reprocesses shared Ogre meshes and causes scene-order dependent results when rewrite=true; fix by deduplicating work per unique mesh: keep a set of processed mesh identifiers (e.g., entity->getMesh() pointer or entity->getMesh()->getName()) while iterating Manager::getSingleton()->getSceneNodes() and only call VertexCacheOptimizer::analyzeEntity (and then VertexCacheOptimizer::mergeReport into aggregate) the first time you encounter that mesh; this ensures each shared mesh is analyzed/rewritten once before VertexCacheOptimizer::finalize(aggregate) is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/MCPServer.cpp`:
- Around line 2808-2819: The loop analyzes per-entity which reprocesses shared
Ogre meshes and causes scene-order dependent results when rewrite=true; fix by
deduplicating work per unique mesh: keep a set of processed mesh identifiers
(e.g., entity->getMesh() pointer or entity->getMesh()->getName()) while
iterating Manager::getSingleton()->getSceneNodes() and only call
VertexCacheOptimizer::analyzeEntity (and then VertexCacheOptimizer::mergeReport
into aggregate) the first time you encounter that mesh; this ensures each shared
mesh is analyzed/rewritten once before VertexCacheOptimizer::finalize(aggregate)
is called.
In `@src/VertexCacheOptimizer.cpp`:
- Around line 162-163: The code currently clamps cacheSize only above
kMaxCachePos but permits zero/negative values; before using cacheSize (near the
existing checks that reference indices, vertexCount, and kMaxCachePos),
explicitly reject non-positive cache sizes by returning false (or otherwise
failing) when cacheSize <= 0 so later calls that do reserve(...) or
enforceCacheCapacity(...)->resize(cacheSize) cannot receive zero/negative
values; update the check around the block containing indices.empty(),
vertexCount, cacheSize and kMaxCachePos to include this validation for
cacheSize.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 954774a6-9e2c-474b-879e-0259b4d9373e
📒 Files selected for processing (5)
src/CLIPipeline.cppsrc/MCPServer.cppsrc/ScanEngine.cppsrc/VertexCacheOptimizer.cppsrc/VertexCacheOptimizer.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/VertexCacheOptimizer.h
- src/CLIPipeline.cpp
- src/ScanEngine.cpp
…498) Adds end-user documentation for slices A/B/C to the website DocsApp, mirroring the existing CmdSection / RuleCard conventions and adding a new Performance section that covers concepts and ties the three new CLI commands together. New NAV group "Performance" with four entries: - Concepts: a long-form explanation of GPU memory, draw calls, and ACMR — what each metric measures, how the validator computes it, and a "what numbers should I see?" table for ACMR (~0.5 optimal, >2 needs reorder). - memory: full CmdSection with synopsis, all flags (--budget / --token / --no-cloud), example text output, and a paragraph explaining the JSON shape returned by the CLI and the MCP get_memory_usage tool. - analyze: CmdSection with synopsis, example output, and the "Reading the report" note explaining what the After-merges number means alongside Draw calls. - vertex-cache: CmdSection covering analyze-only vs rewrite-and- export, the "never regresses" guarantee, the Inspector workflow with the Optimize Vertex Cache button, and the MCP rewrite arg. Scan Reference additions: - New "Performance Rules" subsection under scan-rules with a RuleCard for max_acmr. Documents the Assimp-vs-Ogre index-order discrepancy so users don't get confused when scan numbers don't match the in-app validator — the calibration suggestion is 1.5 on the scan side. Links back to Performance Concepts. - --max-acmr CLI flag added to the scan options table. - max_acmr added to the YAML schema example with a hint pointing at Performance Concepts. Build verified manually: 11 <CmdSection > tags balance (6 closing + 5 self-closing), 15 / 15 <section> tags balance. The local vite build refuses to run on Node 20.18 (needs ≥20.19) so the actual bundle gets verified by CI's website build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clears 4 of the 6 remaining S5350 / S995 minors flagged by Sonar after the post-merge attribution: - CLIPipeline::cmdVertexCache: `entities` reference is now `const auto&` (the loop only reads; no add/remove). - exportRewrittenMesh: `entity` / `node` are now pointer-to-const (MeshImporterExporter::exporter takes const SceneNode* already, the read-only path is straight-through). - VertexCacheOptimizer::readIndexBuffer: takes `const IndexData*` (locks read-only). The remaining S995 on writeIndexBuffer/analyzeEntity are not really fixable: both functions exist specifically to mutate Ogre buffers through the supplied IndexData* / Entity* pointer when rewrite=true. A pointer-to-const there would force a const_cast inside or change the API contract — left as-is. The MAJOR S5817 on toolOptimizeVertexCache stays NOSONAR'd for the same reason as the slice A / B tool methods: ToolHandler is a non-const member-fn pointer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 6 slice C (#261) — vertex cache optimization. Same shape as slices A/B (pure-data analyzer + CLI + MCP + inspector validator row).
VertexCacheOptimizer(new): Tom Forsyth's linear-time vertex-cache optimization plus an ACMR (Average Cache Miss Ratio) calculator. ~300 LoC of pure-data implementation, no external dep — chose to write Forsyth inline over pulling meshoptimizer because we only need this one routine. Uses a 32-entry post-T&L cache model (Forsyth's recommended default).qtmesh vertex-cache <file> [-o <output>] [--json]: analyze-only by default; with-o, rewrites each SubMesh's index buffer in place and exports to a new file. Only writes back when the new ACMR is strictly lower — never regresses.optimize_vertex_cachewithrewrite: true|falsearg. Returns the text summary plus a structuredvertexCachepayload (per-submesh ACMR before/after + weighted totals).Vertex cache: ACMR <n>info row, pointing the user at the CLI / MCP for the actual reorder action so validation stays read-only.Sample output
(Submesh 1 was already near-optimal at ACMR 0.587 — Forsyth's heuristic correctly skips the rewrite because the candidate order would not improve.)
Test plan
VertexCacheOptimizerTest.*pure-data tests covering:MeshValidatorTest.DoValidateValidMeshReportsChecklistextended to assert the new "Vertex cache:" row appearsqtmesh vertex-cacheanalyze + rewrite produce expected ACMR delta; JSON output validatessrc/CMakeLists.txtandtests/CMakeLists.txt🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores