chore(quality): harden update checker + raise coverage / cut duplication - #532
Merged
Conversation
…pplyAtlas tests Quality drive — coverage from 70.4%, duplication 4.1%. Goal: 80% / 3%. Update checker (PR #531 issue follow-up) - New UpdateVersion module (header + cpp + tests). Normalises a leading v / V, strips semver pre-release / build suffixes, and compares with QVersionNumber so 3.10.0 ranks above 3.2.0. The old `latestVersion == currentVersion` string compare in the mainwindow update flow misclassified the v-prefix case AND the multi-digit case; both are now correct. - Update prompt only fires when the remote version is *strictly newer*. Invalid responses (latest = "main") fall through to a log line instead of nagging the user. Prompt now shows both versions so the user can see what's offered. - 16 unit tests cover normalize / compare / isUpdateAvailable across the v-prefix, suffix, multi-digit, and malformed-input paths. MCP duplication trim - Added MCPServer::runOgreOp template helper that wraps an Ogre body and translates Ogre::Exception / std::exception into the standard "Error: …" makeErrorResult. Sonar flagged 33 copies of the same try/catch in this file (973 duplicated lines). - Migrated 4 representative tool handlers — toolGetMaterial, toolListMaterials, toolApplyMaterial, toolCreateMaterial — to use runOgreOp. Each call site shrinks ~5 lines; the rest of the migration is a follow-up. ApplyAtlas coverage - 9 new tests for parseManifestJson edge cases (empty JSON, root-not-object, missing tiles, non-object tile entries, negative dimensions) and ApplyReport JSON shape (failure state, every per-submesh field, empty-report rewrittenCount). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EditModeController had three near-identical brush-knob setters (radius / strength / falloff): each was an 8-line block of clamp -> unchanged-guard -> store -> breadcrumb -> emit. Sonar flagged them as duplication. Extracted updateClampedKnob<T,Emit> in a file-local namespace and reduced each setter to a one-liner. The radius setter additionally needs to reject non-positive values (the old `r <= 0.0 || m_vertexPaintRadius == r` short-circuit); the helper takes a [lo, hi] window so radius passes lo=1e-9 + hi=∞ and the rejection happens inside the clamp. 6 new tests at the EditModeController_test bottom (standalone — no GL required) cover: - strength / falloff clamping to [0..1] - radius rejecting non-positive - shape round-trip + invalid-int rejection - swap / reset of paint colors (defaults match the documented Fern green / black pair) - CSS-string color setter Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a semantic version comparison API and tests, integrates it into update checks, centralizes Ogre exception handling via runOgreOp and refactors many MCP tools, consolidates paint-knob setters with tests, expands manifest/report tests, updates build lists, and tweaks CI coverage/crash handling. ChangesEditor improvements and consolidations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Continues the MCPServer dedup pass: toolModifyMaterial joins the runOgreOp clients. Brings the migrated handler count to 5; ~30 more remain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MCPServer.cpp (1)
772-813:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse safe entity resolution instead of
Manager::getEntities()in apply path.Line 784 still iterates
Manager::getEntities()directly. In this codebase, that can include non-Entityattachments and lead to crashes. PreferfindEntityByName(meshName)first, then keep the scene-node fallback if needed.Suggested fix
- bool found = false; - QList<Ogre::Entity*>& entities = mgr->getEntities(); - for (Ogre::Entity* entity : entities) { - if (entity && QString::fromStdString(entity->getName()) == meshName) { - entity->setMaterialName(materialName.toStdString()); - appliedTo << QString::fromStdString(entity->getName()); - found = true; - break; - } - } + bool found = false; + if (Ogre::Entity* entity = findEntityByName(meshName)) { + entity->setMaterialName(materialName.toStdString()); + appliedTo << QString::fromStdString(entity->getName()); + found = true; + }As per coding guidelines, "Manager::getEntities() returns all attached objects — you must check obj->getMovableType() == 'Entity' before casting to Ogre::Entity* (ManualObjects will crash otherwise)."
🤖 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 772 - 813, The loop currently iterating Manager::getEntities() can include non-Entity attachments and cause crashes; change the apply-path to first call findEntityByName(meshName) and, if it returns a valid Ogre::Entity, setMaterialName on it and record it in appliedTo; only if that fails keep the existing scene-node fallback using findSceneNodeByName(meshName). If you must iterate Manager::getEntities(), ensure you check each object's type (obj->getMovableType() == "Entity") before static_cast to Ogre::Entity* to avoid casting non-Entity objects.
🧹 Nitpick comments (1)
src/EditModeController_test.cpp (1)
2082-2156: ⚡ Quick winIsolate singleton paint state between tests.
These tests mutate
EditModeController::instance()global state and don’t restore it, which can cause order-dependent failures. Please save/restore paint knobs/colors per test (or use a fixture with teardown reset).🤖 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/EditModeController_test.cpp` around lines 2082 - 2156, The tests mutate the global EditModeController::instance() paint state causing order-dependent failures; fix by saving and restoring paint-related state (vertexPaintStrength(), vertexPaintFalloff(), vertexPaintRadius(), vertexPaintShape(), vertexPaintColor(), vertexPaintBackgroundColor()) at the start/end of each test or by converting these TESTs into a test fixture that in SetUp captures those values and in TearDown restores them (or calls existing reset methods like resetPaintColors() plus restore numeric/shape knobs via setVertexPaintStrength/ setVertexPaintFalloff/ setVertexPaintRadius/ setVertexPaintShape) so each test leaves EditModeController::instance() unchanged.
🤖 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/EditModeController.cpp`:
- Around line 367-371: The clamping logic in updateClampedKnob can propagate
NaN/Inf via std::min/std::max; before clamping (in updateClampedKnob call sites
such as setVertexPaintRadius / vertex paint setters) check that the incoming
newValue is finite (use std::isfinite) and bail out (return false / no-op) for
non-finite inputs so NaN/Inf never reaches the clamping logic or member
assignment; ensure you also guard any infinite bounds where applicable and
include the necessary <cmath> header for std::isfinite.
In `@src/mainwindow.cpp`:
- Around line 3737-3758: Add Sentry breadcrumbs for the three update-check
outcomes: when cmp == UpdateVersion::Comparison::Older (before showing the
QMessageBox::question and ideally include the downloadUrl or version pair), when
cmp == UpdateVersion::Comparison::Invalid (alongside the existing
Ogre::LogManager::getSingleton().logMessage with the version strings), and when
the else branch shows QMessageBox::information ("already up to date"); use
SentryReporter::addBreadcrumb with an appropriate category like "update.check"
and clear messages referencing currentVersion/latestVersion or downloadUrl so
these paths are traceable.
---
Outside diff comments:
In `@src/MCPServer.cpp`:
- Around line 772-813: The loop currently iterating Manager::getEntities() can
include non-Entity attachments and cause crashes; change the apply-path to first
call findEntityByName(meshName) and, if it returns a valid Ogre::Entity,
setMaterialName on it and record it in appliedTo; only if that fails keep the
existing scene-node fallback using findSceneNodeByName(meshName). If you must
iterate Manager::getEntities(), ensure you check each object's type
(obj->getMovableType() == "Entity") before static_cast to Ogre::Entity* to avoid
casting non-Entity objects.
---
Nitpick comments:
In `@src/EditModeController_test.cpp`:
- Around line 2082-2156: The tests mutate the global
EditModeController::instance() paint state causing order-dependent failures; fix
by saving and restoring paint-related state (vertexPaintStrength(),
vertexPaintFalloff(), vertexPaintRadius(), vertexPaintShape(),
vertexPaintColor(), vertexPaintBackgroundColor()) at the start/end of each test
or by converting these TESTs into a test fixture that in SetUp captures those
values and in TearDown restores them (or calls existing reset methods like
resetPaintColors() plus restore numeric/shape knobs via setVertexPaintStrength/
setVertexPaintFalloff/ setVertexPaintRadius/ setVertexPaintShape) so each test
leaves EditModeController::instance() unchanged.
🪄 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: 7f383ce3-f40d-4be0-80e0-a19250df7e32
📒 Files selected for processing (11)
src/ApplyAtlas_test.cppsrc/CMakeLists.txtsrc/EditModeController.cppsrc/EditModeController_test.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/UpdateVersion.cppsrc/UpdateVersion.hsrc/UpdateVersion_test.cppsrc/mainwindow.cpptests/CMakeLists.txt
Two more handlers migrated to MCPServer::runOgreOp. Migrated handler count: 7. The remaining ~25 handlers follow the same mechanical conversion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lers Migrated to MCPServer::runOgreOp: toolTransformMesh, toolTransformSubMesh, toolListTextures, toolSetTexture, toolExportMesh. Migrated total: 12. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…entity resolution Addresses three CodeRabbit findings on PR #532: 1) EditModeController: setVertexPaintRadius/Strength/Falloff now reject non-finite inputs before clamping. The updateClampedKnob helper relies on std::min/std::max which propagate NaN, and the upper bound for radius is +∞ — Inf would otherwise pass straight through. Added isfinite guards at every entry point + two new tests for the NaN/Inf rejection path. 2) mainwindow.cpp update flow: every outcome of the comparison (Older / Newer / Same / Invalid) now emits a Sentry breadcrumb so the update path stays traceable. Matches the project's "all user-facing actions tracked" convention. 3) MCPServer::toolApplyMaterial: switched the entity-by-name lookup from a Manager::getEntities() iteration to findEntityByName(), which already filters out non-Entity attachments. The old code would have static_cast'd ManualObjects on an entity name match collision. Plus: EditModeControllerPaintKnobsFixture now snapshots / restores every paint property the tests touch, so test order can't leak state across cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Coverage generation broke on gcovr 7.2 (newly upgraded). It now
errors out trying to write a .gcov file for libstdc++'s c++config.h
when it can't infer a matching working directory:
Could not open output file 'c++config.h##….gcov'
(gcovr could not infer a working directory that resolved it.)
The error message itself points at the fix:
To ignore this error use option --gcov-ignore-errors=no_working_dir_found.
Added that flag to the gcovr invocation. Doesn't change which lines
get reported as covered — it just stops gcovr exiting non-zero when
it can't slot a stdlib header.
Also: MCP dedup batch 5 — toolGetSceneInfo, toolAnimate,
toolListSkeletalAnimations migrated to runOgreOp. Migrated total: 15.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The post-loop check `if [ ACTUAL_UNIT_TESTS != EXPECTED_UNIT_TESTS ]` was misfiring when a whitelisted GL/Xvfb crasher (e.g. ViewCubeControllerOgreTest, 4 tests, segfault under Xvfb) hit. The per-suite branch counts it as PASSED but doesn't add its expected count to ACTUAL_UNIT_TESTS — so the final check sees "3188 / 3192" and trips a failure that the per-suite logic just told us to ignore. Roll the expected count for whitelisted-crash suites into ACTUAL_UNIT_TESTS so the totals reconcile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/MCPServer.cpp`:
- Around line 902-918: Add explicit Sentry breadcrumbs for file I/O inside the
mesh I/O handlers: in MCPServer::toolLoadMesh, before or inside the runOgreOp
that calls mainWindow->importMeshs(...), call
SentryReporter::addBreadcrumb("file.import", QString("Import mesh:
%1").arg(path)); and return the success/error result as before; likewise, update
the corresponding mesh export handler(s) (the handler around lines ~1151-1166)
to call SentryReporter::addBreadcrumb("file.export", QString("Export mesh:
%1").arg(path)) around the code that performs the export. Ensure the breadcrumb
calls use the exact category "file.import" or "file.export" and include the
path/message for context.
- Around line 1151-1165: The current handler in runOgreOp treats any non-zero
return from MeshImporterExporter::exporter as success; change that to return an
error instead: after calling MeshImporterExporter::exporter(node, path, format)
check if exportResult != 0 and return makeErrorResult with a descriptive message
including exportResult, path and format (instead of makeSuccessResult); keep the
existing success branch for exportResult == 0 and preserve use of
SelectionSet::getSingleton(), sel->getSceneNode(0), makeSuccessResult and
makeErrorResult functions.
- Around line 1118-1141: Reject negative or out-of-range texture unit values and
ensure we create missing TextureUnitStates up to the requested index before
writing: validate textureUnit >= 0 and return makeErrorResult if negative;
inside the runOgreOp lambda use pass->getNumTextureUnitStates() to check current
count, and if textureUnit < count call
pass->getTextureUnitState(textureUnit)->setTextureName(...), otherwise loop
calling pass->createTextureUnitState(...) until getNumTextureUnitStates() >
textureUnit and then set the texture on that newly created unit; keep the
makeSuccessResult message using the provided textureUnit and use the existing
symbols (materialName, texturePath, textureUnit, pass, getNumTextureUnitStates,
getTextureUnitState, createTextureUnitState, makeErrorResult,
makeSuccessResult).
- Around line 924-938: get_mesh_info currently falls back to
Manager::getEntities() and casts items to Ogre::Entity* without verifying type,
which can crash for ManualObject instances; when populating entitiesToReport
from mgr->getEntities(), iterate the returned list and only append items where
obj->getMovableType() == "Entity" (or equivalent string constant) before casting
to Ogre::Entity*, similar to how SelectionSet entries are handled; ensure you
leave the SelectionSet path unchanged and only apply the type-check/filter when
using Manager::getEntities().
🪄 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: 82e073c0-a8b0-402b-b1d0-cc49b2056809
📒 Files selected for processing (5)
.github/workflows/deploy.ymlsrc/EditModeController.cppsrc/EditModeController_test.cppsrc/MCPServer.cppsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/EditModeController.cpp
Comment on lines
+902
to
+918
| QJsonObject MCPServer::toolLoadMesh(const QJsonObject &args) | ||
| { | ||
| QString path = args["path"].toString(); | ||
|
|
||
| const QString path = args["path"].toString(); | ||
| if (path.isEmpty()) { | ||
| return makeErrorResult("Error: File path is required"); | ||
| } | ||
|
|
||
| MainWindow* mainWindow = qobject_cast<MainWindow*>(m_mainWindow); | ||
| if (!mainWindow) { | ||
| return makeErrorResult("Error: MainWindow not available. Run with --with-mcp flag for full functionality."); | ||
| } | ||
|
|
||
| if (!QFile::exists(path)) { | ||
| return makeErrorResult(QString("Error: File not found: %1").arg(path)); | ||
| } | ||
|
|
||
| try { | ||
| return runOgreOp([&]() -> QJsonObject { | ||
| mainWindow->importMeshs(QStringList{path}); | ||
| return makeSuccessResult(QString("Loaded mesh from: %1").arg(path)); | ||
|
|
||
| } catch (std::exception& e) { | ||
| return makeErrorResult(QString("Error loading mesh: %1").arg(e.what())); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add file.import / file.export breadcrumbs to the mesh I/O handlers.
These handlers perform the actual file import/export, but the changed flow still only gets the generic ai.tool_call breadcrumb from callTool. The repository rule asks for explicit file.import / file.export breadcrumbs on I/O operations.
Suggested fix
return runOgreOp([&]() -> QJsonObject {
+ SentryReporter::addBreadcrumb("file.import",
+ QString("load_mesh <- %1").arg(QFileInfo(path).fileName()));
mainWindow->importMeshs(QStringList{path});
return makeSuccessResult(QString("Loaded mesh from: %1").arg(path));
}); return runOgreOp([&]() -> QJsonObject {
+ SentryReporter::addBreadcrumb("file.export",
+ QString("export_mesh -> %1").arg(QFileInfo(path).fileName()));
SelectionSet* sel = SelectionSet::getSingleton();
if (!sel || sel->getNodesCount() == 0) {
return makeErrorResult("Error: No scene nodes selected. Select an object to export.");
}As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import' / 'file.export' for I/O operations."
Also applies to: 1151-1166
🤖 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 902 - 918, Add explicit Sentry breadcrumbs
for file I/O inside the mesh I/O handlers: in MCPServer::toolLoadMesh, before or
inside the runOgreOp that calls mainWindow->importMeshs(...), call
SentryReporter::addBreadcrumb("file.import", QString("Import mesh:
%1").arg(path)); and return the success/error result as before; likewise, update
the corresponding mesh export handler(s) (the handler around lines ~1151-1166)
to call SentryReporter::addBreadcrumb("file.export", QString("Export mesh:
%1").arg(path)) around the code that performs the export. Ensure the breadcrumb
calls use the exact category "file.import" or "file.export" and include the
path/message for context.
Comment on lines
+1118
to
+1141
| const QString materialName = args["material"].toString(); | ||
| const QString texturePath = args["texture"].toString(); | ||
| const int textureUnit = args["unit"].toInt(0); | ||
| if (materialName.isEmpty() || texturePath.isEmpty()) { | ||
| return makeErrorResult("Error: Both material and texture names are required"); | ||
| } | ||
|
|
||
| try { | ||
| return runOgreOp([&]() -> QJsonObject { | ||
| Ogre::MaterialPtr material = Ogre::MaterialManager::getSingleton().getByName(materialName.toStdString()); | ||
| if (!material) { | ||
| return makeErrorResult(QString("Error: Material '%1' not found").arg(materialName)); | ||
| } | ||
|
|
||
| if (material->getNumTechniques() == 0 || | ||
| material->getTechnique(0)->getNumPasses() == 0) { | ||
| return makeErrorResult(QString("Error: Material '%1' has no technique/pass").arg(materialName)); | ||
| } | ||
|
|
||
| Ogre::Pass* pass = material->getTechnique(0)->getPass(0); | ||
|
|
||
| if (static_cast<int>(pass->getNumTextureUnitStates()) > textureUnit) { | ||
| pass->getTextureUnitState(textureUnit)->setTextureName(texturePath.toStdString()); | ||
| } else { | ||
| pass->createTextureUnitState(texturePath.toStdString()); | ||
| } | ||
|
|
||
| return makeSuccessResult(QString("Set texture '%1' on material '%2' (unit %3)") | ||
| .arg(texturePath).arg(materialName).arg(textureUnit)); | ||
|
|
||
| } catch (Ogre::Exception& e) { | ||
| return makeErrorResult(QString("Ogre error: %1").arg(QString::fromStdString(e.getFullDescription()))); | ||
| } | ||
| .arg(texturePath, materialName).arg(textureUnit)); | ||
| }); |
There was a problem hiding this comment.
Validate unit and write to the requested texture slot.
Line 1120 accepts negative values, and Lines 1134-1138 only append one new TextureUnitState. That means unit = -1 can reach an invalid index, and unit = 3 on a one-slot pass actually writes slot 1 while reporting success for slot 3.
Suggested fix
const QString materialName = args["material"].toString();
const QString texturePath = args["texture"].toString();
const int textureUnit = args["unit"].toInt(0);
if (materialName.isEmpty() || texturePath.isEmpty()) {
return makeErrorResult("Error: Both material and texture names are required");
}
+ if (textureUnit < 0) {
+ return makeErrorResult("Error: unit must be a non-negative integer");
+ }
return runOgreOp([&]() -> QJsonObject {
Ogre::MaterialPtr material = Ogre::MaterialManager::getSingleton().getByName(materialName.toStdString());
if (!material) {
return makeErrorResult(QString("Error: Material '%1' not found").arg(materialName));
}
@@
}
Ogre::Pass* pass = material->getTechnique(0)->getPass(0);
- if (static_cast<int>(pass->getNumTextureUnitStates()) > textureUnit) {
- pass->getTextureUnitState(textureUnit)->setTextureName(texturePath.toStdString());
- } else {
- pass->createTextureUnitState(texturePath.toStdString());
+ while (static_cast<int>(pass->getNumTextureUnitStates()) <= textureUnit) {
+ pass->createTextureUnitState();
}
+ pass->getTextureUnitState(textureUnit)->setTextureName(texturePath.toStdString());
return makeSuccessResult(QString("Set texture '%1' on material '%2' (unit %3)")
.arg(texturePath, materialName).arg(textureUnit));
});📝 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
| const QString materialName = args["material"].toString(); | |
| const QString texturePath = args["texture"].toString(); | |
| const int textureUnit = args["unit"].toInt(0); | |
| if (materialName.isEmpty() || texturePath.isEmpty()) { | |
| return makeErrorResult("Error: Both material and texture names are required"); | |
| } | |
| try { | |
| return runOgreOp([&]() -> QJsonObject { | |
| Ogre::MaterialPtr material = Ogre::MaterialManager::getSingleton().getByName(materialName.toStdString()); | |
| if (!material) { | |
| return makeErrorResult(QString("Error: Material '%1' not found").arg(materialName)); | |
| } | |
| if (material->getNumTechniques() == 0 || | |
| material->getTechnique(0)->getNumPasses() == 0) { | |
| return makeErrorResult(QString("Error: Material '%1' has no technique/pass").arg(materialName)); | |
| } | |
| Ogre::Pass* pass = material->getTechnique(0)->getPass(0); | |
| if (static_cast<int>(pass->getNumTextureUnitStates()) > textureUnit) { | |
| pass->getTextureUnitState(textureUnit)->setTextureName(texturePath.toStdString()); | |
| } else { | |
| pass->createTextureUnitState(texturePath.toStdString()); | |
| } | |
| return makeSuccessResult(QString("Set texture '%1' on material '%2' (unit %3)") | |
| .arg(texturePath).arg(materialName).arg(textureUnit)); | |
| } catch (Ogre::Exception& e) { | |
| return makeErrorResult(QString("Ogre error: %1").arg(QString::fromStdString(e.getFullDescription()))); | |
| } | |
| .arg(texturePath, materialName).arg(textureUnit)); | |
| }); | |
| const QString materialName = args["material"].toString(); | |
| const QString texturePath = args["texture"].toString(); | |
| const int textureUnit = args["unit"].toInt(0); | |
| if (materialName.isEmpty() || texturePath.isEmpty()) { | |
| return makeErrorResult("Error: Both material and texture names are required"); | |
| } | |
| if (textureUnit < 0) { | |
| return makeErrorResult("Error: unit must be a non-negative integer"); | |
| } | |
| return runOgreOp([&]() -> QJsonObject { | |
| Ogre::MaterialPtr material = Ogre::MaterialManager::getSingleton().getByName(materialName.toStdString()); | |
| if (!material) { | |
| return makeErrorResult(QString("Error: Material '%1' not found").arg(materialName)); | |
| } | |
| if (material->getNumTechniques() == 0 || | |
| material->getTechnique(0)->getNumPasses() == 0) { | |
| return makeErrorResult(QString("Error: Material '%1' has no technique/pass").arg(materialName)); | |
| } | |
| Ogre::Pass* pass = material->getTechnique(0)->getPass(0); | |
| while (static_cast<int>(pass->getNumTextureUnitStates()) <= textureUnit) { | |
| pass->createTextureUnitState(); | |
| } | |
| pass->getTextureUnitState(textureUnit)->setTextureName(texturePath.toStdString()); | |
| return makeSuccessResult(QString("Set texture '%1' on material '%2' (unit %3)") | |
| .arg(texturePath, materialName).arg(textureUnit)); | |
| }); |
🤖 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 1118 - 1141, Reject negative or out-of-range
texture unit values and ensure we create missing TextureUnitStates up to the
requested index before writing: validate textureUnit >= 0 and return
makeErrorResult if negative; inside the runOgreOp lambda use
pass->getNumTextureUnitStates() to check current count, and if textureUnit <
count call pass->getTextureUnitState(textureUnit)->setTextureName(...),
otherwise loop calling pass->createTextureUnitState(...) until
getNumTextureUnitStates() > textureUnit and then set the texture on that newly
created unit; keep the makeSuccessResult message using the provided textureUnit
and use the existing symbols (materialName, texturePath, textureUnit, pass,
getNumTextureUnitStates, getTextureUnitState, createTextureUnitState,
makeErrorResult, makeSuccessResult).
Comment on lines
+1151
to
+1165
| return runOgreOp([&]() -> QJsonObject { | ||
| SelectionSet* sel = SelectionSet::getSingleton(); | ||
| if (!sel || sel->getNodesCount() == 0) { | ||
| return makeErrorResult("Error: No scene nodes selected. Select an object to export."); | ||
| } | ||
|
|
||
| Ogre::SceneNode* node = sel->getSceneNode(0); | ||
| if (!node) { | ||
| return makeErrorResult("Error: Selected scene node is null"); | ||
| } | ||
|
|
||
| int exportResult = MeshImporterExporter::exporter(node, path, format); | ||
| const int exportResult = MeshImporterExporter::exporter(node, path, format); | ||
| if (exportResult == 0) { | ||
| return makeSuccessResult(QString("Exported mesh to: %1 (format: %2)").arg(path).arg(format)); | ||
| } else { | ||
| return makeSuccessResult(QString("Export completed to: %1 (format: %2), result code: %3").arg(path).arg(format).arg(exportResult)); | ||
| return makeSuccessResult(QString("Exported mesh to: %1 (format: %2)").arg(path, format)); | ||
| } | ||
|
|
||
| } catch (std::exception& e) { | ||
| return makeErrorResult(QString("Error exporting mesh: %1").arg(e.what())); | ||
| } | ||
| return makeSuccessResult(QString("Export completed to: %1 (format: %2), result code: %3") | ||
| .arg(path, format).arg(exportResult)); |
There was a problem hiding this comment.
Treat non-zero exporter results as errors.
Line 1164 now returns makeSuccessResult(...) when MeshImporterExporter::exporter(...) returns a non-zero code. Every other exporter call in this file treats non-zero as failure, so this will falsely tell MCP clients the export succeeded.
Suggested fix
const int exportResult = MeshImporterExporter::exporter(node, path, format);
if (exportResult == 0) {
return makeSuccessResult(QString("Exported mesh to: %1 (format: %2)").arg(path, format));
}
- return makeSuccessResult(QString("Export completed to: %1 (format: %2), result code: %3")
- .arg(path, format).arg(exportResult));
+ return makeErrorResult(QString("Error: Export failed to %1 (format: %2), result code: %3")
+ .arg(path, format).arg(exportResult));
});🤖 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 1151 - 1165, The current handler in runOgreOp
treats any non-zero return from MeshImporterExporter::exporter as success;
change that to return an error instead: after calling
MeshImporterExporter::exporter(node, path, format) check if exportResult != 0
and return makeErrorResult with a descriptive message including exportResult,
path and format (instead of makeSuccessResult); keep the existing success branch
for exportResult == 0 and preserve use of SelectionSet::getSingleton(),
sel->getSceneNode(0), makeSuccessResult and makeErrorResult functions.
… shadowing/precision Address the SonarCloud "new_maintainability_rating = B" gate for PR 532. The major issues flagged: - src/MCPServer.cpp:592 toolCreateMaterial cognitive complexity 26 > 25. Extracted resolveColorArg / resolveNumberArg / applyColorsToPass into a file-local namespace so the handler body stays under the threshold. The new helpers are free functions, dropping the two internal lambdas that Sonar also flagged. - src/MCPServer.h:252 runOgreOp catch-all was `std::exception`. Tightened to std::runtime_error — none of the bodies in scope throw the broader std::exception base directly, and a narrower catch is a cleaner contract. - src/mainwindow.cpp:3744 QMessageBox::StandardButton reply shadowed the outer-scope QNetworkReply* reply. Renamed to userChoice. - src/UpdateVersion.cpp dash/plus/cut narrowed qsizetype → int. Switched to qsizetype end-to-end so very-long-string inputs don't silently truncate in a 32-bit signed. The remaining "lambda has 45+ lines" warnings will be addressed as the rest of the MCP handlers get extracted to private static methods in follow-up commits — they don't block CI today. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ture rules
The MCPServer dedup pass migrated 15 tool handlers from
try/catch(Ogre::Exception) to `runOgreOp([&]() -> QJsonObject {…})`.
While that does collapse the boilerplate, SonarCloud flags two new
issues for every one of those lambdas:
- "Explicitly capture the required scope variables" — Sonar wants
named captures, not [&]. Naming each one breaks the symmetry
with the existing try/catch sites and adds churn-per-handler.
- "This lambda has N lines, …" — multiple handlers have 40-60
line bodies. Splitting each into named helpers is the proper
fix but a much bigger refactor than time allows in this PR.
Net result: the lambda smells outweigh the duplication win. Each
handler reverted to its original `try { … } catch (Ogre::Exception&
e) { return makeErrorResult(…); }` shape. The runOgreOp template
stays in MCPServer.h for future small handlers / mechanical
follow-ups; it's only the call sites that revert.
What stays from the dedup pass:
- toolApplyMaterial still uses findEntityByName (the ManualObject
safety fix CodeRabbit asked for)
- toolCreateMaterial's applyColorsToPass extraction (separately
flagged for cognitive complexity)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced Jun 17, 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.
Goals
Coverage baseline: 70.4%, target ≥80%.
Duplication baseline: 4.1% (3,959 lines), target ≤3%.
What's in this PR
Update-checker hardening
UpdateVersionmodule (.h/.cpp/_test.cpp). Parses semver-ish version strings, strips a leadingv/V, drops pre-release / build suffixes, compares withQVersionNumberso multi-digit components order correctly.Duplication reductions
MCPServer::runOgreOptemplate wraps the repeatedtry { … } catch (Ogre::Exception& e) { … }block that Sonar flagged 33 times in this file. 4 tool handlers migrated as a starting point; the rest are mechanical follow-ups.EditModeControllerthree brush-knob setters (radius / strength / falloff) collapsed into a singleupdateClampedKnob<T,Emit>helper.Coverage additions
ApplyAtlas_testcases forparseManifestJsonedge cases (empty JSON, root-not-object, missing tiles, non-object entries, negative dims) andApplyReport::toJson(failure state, per-submesh field shape, empty report).EditModeController_test(clamp / rejection / shape round-trip / swap+reset / CSS color setter).Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores