Lights: Slice H — CLI/MCP parity, tests, docs (#490) - #826
Merged
Conversation
…490). Expose qtmesh light for list/add/remove/edit/rig workflows, five MCP scene-light tools, scene.light.* Sentry breadcrumbs, and CLAUDE.md guidance so lighting matches project conventions end-to-end. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR adds a headless ChangesScene Lighting CLI/MCP/Docs Slice
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SceneLightsCLI
participant LightManager
participant LightRigLibrary
participant SceneLightsIO
User->>SceneLightsCLI: qtmesh light scene.gltf --add/--edit/--apply-rig
SceneLightsCLI->>SceneLightsCLI: loadInputFile()
alt add/remove/edit
SceneLightsCLI->>LightManager: create/delete/update light
else apply-rig
SceneLightsCLI->>LightRigLibrary: apply(rigId)
end
SceneLightsCLI->>SceneLightsIO: build JSON / write sidecar
SceneLightsCLI->>SceneLightsCLI: exportOutput(outputPath)
SceneLightsCLI-->>User: exit code, JSON or message
sequenceDiagram
participant Client
participant MCPServer
participant LightManager
participant LightRigLibrary
Client->>MCPServer: tools/call (create_light, list_lights, ...)
MCPServer->>MCPServer: parseLightType / parseColor
alt create/delete/list/set property
MCPServer->>LightManager: create/delete/find/update light
else apply_light_rig
MCPServer->>LightRigLibrary: apply(rigId)
end
MCPServer-->>Client: JSON result
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![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: 9956a8cc95
ℹ️ 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".
| float range = 10.0f; | ||
| bool enabled = true; | ||
|
|
||
| for (int i = 2; i < argc; ++i) |
There was a problem hiding this comment.
When the app is invoked via the documented --cli mode, e.g. QtMeshEditor --cli light scene.gltf --list, argv[2] is the literal subcommand light. Starting this parser at index 2 makes that string become inputPath, then the real file is treated as an unexpected extra positional argument and the command returns 2, so file-based light actions only work through the qtmesh symlink path. Skip --cli/light here or start from the actual command index.
Useful? React with 👍 / 👎.
|
|
||
| if (isSceneLikePath(path)) | ||
| { | ||
| if (!MeshImporterExporter::sceneImporter(path)) |
There was a problem hiding this comment.
For scene-like inputs with no saved lights, sceneImporter() imports lights with useDefaultWhenEmpty=true, which applies default scene lighting when no document lights exist. That means qtmesh light empty.gltf --add point ... exports both the requested light and the default rig/key light even though the user did not ask for it; the CLI should load scene lights with useDefaultWhenEmpty=false or clear the default before applying the requested mutation.
Useful? React with 👍 / 👎.
Comment on lines
+4825
to
+4828
| handle.light->setDiffuseColour(parseMcpColour(colourValue)); | ||
| } | ||
| if (args.contains(QStringLiteral("intensity"))) | ||
| handle.light->setPowerScale(static_cast<Ogre::Real>(args.value(QStringLiteral("intensity")).toDouble(1.0))); |
There was a problem hiding this comment.
When create_light is called from the live GUI with colour, intensity, range, or cone, createLightAt() has already emitted lightCreated, so LightVisualizer builds its overlay from the default light. These direct writes do not emit lightChanged or call applyProperties, leaving the gizmo colour/range/cone stale until another refresh; apply a snapshot through LightManager after parsing the optional properties.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/MCPServer.cpp (1)
4792-4865: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the MCP-level
scene.light.*breadcrumbs
toolCreateLight,toolDeleteLight, andtoolApplyLightRigeach add a second breadcrumb for actions that already emit the samescene.light.create/scene.light.delete/scene.light.apply_rigentry inLightManagerorLightRigLibrary.callTool()already recordsai.tool_call, so these MCP-side breadcrumbs just add noise.🤖 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 4792 - 4865, Remove the redundant MCP-side Sentry breadcrumbs from toolCreateLight, toolDeleteLight, and toolApplyLightRig in MCPServer.cpp, since LightManager and LightRigLibrary already emit the same scene.light.create, scene.light.delete, and scene.light.apply_rig breadcrumbs and callTool() already records ai.tool_call. Keep the existing tool behavior unchanged, and delete only the extra SentryReporter::addBreadcrumb calls in those MCP tool methods so the breadcrumb stream stays non-duplicative.
🤖 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/LightRigLibrary.cpp`:
- Around line 455-458: `apply()` in `LightRigLibrary` already emits the
`scene.light.apply_rig` breadcrumb, so the MCP path is logging the same category
twice. Fix this on the MCP side in `MCPServer::toolApplyLightRig` by removing
the redundant `scene.light.apply_rig` breadcrumb there and keeping only the
appropriate MCP/UI breadcrumb behavior after the call to `apply()`.
In `@src/MCPServer_test.cpp`:
- Around line 5491-5558: Add focused Google Test coverage in MCPServerTest for
the missing MCP tools, specifically set_light_property and apply_light_rig,
using the existing server->callTool and LightManager::getSingleton() patterns.
Add cases that exercise set_light_property for non-positional fields like
intensity/color and for position/direction updates to verify the light actually
changes, and add an apply_light_rig case that passes a rig id and asserts new
lights are added. Keep the tests alongside the existing
create_light/list_lights/delete_light tests in MCPServerTest so the new tool
behavior is covered directly.
In `@src/SceneLightsCLI.cpp`:
- Around line 300-308: The --intensity and --range parsing in SceneLightsCLI.cpp
accepts invalid strings because QString::toFloat() falls back to 0.0f on
failure. Update the argument handling in the CLI parsing block to validate
conversion success for both fields before setting hasIntensity/hasRange, and
report an error or reject the input when parsing fails. Use the existing arg
handling around --intensity and --range to locate the fix.
- Around line 393-506: The CLI flow in main should validate the -o output path
before any mutating operations are applied. Add an early check near the existing
mutates/input-file validation in SceneLightsCLI.cpp, before initOgreHeadless(),
LightManager::tryConnectToManager(), and any add/remove/edit/applyRig logic, so
missing outputPath fails immediately with the existing exportOutput-style error
instead of after in-memory changes.
---
Nitpick comments:
In `@src/MCPServer.cpp`:
- Around line 4792-4865: Remove the redundant MCP-side Sentry breadcrumbs from
toolCreateLight, toolDeleteLight, and toolApplyLightRig in MCPServer.cpp, since
LightManager and LightRigLibrary already emit the same scene.light.create,
scene.light.delete, and scene.light.apply_rig breadcrumbs and callTool() already
records ai.tool_call. Keep the existing tool behavior unchanged, and delete only
the extra SentryReporter::addBreadcrumb calls in those MCP tool methods so the
breadcrumb stream stays non-duplicative.
🪄 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: 9f1f4e47-cda7-4c1e-9fa5-03fcad4c84de
📒 Files selected for processing (18)
CLAUDE.mdsrc/AppLaunchHandler.cppsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_test.cppsrc/CMakeLists.txtsrc/LightManager.cppsrc/LightPropertiesController.cppsrc/LightRigLibrary.cppsrc/LightVisualizer.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/MCPServer_test.cppsrc/SceneLightsCLI.cppsrc/SceneLightsCLI.hsrc/SceneLightsIO.cppsrc/SceneLightsIO.htests/CMakeLists.txt
Comment on lines
+455
to
458
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.apply_rig"), | ||
| QStringLiteral("Apply light rig: %1").arg(rigId)); | ||
| SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), | ||
| QStringLiteral("Apply light rig: %1").arg(rigId)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Duplicate breadcrumb — see companion comment in src/MCPServer.cpp.
apply() emits scene.light.apply_rig here; MCPServer::toolApplyLightRig (src/MCPServer.cpp Line 4973-4974) emits the same category again right after calling this function, so every MCP-triggered rig apply logs scene.light.apply_rig twice (plus ui.action). This function itself is correct in isolation — the fix belongs on the MCP side.
🤖 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/LightRigLibrary.cpp` around lines 455 - 458, `apply()` in
`LightRigLibrary` already emits the `scene.light.apply_rig` breadcrumb, so the
MCP path is logging the same category twice. Fix this on the MCP side in
`MCPServer::toolApplyLightRig` by removing the redundant `scene.light.apply_rig`
breadcrumb there and keeping only the appropriate MCP/UI breadcrumb behavior
after the call to `apply()`.
Comment on lines
+5491
to
+5558
| // ========================================================================== | ||
| // SLICE H (#490): scene light MCP tools | ||
| // ========================================================================== | ||
|
|
||
| TEST_F(MCPServerTest, ListLightsToolsExposed) | ||
| { | ||
| QJsonObject result = server->handleToolsList(); | ||
| QJsonArray tools = result["tools"].toArray(); | ||
| bool foundCreate = false; | ||
| bool foundList = false; | ||
| for (const QJsonValue& t : tools) | ||
| { | ||
| const QString name = t.toObject()["name"].toString(); | ||
| if (name == QStringLiteral("create_light")) | ||
| foundCreate = true; | ||
| if (name == QStringLiteral("list_lights")) | ||
| foundList = true; | ||
| } | ||
| EXPECT_TRUE(foundCreate); | ||
| EXPECT_TRUE(foundList); | ||
| } | ||
|
|
||
| TEST_F(MCPServerTest, CreateLightAddsUserLight) | ||
| { | ||
| const int before = LightManager::getSingleton()->lights().size(); | ||
|
|
||
| QJsonObject args; | ||
| args[QStringLiteral("type")] = QStringLiteral("point"); | ||
| args[QStringLiteral("position")] = QJsonArray{1.0, 2.0, 3.0}; | ||
| args[QStringLiteral("intensity")] = 2.5; | ||
|
|
||
| QJsonObject result = server->callTool(QStringLiteral("create_light"), args); | ||
| ASSERT_FALSE(isError(result)) << getResultText(result).toStdString(); | ||
|
|
||
| EXPECT_EQ(LightManager::getSingleton()->lights().size(), before + 1); | ||
| } | ||
|
|
||
| TEST_F(MCPServerTest, ListLightsReturnsJsonArray) | ||
| { | ||
| QJsonObject createArgs; | ||
| createArgs[QStringLiteral("type")] = QStringLiteral("directional"); | ||
| createArgs[QStringLiteral("position")] = QJsonArray{0.0, 5.0, 0.0}; | ||
| ASSERT_FALSE(isError(server->callTool(QStringLiteral("create_light"), createArgs))); | ||
|
|
||
| QJsonObject result = server->callTool(QStringLiteral("list_lights"), QJsonObject()); | ||
| ASSERT_FALSE(isError(result)) << getResultText(result).toStdString(); | ||
|
|
||
| const QJsonDocument doc = QJsonDocument::fromJson(getResultText(result).toUtf8()); | ||
| ASSERT_TRUE(doc.isObject()); | ||
| EXPECT_TRUE(doc.object().contains(QStringLiteral("lights"))); | ||
| EXPECT_GE(doc.object().value(QStringLiteral("lightCount")).toInt(), 1); | ||
| } | ||
|
|
||
| TEST_F(MCPServerTest, DeleteLightRemovesUserLight) | ||
| { | ||
| QJsonObject createArgs; | ||
| createArgs[QStringLiteral("type")] = QStringLiteral("point"); | ||
| createArgs[QStringLiteral("position")] = QJsonArray{0.0, 1.0, 0.0}; | ||
| ASSERT_FALSE(isError(server->callTool(QStringLiteral("create_light"), createArgs))); | ||
|
|
||
| const QString name = LightManager::getSingleton()->lights().last().name; | ||
| QJsonObject deleteArgs; | ||
| deleteArgs[QStringLiteral("name")] = name; | ||
| QJsonObject result = server->callTool(QStringLiteral("delete_light"), deleteArgs); | ||
| ASSERT_FALSE(isError(result)) << getResultText(result).toStdString(); | ||
| EXPECT_EQ(LightManager::getSingleton()->findLight(name), nullptr); | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add coverage for set_light_property and apply_light_rig.
Tests cover create_light, list_lights, and delete_light, but not set_light_property or apply_light_rig — two of the five new MCP tools ship without direct tests, including the set_light_property position/direction path flagged separately in src/MCPServer.cpp (Line 4893-4956) as potentially reverting edits. As per coding guidelines, "Add Google Test unit tests for new functionality; test files should live alongside source in src/ and use the _test.cpp suffix." Adding a couple of focused cases here (e.g., set intensity/colour, set position and assert the node actually moved, apply a rig id and check LightManager::getSingleton()->lights() grows) would also have caught the potential revert bug.
Want me to draft these test cases?
🤖 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_test.cpp` around lines 5491 - 5558, Add focused Google Test
coverage in MCPServerTest for the missing MCP tools, specifically
set_light_property and apply_light_rig, using the existing server->callTool and
LightManager::getSingleton() patterns. Add cases that exercise
set_light_property for non-positional fields like intensity/color and for
position/direction updates to verify the light actually changes, and add an
apply_light_rig case that passes a rig id and asserts new lights are added. Keep
the tests alongside the existing create_light/list_lights/delete_light tests in
MCPServerTest so the new tool behavior is covered directly.
Source: Coding guidelines
Comment on lines
+300
to
+308
| else if (arg == QStringLiteral("--intensity") && i + 1 < argc) | ||
| { | ||
| intensity = QString::fromUtf8(argv[++i]).toFloat(); | ||
| hasIntensity = true; | ||
| } | ||
| else if (arg == QStringLiteral("--range") && i + 1 < argc) | ||
| { | ||
| range = QString::fromUtf8(argv[++i]).toFloat(); | ||
| hasRange = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate --intensity and --range float parsing
toFloat() silently returns 0.0f on conversion failure. --intensity abc would set intensity to 0.0 without any error, producing a light with no brightness.
🐛 Proposed fix
else if (arg == QStringLiteral("--intensity") && i + 1 < argc)
{
- intensity = QString::fromUtf8(argv[++i]).toFloat();
+ bool ok = false;
+ intensity = QString::fromUtf8(argv[++i]).toFloat(&ok);
+ if (!ok)
+ return 2;
hasIntensity = true;
}
else if (arg == QStringLiteral("--range") && i + 1 < argc)
{
- range = QString::fromUtf8(argv[++i]).toFloat();
+ bool ok = false;
+ range = QString::fromUtf8(argv[++i]).toFloat(&ok);
+ if (!ok)
+ return 2;
hasRange = true;
}📝 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
| else if (arg == QStringLiteral("--intensity") && i + 1 < argc) | |
| { | |
| intensity = QString::fromUtf8(argv[++i]).toFloat(); | |
| hasIntensity = true; | |
| } | |
| else if (arg == QStringLiteral("--range") && i + 1 < argc) | |
| { | |
| range = QString::fromUtf8(argv[++i]).toFloat(); | |
| hasRange = true; | |
| else if (arg == QStringLiteral("--intensity") && i + 1 < argc) | |
| { | |
| bool ok = false; | |
| intensity = QString::fromUtf8(argv[++i]).toFloat(&ok); | |
| if (!ok) | |
| return 2; | |
| hasIntensity = true; | |
| } | |
| else if (arg == QStringLiteral("--range") && i + 1 < argc) | |
| { | |
| bool ok = false; | |
| range = QString::fromUtf8(argv[++i]).toFloat(&ok); | |
| if (!ok) | |
| return 2; | |
| hasRange = true; |
🤖 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/SceneLightsCLI.cpp` around lines 300 - 308, The --intensity and --range
parsing in SceneLightsCLI.cpp accepts invalid strings because QString::toFloat()
falls back to 0.0f on failure. Update the argument handling in the CLI parsing
block to validate conversion success for both fields before setting
hasIntensity/hasRange, and report an error or reject the input when parsing
fails. Use the existing arg handling around --intensity and --range to locate
the fix.
Comment on lines
+393
to
+506
| const bool mutates = addLight || !removeName.isEmpty() || !editName.isEmpty() || applyRig; | ||
| if (!mutates) | ||
| { | ||
| printHelp(); | ||
| return 2; | ||
| } | ||
|
|
||
| if (!CLIPipeline::initOgreHeadless()) | ||
| return 1; | ||
|
|
||
| LightManager::getSingleton()->tryConnectToManager(); | ||
|
|
||
| QString loadError; | ||
| if (!loadInputFile(inputFi.absoluteFilePath(), &loadError)) | ||
| { | ||
| CLIPipeline::writeCliError(QStringLiteral("Error: %1\n").arg(loadError)); | ||
| return 1; | ||
| } | ||
|
|
||
| auto* lights = LightManager::getSingleton(); | ||
|
|
||
| if (addLight) | ||
| { | ||
| Ogre::Light::LightTypes type = Ogre::Light::LT_POINT; | ||
| if (!parseLightType(lightTypeText, type)) | ||
| { | ||
| CLIPipeline::writeCliError( | ||
| QStringLiteral("Error: Unknown light type '%1'\n").arg(lightTypeText)); | ||
| return 2; | ||
| } | ||
| if (!hasPosition) | ||
| position = Ogre::Vector3(0, 2, 0); | ||
| const bool setDirection = | ||
| type == Ogre::Light::LT_DIRECTIONAL || type == Ogre::Light::LT_SPOTLIGHT; | ||
| if (!hasDirection && setDirection) | ||
| direction = Ogre::Vector3(0, -1, -1); | ||
|
|
||
| LightHandle handle = lights->createLightAt( | ||
| type, LightManager::defaultBaseNameForType(type), position, direction, setDirection); | ||
| if (!handle.isValid()) | ||
| { | ||
| CLIPipeline::writeCliError(QStringLiteral("Error: Failed to create light\n")); | ||
| return 1; | ||
| } | ||
| if (hasColour) | ||
| handle.light->setDiffuseColour(colour); | ||
| if (hasIntensity) | ||
| handle.light->setPowerScale(intensity); | ||
| if (hasRange && type != Ogre::Light::LT_DIRECTIONAL) | ||
| handle.light->setAttenuation(range, 1.0f, 0.0f, 0.0f); | ||
|
|
||
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.create"), | ||
| QStringLiteral("CLI add %1").arg(handle.name)); | ||
| } | ||
|
|
||
| if (!removeName.isEmpty()) | ||
| { | ||
| if (!lights->deleteLight(removeName)) | ||
| { | ||
| CLIPipeline::writeCliError( | ||
| QStringLiteral("Error: Light not found: %1\n").arg(removeName)); | ||
| return 1; | ||
| } | ||
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.delete"), | ||
| QStringLiteral("CLI remove %1").arg(removeName)); | ||
| } | ||
|
|
||
| if (!editName.isEmpty()) | ||
| { | ||
| LightHandle* handle = lights->findLight(editName); | ||
| if (!handle || !handle->isValid()) | ||
| { | ||
| CLIPipeline::writeCliError(QStringLiteral("Error: Light not found: %1\n").arg(editName)); | ||
| return 1; | ||
| } | ||
|
|
||
| LightSnapshot snapshot = LightSnapshot::fromHandle(*handle); | ||
| if (hasPosition) | ||
| handle->sceneNode->setPosition(position); | ||
| if (hasDirection) | ||
| handle->sceneNode->setDirection(direction); | ||
| if (hasColour) | ||
| snapshot.diffuse = colour; | ||
| if (hasIntensity) | ||
| snapshot.powerScale = intensity; | ||
| if (hasRange) | ||
| snapshot.attenuationRange = range; | ||
| if (hasEnabled) | ||
| snapshot.enabled = enabled; | ||
|
|
||
| if (!lights->applyProperties(editName, snapshot)) | ||
| { | ||
| CLIPipeline::writeCliError( | ||
| QStringLiteral("Error: Failed to edit light: %1\n").arg(editName)); | ||
| return 1; | ||
| } | ||
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.edit"), | ||
| QStringLiteral("CLI edit %1").arg(editName)); | ||
| } | ||
|
|
||
| if (applyRig) | ||
| { | ||
| const LightRigApplyResult result = LightRigLibrary::apply(rigId, replaceRig); | ||
| if (!result.ok) | ||
| { | ||
| CLIPipeline::writeCliError(QStringLiteral("Error: %1\n").arg(result.error)); | ||
| return 1; | ||
| } | ||
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.apply_rig"), | ||
| QStringLiteral("CLI rig %1").arg(rigId)); | ||
| } | ||
|
|
||
| QString exportError; | ||
| const int exportRc = exportOutput(outputPath, &exportError); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate output path before performing mutations
The -o output path is only validated inside exportOutput (line 117) after all mutations have already been applied in-memory. If the user forgets -o, the tool loads the file, performs add/remove/edit/apply-rig, then fails with "Output path (-o) is required" — wasting work and producing a confusing error sequence.
🐛 Proposed fix
const bool mutates = addLight || !removeName.isEmpty() || !editName.isEmpty() || applyRig;
if (!mutates)
{
printHelp();
return 2;
}
+ if (outputPath.isEmpty())
+ {
+ CLIPipeline::writeCliError(QStringLiteral("Error: Output path (-o) is required\n"));
+ return 2;
+ }
+
if (!CLIPipeline::initOgreHeadless())📝 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 bool mutates = addLight || !removeName.isEmpty() || !editName.isEmpty() || applyRig; | |
| if (!mutates) | |
| { | |
| printHelp(); | |
| return 2; | |
| } | |
| if (!CLIPipeline::initOgreHeadless()) | |
| return 1; | |
| LightManager::getSingleton()->tryConnectToManager(); | |
| QString loadError; | |
| if (!loadInputFile(inputFi.absoluteFilePath(), &loadError)) | |
| { | |
| CLIPipeline::writeCliError(QStringLiteral("Error: %1\n").arg(loadError)); | |
| return 1; | |
| } | |
| auto* lights = LightManager::getSingleton(); | |
| if (addLight) | |
| { | |
| Ogre::Light::LightTypes type = Ogre::Light::LT_POINT; | |
| if (!parseLightType(lightTypeText, type)) | |
| { | |
| CLIPipeline::writeCliError( | |
| QStringLiteral("Error: Unknown light type '%1'\n").arg(lightTypeText)); | |
| return 2; | |
| } | |
| if (!hasPosition) | |
| position = Ogre::Vector3(0, 2, 0); | |
| const bool setDirection = | |
| type == Ogre::Light::LT_DIRECTIONAL || type == Ogre::Light::LT_SPOTLIGHT; | |
| if (!hasDirection && setDirection) | |
| direction = Ogre::Vector3(0, -1, -1); | |
| LightHandle handle = lights->createLightAt( | |
| type, LightManager::defaultBaseNameForType(type), position, direction, setDirection); | |
| if (!handle.isValid()) | |
| { | |
| CLIPipeline::writeCliError(QStringLiteral("Error: Failed to create light\n")); | |
| return 1; | |
| } | |
| if (hasColour) | |
| handle.light->setDiffuseColour(colour); | |
| if (hasIntensity) | |
| handle.light->setPowerScale(intensity); | |
| if (hasRange && type != Ogre::Light::LT_DIRECTIONAL) | |
| handle.light->setAttenuation(range, 1.0f, 0.0f, 0.0f); | |
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.create"), | |
| QStringLiteral("CLI add %1").arg(handle.name)); | |
| } | |
| if (!removeName.isEmpty()) | |
| { | |
| if (!lights->deleteLight(removeName)) | |
| { | |
| CLIPipeline::writeCliError( | |
| QStringLiteral("Error: Light not found: %1\n").arg(removeName)); | |
| return 1; | |
| } | |
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.delete"), | |
| QStringLiteral("CLI remove %1").arg(removeName)); | |
| } | |
| if (!editName.isEmpty()) | |
| { | |
| LightHandle* handle = lights->findLight(editName); | |
| if (!handle || !handle->isValid()) | |
| { | |
| CLIPipeline::writeCliError(QStringLiteral("Error: Light not found: %1\n").arg(editName)); | |
| return 1; | |
| } | |
| LightSnapshot snapshot = LightSnapshot::fromHandle(*handle); | |
| if (hasPosition) | |
| handle->sceneNode->setPosition(position); | |
| if (hasDirection) | |
| handle->sceneNode->setDirection(direction); | |
| if (hasColour) | |
| snapshot.diffuse = colour; | |
| if (hasIntensity) | |
| snapshot.powerScale = intensity; | |
| if (hasRange) | |
| snapshot.attenuationRange = range; | |
| if (hasEnabled) | |
| snapshot.enabled = enabled; | |
| if (!lights->applyProperties(editName, snapshot)) | |
| { | |
| CLIPipeline::writeCliError( | |
| QStringLiteral("Error: Failed to edit light: %1\n").arg(editName)); | |
| return 1; | |
| } | |
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.edit"), | |
| QStringLiteral("CLI edit %1").arg(editName)); | |
| } | |
| if (applyRig) | |
| { | |
| const LightRigApplyResult result = LightRigLibrary::apply(rigId, replaceRig); | |
| if (!result.ok) | |
| { | |
| CLIPipeline::writeCliError(QStringLiteral("Error: %1\n").arg(result.error)); | |
| return 1; | |
| } | |
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.apply_rig"), | |
| QStringLiteral("CLI rig %1").arg(rigId)); | |
| } | |
| QString exportError; | |
| const int exportRc = exportOutput(outputPath, &exportError); | |
| const bool mutates = addLight || !removeName.isEmpty() || !editName.isEmpty() || applyRig; | |
| if (!mutates) | |
| { | |
| printHelp(); | |
| return 2; | |
| } | |
| if (outputPath.isEmpty()) | |
| { | |
| CLIPipeline::writeCliError(QStringLiteral("Error: Output path (-o) is required\n")); | |
| return 2; | |
| } | |
| if (!CLIPipeline::initOgreHeadless()) | |
| return 1; | |
| LightManager::getSingleton()->tryConnectToManager(); | |
| QString loadError; | |
| if (!loadInputFile(inputFi.absoluteFilePath(), &loadError)) | |
| { | |
| CLIPipeline::writeCliError(QStringLiteral("Error: %1\n").arg(loadError)); | |
| return 1; | |
| } | |
| auto* lights = LightManager::getSingleton(); | |
| if (addLight) | |
| { | |
| Ogre::Light::LightTypes type = Ogre::Light::LT_POINT; | |
| if (!parseLightType(lightTypeText, type)) | |
| { | |
| CLIPipeline::writeCliError( | |
| QStringLiteral("Error: Unknown light type '%1'\n").arg(lightTypeText)); | |
| return 2; | |
| } | |
| if (!hasPosition) | |
| position = Ogre::Vector3(0, 2, 0); | |
| const bool setDirection = | |
| type == Ogre::Light::LT_DIRECTIONAL || type == Ogre::Light::LT_SPOTLIGHT; | |
| if (!hasDirection && setDirection) | |
| direction = Ogre::Vector3(0, -1, -1); | |
| LightHandle handle = lights->createLightAt( | |
| type, LightManager::defaultBaseNameForType(type), position, direction, setDirection); | |
| if (!handle.isValid()) | |
| { | |
| CLIPipeline::writeCliError(QStringLiteral("Error: Failed to create light\n")); | |
| return 1; | |
| } | |
| if (hasColour) | |
| handle.light->setDiffuseColour(colour); | |
| if (hasIntensity) | |
| handle.light->setPowerScale(intensity); | |
| if (hasRange && type != Ogre::Light::LT_DIRECTIONAL) | |
| handle.light->setAttenuation(range, 1.0f, 0.0f, 0.0f); | |
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.create"), | |
| QStringLiteral("CLI add %1").arg(handle.name)); | |
| } | |
| if (!removeName.isEmpty()) | |
| { | |
| if (!lights->deleteLight(removeName)) | |
| { | |
| CLIPipeline::writeCliError( | |
| QStringLiteral("Error: Light not found: %1\n").arg(removeName)); | |
| return 1; | |
| } | |
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.delete"), | |
| QStringLiteral("CLI remove %1").arg(removeName)); | |
| } | |
| if (!editName.isEmpty()) | |
| { | |
| LightHandle* handle = lights->findLight(editName); | |
| if (!handle || !handle->isValid()) | |
| { | |
| CLIPipeline::writeCliError(QStringLiteral("Error: Light not found: %1\n").arg(editName)); | |
| return 1; | |
| } | |
| LightSnapshot snapshot = LightSnapshot::fromHandle(*handle); | |
| if (hasPosition) | |
| handle->sceneNode->setPosition(position); | |
| if (hasDirection) | |
| handle->sceneNode->setDirection(direction); | |
| if (hasColour) | |
| snapshot.diffuse = colour; | |
| if (hasIntensity) | |
| snapshot.powerScale = intensity; | |
| if (hasRange) | |
| snapshot.attenuationRange = range; | |
| if (hasEnabled) | |
| snapshot.enabled = enabled; | |
| if (!lights->applyProperties(editName, snapshot)) | |
| { | |
| CLIPipeline::writeCliError( | |
| QStringLiteral("Error: Failed to edit light: %1\n").arg(editName)); | |
| return 1; | |
| } | |
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.edit"), | |
| QStringLiteral("CLI edit %1").arg(editName)); | |
| } | |
| if (applyRig) | |
| { | |
| const LightRigApplyResult result = LightRigLibrary::apply(rigId, replaceRig); | |
| if (!result.ok) | |
| { | |
| CLIPipeline::writeCliError(QStringLiteral("Error: %1\n").arg(result.error)); | |
| return 1; | |
| } | |
| SentryReporter::addBreadcrumb(QStringLiteral("scene.light.apply_rig"), | |
| QStringLiteral("CLI rig %1").arg(rigId)); | |
| } | |
| QString exportError; | |
| const int exportRc = exportOutput(outputPath, &exportError); |
🤖 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/SceneLightsCLI.cpp` around lines 393 - 506, The CLI flow in main should
validate the -o output path before any mutating operations are applied. Add an
early check near the existing mutates/input-file validation in
SceneLightsCLI.cpp, before initOgreHeadless(),
LightManager::tryConnectToManager(), and any add/remove/edit/applyRig logic, so
missing outputPath fails immediately with the existing exportOutput-style error
instead of after in-memory changes.
|
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
qtmesh lightwith--list,--list-rigs,--add,--remove,--edit, and--apply-rig(JSON where applicable), exporting through the same scene/mesh paths as the GUI.create_light,delete_light,list_lights,set_light_property, andapply_light_rigoperating on the live scene.scene.light.*Sentry breadcrumbs on core light operations and documents Scene Lighting + intensity units inCLAUDE.md.Test plan
UnitTests --gtest_filter="CLIPipelineCmdLight*"UnitTests --gtest_filter="MCPServerTest.*Light*"qtmesh light scene.gltf --list --jsonlist_lights/create_lightvia--with-mcpCloses #490
Made with Cursor
Summary by CodeRabbit
New Features
lightcommand for listing, creating, editing, removing, and applying scene light rigs from the command line.Bug Fixes