feat(perf): draw-call analysis + merge suggestions (Phase 6 slice B) - #497
Merged
Conversation
Builds the second pillar of the optimization pipeline on top of Slice A's reporting scaffold (#494). DrawCallAnalyzer (new) — pure-data analyzer. Takes any list of Ogre::Entity* and produces a DrawCallReport: - totals: entities, submeshes, draw calls (1 per SubEntity), unique materials, projected draw-call count after merging, total savings; - clusters: every material plus the entities that use it and the draw-call savings unlocked by merging them; - suggestions: clusters with >=2 entities, ranked by savings. Surfaced through: - MeshInfoOverlay — appends "Draws: N (save K by merging)" so the overlay shows the merge potential at a glance, in sync with the current selection. - MCP tool analyze_draw_calls — returns the formatted summary in the standard content field plus a structured `drawCalls` payload (the same shape as get_memory_usage from slice A). - CLI subcommand `qtmesh analyze <file> [--json]` for headless / CI workflows. The one-click merge action listed in #261's slice B scope is deliberately deferred: it is a write operation that touches the undo system, material/skeleton remapping, and submesh combination logic. The analysis alone (the harder pure-data work) is the bulk of the value — the merge suggestion in the JSON already tells users or automation which entities to combine. Tests: 12 DrawCallAnalyzerTest cases cover the byte math, suggestion filtering, JSON/text serialisation, and null-entity handling. The new MeshInfoOverlay test confirms the "Draws:" line appears. Ogre- backed paths follow the existing tryInitOgre() pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds DrawCallAnalyzer (data model + analysis) to compute per-material draw-call costs and merge suggestions; integrates it into a new CLI ChangesDraw-Call Analysis Feature
Sequence Diagram(s)sequenceDiagram
participant CLI
participant MCPServer
participant MeshInfoOverlay
participant MeshValidator
participant DrawCallAnalyzer
CLI->>DrawCallAnalyzer: analyze(entities from imported file)
MCPServer->>DrawCallAnalyzer: analyzeScene()
MeshInfoOverlay->>DrawCallAnalyzer: analyze(validEntities)
MeshValidator->>DrawCallAnalyzer: analyze(validEntities)
DrawCallAnalyzer->>DrawCallAnalyzer: cluster by material (collect submesh counts, entity names)
DrawCallAnalyzer->>DrawCallAnalyzer: compute mergeSavings per cluster
DrawCallAnalyzer->>CLI: toJson(report) / toText(report)
DrawCallAnalyzer->>MCPServer: QJsonObject + text content
DrawCallAnalyzer->>MeshInfoOverlay: QString formatted report
DrawCallAnalyzer->>MeshValidator: DrawCallReport for checklist row
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 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: 3
🤖 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 3452-3456: The analyze import step performs file I/O but only logs
"cli.analyze"; add a file.import breadcrumb immediately before calling
MeshImporterExporter::importer so the operation is tracked: call
SentryReporter::addBreadcrumb("file.import", QString("Import
%1").arg(fi.absoluteFilePath())) (or a similar message referencing
fi.suffix()/fi.absoluteFilePath()) right before
MeshImporterExporter::importer({fi.absoluteFilePath()}, 0) so Sentry records the
file import action.
- Around line 3468-3470: The text output path is missing the analyzed filename;
when writing human-readable text you should include the report.file value so
text and JSON outputs are consistent—either update
DrawCallAnalyzer::toText(const Report& report) to prepend/format report.file
into the returned string, or change the caller (where
cliWrite(DrawCallAnalyzer::toText(report)) is used) to call cliWrite by
composing the filename and the text (e.g., format "filename: " +
DrawCallAnalyzer::toText(report)); ensure you reference the Report's file member
when assembling the output so CI logs can correlate entries with files.
In `@src/DrawCallAnalyzer.cpp`:
- Around line 9-11: The file uses QSet<QString> (symbol: QSet<QString>) but
doesn't explicitly include <QSet>, relying on transitive includes; add an
explicit include for <QSet> in the header includes at the top of
DrawCallAnalyzer.cpp (alongside existing includes like <QHash> and <QJsonArray>)
so the code compiles robustly across configurations.
🪄 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: aac99eab-bf3d-4d68-9de0-712abc114330
📒 Files selected for processing (12)
src/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/DrawCallAnalyzer.cppsrc/DrawCallAnalyzer.hsrc/DrawCallAnalyzer_test.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/MeshInfoOverlay.cppsrc/MeshInfoOverlay_test.cppsrc/main.cpptests/CMakeLists.txt
…497) Quality Gate already passed; this clears the open issues: CodeRabbit - Added a `file.import` Sentry breadcrumb in cmdAnalyze immediately before the import call, matching the project convention for I/O operations (CLAUDE.md guidance). - Text output now prepends `File: <name>` so it mirrors the JSON's `file` field — same correlation in CI logs. - Added explicit #include <QSet> in DrawCallAnalyzer.cpp instead of relying on transitive includes. SonarCloud - S5276 (size_t → unsigned int): switched the SubEntity iteration in analyze() to size_t to match Ogre::Entity::getNumSubEntities()'s return type. - S5276 (qsizetype → int): cast Container::size() at the assignment site in mergeSavings() and the uniqueMaterials accumulator. - S5350: const Ogre::SceneNode* in the analyzeScene scene-graph walk. - S6004: drawReport now declared inside an if-init-statement in MeshInfoOverlay::formatStats. The remaining MCPServer.cpp:2776 S5817 (`should be const`) is the same intentional non-const tool method covered by the existing NOSONAR comment in slice A. MCPServer.cpp:3394 S1116 is a pre-existing Q_UNUSED(args); in toolGetPivotMode that Sonar mis-attributed to the new code window — not touched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…#497) User feedback on the slice B PR: "Run Validation" today only performs the three geometry/UV checks and reports a bare "No issues found." line when they all pass, hiding the scope of the analysis from the user. This commit: - Folds the new draw-call (slice B) and GPU-memory (slice A) analyses into the validator's flow, so a single click reports everything we can derive without exporting the mesh. - Replaces the single-line success message with a per-dimension checklist. Every check now emits its own row in the issues list — errors and warnings stay as before, passing checks become "ok" rows ("Geometry: 1,234 triangle(s) across 3 submesh(es), no degenerate faces"), and neutral observations become a new "info" row type ("Draws: 5 — save 3 by merging", "GPU: ~245 KB of vertex + index buffers"). - QML icon palette extended for the new "info" type (ℹ blue). Why include this on slice B's branch instead of a separate PR: the validator change is what makes the slice B feature land in the single place users already know to click for asset health, and it shares the analyzers introduced in slice B (DrawCallAnalyzer) and slice A (MemoryEstimator). Tests: refreshed two MeshValidatorTest cases to assert the new checklist shape (geometry / UVs / draws / GPU rows present, none of them error/warning on a clean mesh). Existing DoValidateDetectsDegeneratesAndUvProblems narrowed its substring match to the still-present words ("non-finite", "extreme values") to match the new "Geometry: " / "UVs: " row prefixes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/MeshValidator.cpp`:
- Around line 178-180: The vertex total is double-counting when multiple
submeshes share the same Ogre::VertexData (vd); change the accumulation logic so
you only add vd->vertexCount once per unique Ogre::VertexData pointer: create a
local container (e.g., std::unordered_set<const Ogre::VertexData*>) in the scope
that computes totalVerts, check if vd is already present before doing totalVerts
+= static_cast<int>(vd->vertexCount), and insert vd when first seen; keep the
existing ++totalSubmeshes and totalTris logic unchanged and include the
necessary header for the chosen container.
🪄 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: cff896e6-cc6b-4af2-bb47-a3a808c36360
📒 Files selected for processing (3)
qml/PropertiesPanel.qmlsrc/MeshValidator.cppsrc/MeshValidator_test.cpp
Comment on lines
+178
to
+180
| ++totalSubmeshes; | ||
| totalVerts += static_cast<int>(vd->vertexCount); | ||
| totalTris += static_cast<int>(id->indexCount / 3); |
There was a problem hiding this comment.
Avoid double-counting shared vertex data in the vertex total
Line 179 adds vd->vertexCount per submesh. When multiple submeshes share the same Ogre::VertexData, the reported vertex count is inflated (and the GPU summary at Line 373 becomes misleading).
Suggested fix
+#include <QSet>
...
- for (Ogre::Entity* entity : targets) {
+ for (Ogre::Entity* entity : targets) {
+ QSet<const Ogre::VertexData*> countedVertexData;
Ogre::MeshPtr mesh = entity->getMesh();
if (!mesh) continue;
...
- totalVerts += static_cast<int>(vd->vertexCount);
+ if (!countedVertexData.contains(vd)) {
+ countedVertexData.insert(vd);
+ totalVerts += static_cast<int>(vd->vertexCount);
+ }🤖 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/MeshValidator.cpp` around lines 178 - 180, The vertex total is
double-counting when multiple submeshes share the same Ogre::VertexData (vd);
change the accumulation logic so you only add vd->vertexCount once per unique
Ogre::VertexData pointer: create a local container (e.g.,
std::unordered_set<const Ogre::VertexData*>) in the scope that computes
totalVerts, check if vd is already present before doing totalVerts +=
static_cast<int>(vd->vertexCount), and insert vd when first seen; keep the
existing ++totalSubmeshes and totalTris logic unchanged and include the
necessary header for the chosen container.
|
5 tasks
2 tasks
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 B (#261) — builds on Slice A (#494) using the same shape:
DrawCallAnalyzer(new): pure-data analyzer. Groups every entity by the materials its SubEntities use, counts one draw call per SubEntity, and produces ranked merge suggestions (clusters where N≥2 entities share a material — saving N−1 draw calls per cluster).Draws: 5 (save 3 by merging)to the floating mesh-info panel.analyze_draw_calls: returns the formatted summary plus a structureddrawCallsJSON payload.qtmesh analyze <file> [--json]: headless/CI surface.No issues found.line.Closes the draw-call analysis overlay + merge suggestions acceptance criterion of #261. One-click merge is deliberately deferred — it's a write operation that touches the undo system, material/skeleton remapping, and submesh combination, none of which the analysis itself needs.
Run Validation feedback before / after
Before (single line on success):
✔ No issues found.After (one row per analyzed dimension):
Errors / warnings keep their existing presentation, just with a
Geometry:/UVs:prefix for consistency. A newinfoicon (ℹ blue) covers neutral observations that aren't pass/fail.Sample CLI output
Test plan
DrawCallAnalyzerTest.*pure-data tests (cluster grouping, suggestion filtering, merge-savings arithmetic, JSON/text serialisation, null-entity handling). Run on every CI build.MeshInfoOverlayIntegrationTest.FormatStatsIncludesDrawCalls— overlay'sDraws:line appears for valid entities.MeshValidatorTest.DoValidateValidMeshReportsChecklist— the validator emits Geometry/UVs/Draws/GPU rows for a clean mesh, none of them error/warning.MeshValidatorTest.DoValidateDetectsDegeneratesAndUvProblems— still catches the three failure modes after the row-prefix rename.qtmesh analyzeon robot.mesh / ninja.mesh produces correct text and JSON; exit 0.src/CMakeLists.txt(main editor) andtests/CMakeLists.txt(MaterialEditorQML test executables) — slice A regression check.🤖 Generated with Claude Code