feat(perf): GPU memory & VRAM reporting (Phase 6 slice A) - #494
Merged
Conversation
Adds a pure-data MemoryEstimator that walks Ogre's mesh vertex/index buffers and the live texture pool to produce a SceneMemoryReport (per-mesh bytes, per-texture bytes, totals, optional budget warning). Surfaced through: - MeshInfoOverlay — appends "GPU: <bytes>" to the floating mesh-info panel so the value is visible in the editor itself. - MCP tool `get_memory_usage` — returns the formatted summary plus a compact JSON payload for LLM clients. - CLI subcommand `qtmesh memory <file> [--json] [--budget <size>]` with non-zero exit when the budget is exceeded, for CI pipelines. Closes the first acceptance criterion of issue #261 (memory usage panel with per-asset breakdown and configurable budgets). Tests cover the byte math, budget parsing, JSON/text serialisation, and the new overlay line. Ogre-backed paths are guarded by tryInitOgre() per existing project conventions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a MemoryEstimator library that computes GPU/VRAM for Ogre scenes, integrates it into a new ChangesGPU Memory Estimation
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLIPipeline::cmdMemory
participant Estimator as MemoryEstimator
participant Ogre as OgreScene
participant Output as JSON/Text
User->>CLI: qtmesh memory file.mesh [--json] [--budget 512M]
CLI->>Estimator: parseBudget("512M")
Estimator-->>CLI: budgetBytes
CLI->>Ogre: initialize headless + load mesh via importer
CLI->>Estimator: estimateScene(budgetBytes)
Estimator->>Ogre: iterate scene nodes and entities
Estimator->>Ogre: read mesh vertex/index buffers
Estimator->>Ogre: enumerate textures and mipmaps
Estimator-->>CLI: SceneMemoryReport (meshes, textures, totals, budget, overBudget)
alt JSON mode
CLI->>Estimator: toJson(report)
Estimator-->>Output: JSON object
else Text mode
CLI->>Estimator: toText(report)
Estimator-->>Output: Formatted text summary
end
CLI-->>User: Output + exit code (0 if under budget, 1 if over)
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: 1
🧹 Nitpick comments (2)
src/MemoryEstimator.cpp (1)
169-169: 💤 Low valueConsider using unsigned loop counter for type correctness.
The loop uses
intwhilenumAttachedObjects()likely returnsunsigned intorsize_t. Usingunsigned int iwould be more type-correct and avoid the cast.Optional refactor
- for (int i = 0; i < static_cast<int>(node->numAttachedObjects()); ++i) { + for (unsigned int i = 0; i < node->numAttachedObjects(); ++i) { Ogre::MovableObject* obj = node->getAttachedObject(i);🤖 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/MemoryEstimator.cpp` at line 169, The loop in MemoryEstimator.cpp that iterates "for (int i = 0; i < static_cast<int>(node->numAttachedObjects()); ++i)" should use an unsigned loop counter matching node->numAttachedObjects() to avoid the signed/unsigned mismatch; change the loop header to use an unsigned type (e.g., size_t or unsigned int or auto) for "i" and remove the static_cast on node->numAttachedObjects(), keeping the loop condition directly comparing like types (refer to the loop using node->numAttachedObjects()).src/CLIPipeline.cpp (1)
3294-3298: ⚡ Quick winClarify budget parsing error message.
The error message could be more specific about why budget parsing failed. Since
parseBudgetreturns0for both invalid formats and explicit zero values, users who type--budget 0MBwill see "Invalid --budget value" without understanding that zero is not allowed (or that they should omit--budgetfor unlimited).Suggested improvement
if (arg == "--budget" && i + 1 < argc) { budgetBytes = MemoryEstimator::parseBudget(argv[++i]); if (budgetBytes == 0) { - err() << "Error: Invalid --budget value (use e.g. 50MB, 1GB)" << Qt::endl; + err() << "Error: Invalid --budget value. Use a positive size (e.g. 50MB, 1GB) or omit --budget for unlimited." << Qt::endl; return 2; } 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 3294 - 3298, The error handling for budget parsing conflates an invalid format and an explicit zero value because parseBudget returns 0 for both; update the check after calling parseBudget(argv[++i]) to inspect the original argument string and produce a clearer message: if budgetBytes == 0 and the input string represents zero (e.g., "0", "0MB", "0GB", case-insensitive, or numeric 0) call err() with "Error: --budget value of zero is not allowed; omit --budget for unlimited" otherwise call err() with "Error: Invalid --budget value (use e.g. 50MB, 1GB)". Use the existing identifiers parseBudget, budgetBytes, err(), and argv[++i] to locate and implement this change.
🤖 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.
Nitpick comments:
In `@src/CLIPipeline.cpp`:
- Around line 3294-3298: The error handling for budget parsing conflates an
invalid format and an explicit zero value because parseBudget returns 0 for
both; update the check after calling parseBudget(argv[++i]) to inspect the
original argument string and produce a clearer message: if budgetBytes == 0 and
the input string represents zero (e.g., "0", "0MB", "0GB", case-insensitive, or
numeric 0) call err() with "Error: --budget value of zero is not allowed; omit
--budget for unlimited" otherwise call err() with "Error: Invalid --budget value
(use e.g. 50MB, 1GB)". Use the existing identifiers parseBudget, budgetBytes,
err(), and argv[++i] to locate and implement this change.
In `@src/MemoryEstimator.cpp`:
- Line 169: The loop in MemoryEstimator.cpp that iterates "for (int i = 0; i <
static_cast<int>(node->numAttachedObjects()); ++i)" should use an unsigned loop
counter matching node->numAttachedObjects() to avoid the signed/unsigned
mismatch; change the loop header to use an unsigned type (e.g., size_t or
unsigned int or auto) for "i" and remove the static_cast on
node->numAttachedObjects(), keeping the loop condition directly comparing like
types (refer to the loop using node->numAttachedObjects()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab768292-537f-4510-8d36-4425b2402940
📒 Files selected for processing (11)
src/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MemoryEstimator.cppsrc/MemoryEstimator.hsrc/MemoryEstimator_test.cppsrc/MeshInfoOverlay.cppsrc/MeshInfoOverlay_test.cppsrc/main.cpp
… CR feedback CI failure on #494: MaterialEditorQML_perf_test / _qml_test linked against MCPServer / CLIPipeline / MeshInfoOverlay sources but the new MemoryEstimator.cpp was missing from tests/CMakeLists.txt's source list, producing undefined references on the GCC link stage. Added it alongside TextureChannelPacker.cpp / NormalMapGenerator.cpp. CodeRabbit feedback addressed in the same commit (keep the diff small): - MCP get_memory_usage now returns the SceneMemoryReport as a structured `memory` field in the result envelope rather than appending a JSON string to the human summary. Machine consumers no longer have to text-parse the response. - MemoryEstimator::estimateScene now iterates node->numAttachedObjects() with an unsigned counter, matching the Ogre API and dropping the static_cast<int> noise. - qtmesh memory --budget error message clarifies that omitting the flag means unlimited (was ambiguous on `--budget 0`). Also adds cloud-rules integration the user asked for: when --budget is not passed and a QtMesh Cloud token is available (--token / QTMESH_TOKEN / QTMESH_CLOUD_TOKEN), `qtmesh memory` GETs /v1/ingest/rules and reads rules.memory_budget_mb from the project config. --no-cloud opts out. The selected source is recorded in Sentry breadcrumbs and in --json output (budgetSource: "cli" | "cloud:<source>"). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit review hint: the tool description should advertise the structured payload now that the response carries both text and JSON. Clarifies that machine consumers should read result["memory"] for per-mesh, per-texture, totals, and budget fields rather than parsing the human-readable summary in `content`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Line 3371: Before calling MeshImporterExporter::importer, add a Sentry
breadcrumb to record the file import; specifically call
SentryReporter::addBreadcrumb with category "file.import" and a message
containing the path (use fi.absoluteFilePath() or the same string passed to
importer) immediately before the
MeshImporterExporter::importer({fi.absoluteFilePath()}, 0) invocation so the I/O
operation is tracked per guidelines.
- Around line 3300-3314: The code currently silently ignores trailing "--token"
or "--budget" with no following value; update the CLI parsing in CLIPipeline.cpp
to detect when arg == "--token" or arg == "--budget" and i+1 >= argc and in that
case print a usage/error message via err() << ... << Qt::endl and return 2,
rather than continuing; keep existing behavior when a value is present (set
tokenArg = QString::fromLocal8Bit(argv[++i]) for "--token", and parse with
MemoryEstimator::parseBudget(argv[++i]) for "--budget", check budgetBytes == 0
and set budgetExplicit = true).
🪄 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: bb4aba2e-096b-49f2-9ee9-8483acb8b8e6
📒 Files selected for processing (4)
src/CLIPipeline.cppsrc/MCPServer.cppsrc/MemoryEstimator.cpptests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- tests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MemoryEstimator.cpp
| budgetBytes > 0 ? QString(" budget=%1B").arg(budgetBytes) : QString(), | ||
| budgetSource.isEmpty() ? QStringLiteral("none") : budgetSource)); | ||
|
|
||
| MeshImporterExporter::importer({fi.absoluteFilePath()}, 0); |
There was a problem hiding this comment.
Add file.import breadcrumb for memory command file load
Line 3371 performs a significant file I/O operation, but it is only tracked under cli.memory. Add a file.import breadcrumb before the importer call for guideline compliance.
Suggested patch
+ SentryReporter::addBreadcrumb("file.import",
+ QString("Importing file %1").arg(fi.absoluteFilePath()));
MeshImporterExporter::importer({fi.absoluteFilePath()}, 0);As per coding guidelines: "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message) ... 'file.import' / 'file.export' for I/O operations"
📝 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
| MeshImporterExporter::importer({fi.absoluteFilePath()}, 0); | |
| SentryReporter::addBreadcrumb("file.import", | |
| QString("Importing file %1").arg(fi.absoluteFilePath())); | |
| MeshImporterExporter::importer({fi.absoluteFilePath()}, 0); |
🤖 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` at line 3371, Before calling
MeshImporterExporter::importer, add a Sentry breadcrumb to record the file
import; specifically call SentryReporter::addBreadcrumb with category
"file.import" and a message containing the path (use fi.absoluteFilePath() or
the same string passed to importer) immediately before the
MeshImporterExporter::importer({fi.absoluteFilePath()}, 0) invocation so the I/O
operation is tracked per guidelines.
Sonar Quality Gate was already passing; this clears the 16 issues flagged on the new code: CLIPipeline.cpp / cmdMemory - S3776 / S134: cognitive complexity 44 → ~15, max nesting depth back below the threshold. Extracted parseMemoryArgs, applyCloudBudget, and emitMemoryReport into an anonymous-namespace block. cmdMemory is now a thin orchestrator that delegates parsing, cloud lookup, and serialisation. - S886 / S5350 / S6004: tightened `for (int i = …)` parsing loop with const-correct locals, moved `entities` into an if-init-statement and dropped the redundant reference binding. MCPServer.cpp / toolGetMemoryUsage - S5350 / S6004: Manager* → const Manager* via if-init-statement; `spec` declared const. Left the method non-const with an inline note: ToolHandler is a non-const member-fn pointer (every other tool method in this class follows the same convention, so flipping just this one would break the registry signature). MemoryEstimator.cpp - S5276: formatBytes now stores 1024/1024² constants as quint64 and casts to double at the division site, so the comparison stays in integer space and the precision-loss warnings go away. - S995 / S5350: estimateEntity's accumulateVertexData takes a pointer-to-const VertexData; estimateScene iterates with const Ogre::SceneNode* / const Ogre::Entity*. - S5276: vertex stride stored as size_t to match Ogre's API return type instead of narrowing to unsigned int at the call site. No behaviour changes; manual smoke test reruns of `qtmesh memory robot.mesh --budget 10KB` still produce the expected text report, JSON payload, and exit-1 over-budget signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the first refactor pass dropped issues 16 → 4, this clears the final three actionable items: - MemoryEstimator.cpp:42 (S6004): hoist KB/MB/GB constexpr quint64 constants to anonymous namespace scope so each if's first condition can read them directly without a per-branch init-statement. - MemoryEstimator.cpp:103 (S5350): SubMesh* → const SubMesh* in estimateEntity's submesh loop. - CLIPipeline.cpp:3302 (S886): rewrite parseMemoryArgs's for loop as an index-driven while, so the in-body ++i (used to consume the argument of --token / --budget) no longer mutates the loop variable Sonar tracks. The remaining MCPServer.cpp:2737 "should be const" finding is intentional and now flagged with a NOSONAR(cpp:S5817) comment: the class's ToolHandler is a non-const member-function pointer, so flipping just one tool method would break the registry signature. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
8 tasks
fernandotonon
added a commit
that referenced
this pull request
May 12, 2026
…497) * feat(perf): draw-call analysis + merge suggestions (Phase 6 slice B) 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> * refactor(perf): address CodeRabbit + SonarCloud findings on slice B (#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> * feat(validate): per-check feedback rows + draw-call & memory analyses (#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> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Kicks off Phase 6 (Mesh Optimization Pipeline, #261) with the lowest-risk feature: per-mesh GPU memory + per-texture VRAM estimation. No new external dependencies, no UI restructure — extends existing surfaces.
MemoryEstimator(new): pure-data utility that walks Ogre vertex/index buffers and the live texture pool, returning aSceneMemoryReport(per-asset bytes, totals, optional budget warning).MeshInfoOverlay: now appends aGPU: 17.9 KBline to the floating mesh-info panel.get_memory_usage: returns the text summary plus a compact JSON payload (for LLM consumers).qtmesh memory <file> [--json] [--budget <size>]: non-zero exit when over budget — for CI pipelines.Closes the memory & VRAM panel acceptance criterion of #261. Six more slices to go (B: draw call analysis, C: vertex cache opt, D: decimation polish, E: texture atlas, F: Draco, G: master
qtmesh optimize).Why this order
MeshInfoOverlay/CLI/MCP scaffolding.MemoryEstimator) is pure data, fully unit-tested without Ogre.Test plan
MemoryEstimatorTest.*— 17 pure-data tests (byte math, budget parsing, JSON/text serialisation, over-budget flag). Run on every CI build.MeshInfoOverlayIntegrationTest.FormatStatsIncludesGpuBytes— new GPU line surfaces in the overlay text.qtmesh memory media/models/robot.mesh→ text report;--json→ structured payload;--budget 10KB→ exits 1;--budget 50MB→ exits 0.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests