feat(lights): scene light authoring epic (#482–#487)#803
Conversation
Add user scene lights with outliner integration, inspector properties, viewport gizmos/icons, preset rigs (three-point default), and undoable apply/replace flows. Pair outdoor/indoor/studio rigs with bundled HDRIs without reloading when already active. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR adds a full lighting subsystem: user-light management, preset rig application, light property editing, viewport light overlays, scene-tree integration, and QML/UI wiring for light creation and editing. ChangesLighting Subsystem
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MainWindow
participant LightsController
participant LightManager
participant UndoManager
MainWindow->>LightsController: addPointLightAtViewport()
LightsController->>LightManager: createLightAt(type, position, direction)
LightsController->>UndoManager: push CreateLightCommand
LightsController-->>MainWindow: select created light
sequenceDiagram
participant SceneLightingController
participant LightRigLibrary
participant LightManager
participant UndoManager
SceneLightingController->>LightRigLibrary: apply(rigId, replaceExisting)
LightRigLibrary->>LightManager: capture/remove/apply rig lights
LightRigLibrary-->>SceneLightingController: LightRigApplyResult
SceneLightingController->>UndoManager: push ApplyLightRigCommand
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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 |
![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: 3407eb095c
ℹ️ 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".
| m_after = cmd->m_after; | ||
| return true; |
There was a problem hiding this comment.
When two consecutive edits change the same property class on different selections, QUndoStack can merge them because the id only encodes the property class. For example, editing Light A's intensity and then Light B's intensity leaves m_before for A but replaces m_after with B here, so Undo reverts A while B's edit remains applied, and Redo applies only B. The merge needs to reject commands whose snapshot names/counts do not match the existing command.
Useful? React with 👍 / 👎.
| for (const LightSnapshot& snapshot : m_removedLights) | ||
| lights->restoreSnapshot(snapshot); |
There was a problem hiding this comment.
Undoing a rig apply restores the lights that were removed from previous rig groups as ordinary root-level lights. If a user applies Three-point Studio without replacing over an existing Single Key rig, Undo restores the old KeyLight outside any rig group; Redo with replaceExisting == false then keeps that restored light and creates KeyLight1, and the next Undo deletes the restored light instead of the redone rig light. The command needs to restore the removed rig group/parent relationship or replay the exact captured snapshots instead of relying on current rig-group discovery.
Useful? React with 👍 / 👎.
| UndoManager::getSingleton()->push(new ApplyLightRigCommand(result)); | ||
| maybeLoadSuggestedHdri(result.suggestedHdri); |
There was a problem hiding this comment.
The rig application command is pushed before loading the suggested HDRI, so the HDRI/skybox change is outside the undoable state. Applying, for example, Outdoor Sunset over a Studio environment and then pressing Undo restores only lights and ambient; the sunset skybox remains active, and Redo from the undo path calls LightRigLibrary::apply() without reloading the suggested HDRI. Capture the previous/current environment in the command or keep the HDRI load out of the undoable rig apply.
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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Manager.cpp (1)
128-141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the LightManager connection out of the Manager constructor
src/Manager.cpp:137
Manager::m_pSingletonis still null while this constructor runs, soLightManager::tryConnectToManager()returns before wiringsceneNodeDestroyed. Connect it afternew Manager(parent)returns, or in a later init step.🤖 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/Manager.cpp` around lines 128 - 141, Move the LightManager hookup out of Manager::Manager so it runs only after Manager::m_pSingleton has been assigned. The current call to LightManager::getSingleton()->tryConnectToManager() happens too early in the constructor, causing the connection logic to return before sceneNodeDestroyed is wired; remove that call from Manager::Manager and perform it after new Manager(parent) completes or in a later initialization method that runs once the singleton is ready.src/commands/LightCommands_test.cpp (1)
1-90: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftNo tests for
ApplyLightRigCommandorSetSceneAmbientCommand.Both new command classes lack any coverage here. Adding an undo→redo→undo cycle test for
ApplyLightRigCommandin particular would have caught the stale-state issue flagged inLightCommands.cpp(redo() after undo doesn't refreshm_addedLights/m_rigGroupNodeName).As per coding guidelines: "Add Google Test unit tests for new functionality, and keep test files alongside source in
src/with the_test.cppsuffix."Want me to draft
ApplyLightRigCommand/SetSceneAmbientCommandtests, including a redo-after-undo-after-redo cycle that exercises the identified staleness bug?🤖 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/commands/LightCommands_test.cpp` around lines 1 - 90, The test suite in LightCommandsOgreTest only covers create/delete, duplicate, and rename flows, so add coverage for the new ApplyLightRigCommand and SetSceneAmbientCommand. Extend this test file with Google Test cases that exercise undo→redo→undo behavior for ApplyLightRigCommand, specifically verifying redo after undo refreshes state and does not reuse stale m_addedLights or m_rigGroupNodeName. Also add a basic undo/redo test for SetSceneAmbientCommand, using the existing LightManager, UndoManager, and LightsController helpers to validate the command lifecycle.Source: Coding guidelines
♻️ Duplicate comments (1)
src/PropertiesPanelController.cpp (1)
338-342: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRename result is ignored — see root-cause note in
LightsController.h.
renameLight(...)returnsvoidupstream, so this always emitsselectionChanged()even if the rename was rejected (empty/duplicate name). Fix at the source (LightsController::renameLight/LightsController.h) so this call site can react to failure.🤖 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/PropertiesPanelController.cpp` around lines 338 - 342, The rename flow in PropertiesPanelController::renameSceneTreeLight currently assumes LightsController::renameLight always succeeds, so selectionChanged() is emitted even when the rename is rejected. Update LightsController::renameLight in LightsController.h/.cpp to return a success/failure result for invalid or duplicate names, then adjust PropertiesPanelController::renameSceneTreeLight to emit selectionChanged() only on success and handle the failure path appropriately.
🧹 Nitpick comments (4)
src/PropertiesPanel_qml_test.cpp (1)
9-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWorking-directory-relative path is fragile across platforms/build layouts.
propertiesPanelQmlPath()assumes the test binary's CWD is exactly one level below theqml/directory. Multi-config build layouts (e.g. Visual Studio generators placing binaries underDebug//Release/) or CI invokingctestfrom a different working directory can break this on Windows even if it works on Linux/macOS CI.🛠️ Suggested alternative
Consider resolving the path from a compile-time-known source root (e.g. a CMake-configured header/definition, or
QFINDTESTDATA) instead ofQDir::currentPath().As per coding guidelines, "Code must compile and run on Windows, Linux (Ubuntu), and macOS."
🤖 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/PropertiesPanel_qml_test.cpp` around lines 9 - 12, The path lookup in propertiesPanelQmlPath() is using QDir::currentPath(), which makes the test depend on the process working directory and breaks in different build layouts. Update this helper to resolve PropertiesPanel.qml from a stable test/data location instead of the current directory, for example by using a compile-time configured source/test root or QFINDTESTDATA so the PropertiesPanel_qml_test.cpp lookup works across Windows, Linux, and macOS. Keep the change localized to propertiesPanelQmlPath().Source: Coding guidelines
src/LightRigLibrary.cpp (1)
368-407: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
apply()doesn't clean up an empty rig group on failure, and silently skips failed light creations.If
createLightUnderParentfails for a light spec, the loop justcontinues without recording the failure; and if it fails for all specs,result.okbecomesfalse(Line 403-406) but the just-created, taggedrigGroupnode (Line 368-375) is never destroyed, leaving orphaned scene state. Currently unreachable since every catalog entry has ≥1 light, but worth hardening for future rigs/failure modes.♻️ Possible hardening
mgr->getSceneMgr()->setAmbientLight(spec->ambient); result.ambientAfter = spec->ambient; result.suggestedHdri = spec->suggestedHdri; result.replaceExisting = replaceExisting; result.ok = !result.addedLights.isEmpty(); if (!result.ok) + { result.error = QStringLiteral("Rig produced no lights"); + destroyRigGroupNode(result.rigGroupNodeName); + }🤖 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 368 - 407, The apply() path in LightRigLibrary::apply leaves behind a tagged rigGroup node when no lights are successfully created, and it silently ignores per-light creation failures. Update the loop around createLightUnderParent to record failed light specs (or at least surface a failure when none succeed), and if result.ok remains false after the loop, destroy the rigGroup node before returning. Use the existing rigGroup, result.addedLights, and result.error handling in apply() to keep the cleanup and failure reporting localized.src/LightRigLibrary_test.cpp (1)
1-111: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test for
apply()with an unknown rig id.
LightRigLibrary::apply()'s error path ("Unknown light rig: %1") isn't exercised by any test here.As per coding guidelines: "Add Google Test unit tests for new functionality".
TEST_F(LightRigLibraryOgreTest, ApplyUnknownRig_ReturnsError) { const LightRigApplyResult result = LightRigLibrary::apply(QStringLiteral("does_not_exist"), true); EXPECT_FALSE(result.ok); EXPECT_FALSE(result.error.isEmpty()); }Otherwise the rest of the coverage (catalog size, apply results, ambient, HDRI suggestions, replace/no-replace, idempotent re-apply) looks solid.
🤖 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_test.cpp` around lines 1 - 111, Add a Google Test that exercises the error path in LightRigLibrary::apply() for an unknown rig id, since the current suite only covers valid rigs. Use the existing LightRigLibraryOgreTest fixture and call LightRigLibrary::apply() with a non-existent id, then assert result.ok is false and result.error is populated to cover the “Unknown light rig” behavior.Source: Coding guidelines
src/LightsController.cpp (1)
280-294: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftSilent no-op on rename failure.
If
findLight(oldName)misses orLightManager::renameLightfails (e.g. duplicate name), the function just returns with no signal/feedback. Downstream,qml/SceneTreeNode.qml's renameTextInput.onEditingFinished(lines 201-206) unconditionally setstreeNode.renaming = falseafter calling the rename, so a failed rename appears to silently do nothing with no user-visible error.Consider emitting a
renameFailed(QString reason)signal (or returning a status QML can react to) so the UI can surface why the rename didn't apply.🤖 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/LightsController.cpp` around lines 280 - 294, The rename flow in LightsController::renameLight currently returns silently when LightManager::findLight or LightManager::renameLight fails, so the UI cannot explain why the rename did not apply. Update LightsController::renameLight to report failure back to QML, either by emitting a renameFailed(QString reason) signal or by returning a status that qml/SceneTreeNode.qml can inspect, and make sure the failure paths for missing oldName and renameLight rejection provide a user-visible reason while preserving the existing success path that pushes RenameLightCommand and calls selectLightHandle.
🤖 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 `@qml/PropertiesPanel.qml`:
- Around line 3906-3925: The light color swatches in PropertiesPanel.qml are
mouse-only and need keyboard access. Update the Rectangle swatches used for
diffuse and the other color picker so they can receive focus, expose accessible
metadata, and trigger the same pickers from Enter/Space. Use the existing
LightPropertiesController.pickDiffuseColor() and the corresponding second picker
handler as the action targets, and add the keyboard handling to the swatch items
or their MouseArea so keyboard users can open them too.
- Around line 5045-5066: The “Apply preset rig” control in PropertiesPanel.qml
is mouse-only and needs keyboard accessibility. Update the Rectangle/MouseArea
pair for the applyRigMa block so it can receive tab focus, exposes an accessible
button role/name, and triggers SceneLightingController.applySelectedRig() from
keyboard activation as well as click. Keep the existing visual behavior, but
make the control reachable and operable without a mouse.
In `@qml/SceneTreeNode.qml`:
- Around line 90-102: Right-click on non-Light rows is falling through to the
generic selection path in SceneTreeNode.qml because onClicked only special-cases
treeNode.isLightType. Update the onClicked handler to ignore Qt.RightButton
unless the node is a Light with a context menu, and only call
treeModel.selectItem(...) for left-clicks (or other intended selection buttons);
use the existing onClicked, lightContextMenu.popup, treeNode.isLightType, and
treeModel.selectItem symbols to keep the behavior scoped correctly.
In `@src/commands/LightCommands.cpp`:
- Around line 209-218: ApplyLightRigCommand::redo() is leaving the command state
stale after undo/redo cycles because the result of LightRigLibrary::apply() is
ignored on subsequent redos. Update redo() so it refreshes m_addedLights,
m_rigGroupNodeName, and m_removedLights from the new LightRigApplyResult
returned by LightRigLibrary::apply(m_rigId, m_replaceExisting), keeping the
command’s undo state in sync with the latest apply.
In `@src/LightManager.cpp`:
- Around line 336-337: The Sentry breadcrumbs in LightManager use non-standard
categories, so update the addBreadcrumb calls in LightManager::createKeyLight,
LightManager::duplicateLight, and LightManager::renameLight to use one of the
approved categories instead of scene.light.create/duplicate/rename, while
keeping the existing message text intact.
- Around line 301-314: deleteAllUserLights() and clearAllLights() currently
duplicate the same loop-and-delete logic in LightManager, so consolidate them to
a single implementation to prevent drift; choose one method as the source of
truth and have the other delegate to it, keeping the deleteLight(name) behavior
unchanged and using the existing LightManager methods/fields (m_lights,
deleteLight) to preserve semantics.
- Around line 200-214: The stacking offset in LightManager::createLightInternal
is using m_lights.size(), which can repeat after deletions and cause new lights
to spawn on top of existing ones. Replace this size-based index with a monotonic
counter that only increases for each created light, and use that counter to
compute stackedOffset so each newly created light gets a unique position
regardless of removals.
In `@src/LightPropertiesController.cpp`:
- Around line 136-137: The breadcrumb category used in
LightPropertiesController::updateLightProperty is not one of the established
Sentry categories. Update the SentryReporter::addBreadcrumb call to use a valid
category from the approved set (ui.action, ai.tool_call, file.import, or
file.export) while keeping the existing message from
lightPropertyClassLabel(propertyClass). Ensure the breadcrumb still clearly
represents the light edit action but matches the coding guidelines.
In `@src/LightRigLibrary.cpp`:
- Around line 399-411: The breadcrumb in the rig-application path is using a
non-standard category, so update the SentryReporter::addBreadcrumb call in
LightRigLibrary::applyRig to use one of the established categories instead of
"scene.light.apply_rig". Keep the message detail (rigId) but route it through
the appropriate category for a user-facing or significant operation, consistent
with the breadcrumb guidelines.
In `@src/LightsController.cpp`:
- Around line 147-148: The Sentry breadcrumb categories in LightsController use
non-standard values for light create/delete actions. Update the
SentryReporter::addBreadcrumb calls in the light create and light delete paths
to use one of the established categories (such as ui.action) while keeping the
existing action-specific message, and ensure both the create and delete
breadcrumb sites are made consistent.
- Around line 184-216: The multi-light duplication flow in
duplicateSelectedLights is reselecting clones through selectLightHandle, which
uses single-selection behavior and clears earlier picks, so only the last
duplicated light remains selected. After clearing the current selection, update
this path to select all valid clone names in a multi-select way instead of
calling selectLightHandle in a loop, using the existing SelectionSet selection
API or an equivalent multi-selection helper so every entry in cloneNames stays
selected.
In `@src/LightsController.h`:
- Line 31: `LightsController::renameLight` currently hides rename rejection by
returning void, which causes callers like
`PropertiesPanelController::renameSceneTreeLight` to refresh the UI even when
the manager rejects the change. Update `renameLight` to return a success flag
and propagate the manager result from the rename path, then have
`renameSceneTreeLight` check that result before emitting `selectionChanged()`.
Keep the change aligned with the existing `LightsController` and
`PropertiesPanelController` methods so the UI only updates on successful
renames.
In `@src/LightVisualizer_test.cpp`:
- Around line 42-43: The Ogre fixture setup in LightVisualizer_test is missing
the mesh-file prerequisite check, so add an ASSERT_TRUE(canLoadMeshFiles())
alongside the existing ASSERT_TRUE(tryInitOgre()) in the fixture initialization
path before createStandardOgreMaterials() runs. Use the fixture setup code in
the test class to fail loudly in CI when mesh loading is unavailable, matching
the existing loud prerequisite pattern.
In `@src/LightVisualizer.cpp`:
- Around line 313-315: The breadcrumb in LightVisualizer’s user-facing View-menu
toggle uses a custom category instead of an established Sentry category. Update
the SentryReporter::addBreadcrumb calls in the relevant gizmo visibility
handlers to use the standard ui.action category while keeping the existing
show/hide message text, and apply the same change to the other matching
breadcrumb call in this file so both actions are consistent.
- Around line 419-422: The overlay insertion order in LightVisualizer is wrong:
updateOverlayVisibility(handle.name) runs before the new overlay is present in
mOverlays, so it can no-op and leave newly created unselected gizmos visible
when “selected gizmos only” is enabled. In the LightVisualizer update path
around rebuildGizmoGeometry and updateOverlayVisibility, insert the new entry
into mOverlays first, then call updateOverlayVisibility(handle.name) so the
visibility state is applied to the freshly created overlay.
- Around line 172-204: The cached icon material is not being cleared before
recreating it, so repeated calls to uploadIconTexture() can fail when
MaterialManager::create() sees an existing LightVisualizer/Icon/<name>/Mat.
Update uploadIconTexture() to mirror the existing texture cleanup by removing
the old material resource first, using the same texName-derived material name
before creating the new Ogre::MaterialPtr.
In `@src/mainwindow.cpp`:
- Around line 1287-1340: The new Add Light toolbar/menu actions in
MainWindow::setupObjectsToolbar are missing Sentry breadcrumbs, so update each
QAction trigger lambda in the light menu setup to call
SentryReporter::addBreadcrumb with the established ui.action category before
invoking LightsController::instance() or
SceneLightingController::instance().applyRig; include the light type or preset
rig name in the breadcrumb message, and apply the same pattern used by the
sibling actions later in this function so all user-facing lighting actions are
covered.
- Around line 2428-2435: m_lightVisualizer is being torn down too late, after
Manager::kill() can invalidate the Ogre scene manager it depends on. Move its
deletion into the early-teardown path in MainWindow, alongside m_meshInfoOverlay
and m_normalVisualizer, so LightVisualizer::~LightVisualizer() runs while
mSceneMgr is still valid. Use the m_lightVisualizer member and the existing
teardown block in MainWindow to place the cleanup in the correct order.
In `@src/SceneLightingController.cpp`:
- Around line 118-145: SceneLightingController::applyRig performs a major
user-facing scene mutation but does not record a Sentry breadcrumb. Add a
SentryReporter::addBreadcrumb call at the start of applyRig, using the
established "ui.action" category and a message describing the rig application
action, similar to the toolbar action breadcrumbs in mainwindow.cpp. Keep it
before the early returns so both successful and rejected attempts are captured.
In `@src/TransformOperator.cpp`:
- Around line 539-572: The removeSelected() flow is incorrectly short-circuiting
mixed selections by calling LightsController::instance()->deleteSelectedLights()
whenever all selected nodes are lights, which clears the entire SelectionSet and
drops any selected entities or sub-entities. Update the logic around
SelectionSet::getSingleton(), hasNodes(), and deleteSelectedLights() so this
branch only runs for node-only/light-only selections, or process the remaining
selection types before returning. Keep the fallback destroySceneNode loop and
selection cleanup path for mixed selections so nothing is lost.
---
Outside diff comments:
In `@src/commands/LightCommands_test.cpp`:
- Around line 1-90: The test suite in LightCommandsOgreTest only covers
create/delete, duplicate, and rename flows, so add coverage for the new
ApplyLightRigCommand and SetSceneAmbientCommand. Extend this test file with
Google Test cases that exercise undo→redo→undo behavior for
ApplyLightRigCommand, specifically verifying redo after undo refreshes state and
does not reuse stale m_addedLights or m_rigGroupNodeName. Also add a basic
undo/redo test for SetSceneAmbientCommand, using the existing LightManager,
UndoManager, and LightsController helpers to validate the command lifecycle.
In `@src/Manager.cpp`:
- Around line 128-141: Move the LightManager hookup out of Manager::Manager so
it runs only after Manager::m_pSingleton has been assigned. The current call to
LightManager::getSingleton()->tryConnectToManager() happens too early in the
constructor, causing the connection logic to return before sceneNodeDestroyed is
wired; remove that call from Manager::Manager and perform it after new
Manager(parent) completes or in a later initialization method that runs once the
singleton is ready.
---
Duplicate comments:
In `@src/PropertiesPanelController.cpp`:
- Around line 338-342: The rename flow in
PropertiesPanelController::renameSceneTreeLight currently assumes
LightsController::renameLight always succeeds, so selectionChanged() is emitted
even when the rename is rejected. Update LightsController::renameLight in
LightsController.h/.cpp to return a success/failure result for invalid or
duplicate names, then adjust PropertiesPanelController::renameSceneTreeLight to
emit selectionChanged() only on success and handle the failure path
appropriately.
---
Nitpick comments:
In `@src/LightRigLibrary_test.cpp`:
- Around line 1-111: Add a Google Test that exercises the error path in
LightRigLibrary::apply() for an unknown rig id, since the current suite only
covers valid rigs. Use the existing LightRigLibraryOgreTest fixture and call
LightRigLibrary::apply() with a non-existent id, then assert result.ok is false
and result.error is populated to cover the “Unknown light rig” behavior.
In `@src/LightRigLibrary.cpp`:
- Around line 368-407: The apply() path in LightRigLibrary::apply leaves behind
a tagged rigGroup node when no lights are successfully created, and it silently
ignores per-light creation failures. Update the loop around
createLightUnderParent to record failed light specs (or at least surface a
failure when none succeed), and if result.ok remains false after the loop,
destroy the rigGroup node before returning. Use the existing rigGroup,
result.addedLights, and result.error handling in apply() to keep the cleanup and
failure reporting localized.
In `@src/LightsController.cpp`:
- Around line 280-294: The rename flow in LightsController::renameLight
currently returns silently when LightManager::findLight or
LightManager::renameLight fails, so the UI cannot explain why the rename did not
apply. Update LightsController::renameLight to report failure back to QML,
either by emitting a renameFailed(QString reason) signal or by returning a
status that qml/SceneTreeNode.qml can inspect, and make sure the failure paths
for missing oldName and renameLight rejection provide a user-visible reason
while preserving the existing success path that pushes RenameLightCommand and
calls selectLightHandle.
In `@src/PropertiesPanel_qml_test.cpp`:
- Around line 9-12: The path lookup in propertiesPanelQmlPath() is using
QDir::currentPath(), which makes the test depend on the process working
directory and breaks in different build layouts. Update this helper to resolve
PropertiesPanel.qml from a stable test/data location instead of the current
directory, for example by using a compile-time configured source/test root or
QFINDTESTDATA so the PropertiesPanel_qml_test.cpp lookup works across Windows,
Linux, and macOS. Keep the change localized to propertiesPanelQmlPath().
🪄 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: d5e98f47-d5c5-4134-a56f-e3b295f1c6e5
📒 Files selected for processing (35)
qml/PropertiesPanel.qmlqml/SceneTreeNode.qmlsrc/AppSettingsKeys.hsrc/CMakeLists.txtsrc/GlobalDefinitions.hsrc/LightManager.cppsrc/LightManager.hsrc/LightManager_test.cppsrc/LightPropertiesController.cppsrc/LightPropertiesController.hsrc/LightPropertiesController_test.cppsrc/LightRigLibrary.cppsrc/LightRigLibrary.hsrc/LightRigLibrary_test.cppsrc/LightVisualizer.cppsrc/LightVisualizer.hsrc/LightVisualizer_test.cppsrc/LightsController.cppsrc/LightsController.hsrc/Manager.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/PropertiesPanel_qml_test.cppsrc/SceneLightingController.cppsrc/SceneLightingController.hsrc/SceneTreeModel.cppsrc/SceneTreeModel.hsrc/TransformOperator.cppsrc/commands/LightCommands.cppsrc/commands/LightCommands.hsrc/commands/LightCommands_test.cppsrc/mainwindow.cppsrc/mainwindow.hsrc/mainwindow_test.cppui_files/mainwindow.ui
| void SceneLightingController::applyRig(const QString& rigId, bool replaceExisting) | ||
| { | ||
| auto* lights = LightManager::getSingleton(); | ||
| if (!lights) | ||
| return; | ||
|
|
||
| QWidget* parent = Manager::getSingletonPtr() ? Manager::getSingleton()->getMainWindow() : nullptr; | ||
| if (replaceExisting && !lights->lights().isEmpty() | ||
| && !confirmReplaceExisting(parent)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const LightRigApplyResult result = LightRigLibrary::apply(rigId, replaceExisting); | ||
| if (!result.ok) | ||
| { | ||
| if (!result.error.isEmpty()) | ||
| { | ||
| QMessageBox::warning(parent, | ||
| QObject::tr("Light rig"), | ||
| result.error); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| UndoManager::getSingleton()->push(new ApplyLightRigCommand(result)); | ||
| maybeLoadSuggestedHdri(result.suggestedHdri); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing Sentry breadcrumb for a significant user action.
applyRig mutates the scene (destroys rig groups/lights, potentially deletes all user lights, pushes an undo command) but never calls SentryReporter::addBreadcrumb. Every comparable user-triggered operation elsewhere in this cohort (toolbar Extrude/Bevel/Delete/etc. in mainwindow.cpp) adds a "ui.action" breadcrumb.
📝 Proposed fix
void SceneLightingController::applyRig(const QString& rigId, bool replaceExisting)
{
auto* lights = LightManager::getSingleton();
if (!lights)
return;
+ SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+ QStringLiteral("Apply light rig: %1").arg(rigId));
+
QWidget* parent = Manager::getSingletonPtr() ? Manager::getSingleton()->getMainWindow() : nullptr;As per coding guidelines, "All user-facing actions and significant operations must add a Sentry breadcrumb via SentryReporter::addBreadcrumb(category, message) using the established categories (ui.action, ai.tool_call, file.import, file.export)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void SceneLightingController::applyRig(const QString& rigId, bool replaceExisting) | |
| { | |
| auto* lights = LightManager::getSingleton(); | |
| if (!lights) | |
| return; | |
| QWidget* parent = Manager::getSingletonPtr() ? Manager::getSingleton()->getMainWindow() : nullptr; | |
| if (replaceExisting && !lights->lights().isEmpty() | |
| && !confirmReplaceExisting(parent)) | |
| { | |
| return; | |
| } | |
| const LightRigApplyResult result = LightRigLibrary::apply(rigId, replaceExisting); | |
| if (!result.ok) | |
| { | |
| if (!result.error.isEmpty()) | |
| { | |
| QMessageBox::warning(parent, | |
| QObject::tr("Light rig"), | |
| result.error); | |
| } | |
| return; | |
| } | |
| UndoManager::getSingleton()->push(new ApplyLightRigCommand(result)); | |
| maybeLoadSuggestedHdri(result.suggestedHdri); | |
| } | |
| void SceneLightingController::applyRig(const QString& rigId, bool replaceExisting) | |
| { | |
| auto* lights = LightManager::getSingleton(); | |
| if (!lights) | |
| return; | |
| SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), | |
| QStringLiteral("Apply light rig: %1").arg(rigId)); | |
| QWidget* parent = Manager::getSingletonPtr() ? Manager::getSingleton()->getMainWindow() : nullptr; | |
| if (replaceExisting && !lights->lights().isEmpty() | |
| && !confirmReplaceExisting(parent)) | |
| { | |
| return; | |
| } | |
| const LightRigApplyResult result = LightRigLibrary::apply(rigId, replaceExisting); | |
| if (!result.ok) | |
| { | |
| if (!result.error.isEmpty()) | |
| { | |
| QMessageBox::warning(parent, | |
| QObject::tr("Light rig"), | |
| result.error); | |
| } | |
| return; | |
| } | |
| UndoManager::getSingleton()->push(new ApplyLightRigCommand(result)); | |
| maybeLoadSuggestedHdri(result.suggestedHdri); | |
| } |
🤖 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/SceneLightingController.cpp` around lines 118 - 145,
SceneLightingController::applyRig performs a major user-facing scene mutation
but does not record a Sentry breadcrumb. Add a SentryReporter::addBreadcrumb
call at the start of applyRig, using the established "ui.action" category and a
message describing the rig application action, similar to the toolbar action
breadcrumbs in mainwindow.cpp. Keep it before the early returns so both
successful and rejected attempts are captured.
Source: Coding guidelines
| SentryReporter::addBreadcrumb("ui.action", "Remove selected objects"); | ||
| SelectionSet* pCurrentSelection = SelectionSet::getSingleton(); | ||
| if (pCurrentSelection->isEmpty()) | ||
| return; | ||
|
|
||
| if (pCurrentSelection->hasNodes()) | ||
| { | ||
| bool allLights = true; | ||
| for (Ogre::SceneNode* node : pCurrentSelection->getNodesSelectionList()) | ||
| { | ||
| foreach(Ogre::SceneNode* node,SelectionSet::getSingleton()->getNodesSelectionList()) | ||
| { | ||
| Manager::getSingleton()->destroySceneNode(node); | ||
| } | ||
| pCurrentSelection->clearList(); | ||
| if (!LightManager::sceneNodeIsUserLight(node)) | ||
| { | ||
| allLights = false; | ||
| break; | ||
| } | ||
| } | ||
| if (allLights) | ||
| { | ||
| LightsController::instance()->deleteSelectedLights(); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Clear undo stack — destroyed nodes invalidate any stored commands | ||
| UndoManager::getSingleton()->clear(); | ||
| if (!pCurrentSelection->isEmpty()) | ||
| { | ||
| foreach (Ogre::SceneNode* node, SelectionSet::getSingleton()->getNodesSelectionList()) | ||
| { | ||
| Manager::getSingleton()->destroySceneNode(node); | ||
| } | ||
| pCurrentSelection->clearList(); | ||
|
|
||
| // Clear undo stack — destroyed nodes invalidate any stored commands | ||
| UndoManager::getSingleton()->clear(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant code and nearby selection APIs.
for f in src/TransformOperator.cpp src/SelectionSet.* src/LightsController.* src/Manager.*; do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
fi
done
echo "===== TransformOperator.cpp around removeSelected ====="
sed -n '520,590p' src/TransformOperator.cpp
echo "===== SelectionSet declarations/definitions ====="
grep -nE 'isEmpty|hasNodes|hasSubEntities|hasEntities|getNodesSelectionList|getSubEntitiesSelectionList|getEntitiesSelectionList|clearList|remove' -n src/SelectionSet.* || true
echo "===== LightsController relevant parts ====="
grep -nE 'deleteSelectedLights|isLightNode|sceneNodeIsUserLight|getSceneNode' -n src/LightsController.* src/TransformOperator.cpp src/Manager.* || trueRepository: fernandotonon/QtMeshEditor
Length of output: 9003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant implementation slices if the above search shows them.
sed -n '1,260p' src/SelectionSet.h 2>/dev/null || true
sed -n '1,320p' src/SelectionSet.cpp 2>/dev/null || true
sed -n '1,260p' src/LightsController.h 2>/dev/null || true
sed -n '1,360p' src/LightsController.cpp 2>/dev/null || trueRepository: fernandotonon/QtMeshEditor
Length of output: 21767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether delete/remove handling exists for entities or sub-entities elsewhere.
rg -n "hasEntities\\(|hasSubEntities\\(|getEntitiesSelectionList\\(|getSubEntitiesSelectionList\\(|deleteSelected.*Entity|destroyEntity|destroySubEntity|removeSelected" src | sed -n '1,220p'Repository: fernandotonon/QtMeshEditor
Length of output: 12552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect tests around removeSelected to understand intended behavior.
sed -n '640,730p' src/TransformOperator_test.cpp
echo "===== nearby removeSelected references in commands ====="
sed -n '140,220p' src/commands/TransformCommands.cppRepository: fernandotonon/QtMeshEditor
Length of output: 6524
removeSelected() should not short-circuit mixed selections
deleteSelectedLights() clears the whole SelectionSet, so the light-only branch drops any selected entities/sub-entities without deleting them. Restrict this path to node-only selections, or handle the remaining selection types before returning.
🤖 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/TransformOperator.cpp` around lines 539 - 572, The removeSelected() flow
is incorrectly short-circuiting mixed selections by calling
LightsController::instance()->deleteSelectedLights() whenever all selected nodes
are lights, which clears the entire SelectionSet and drops any selected entities
or sub-entities. Update the logic around SelectionSet::getSingleton(),
hasNodes(), and deleteSelectedLights() so this branch only runs for
node-only/light-only selections, or process the remaining selection types before
returning. Keep the fallback destroySceneNode loop and selection cleanup path
for mixed selections so nothing is lost.
Register light epic sources in qtmesh_test_common, pair outdoor overcast with overcast_outdoor.hdr, and harden rig undo/redo, overlay visibility, multi-select duplicate, and teardown order. Co-authored-by: Cursor <cursoragent@cursor.com>
Capture the active environment before applying a preset rig so undo restores the previous skybox and redo reloads the rig's suggested HDRI. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed review feedback in b29fe3e + f5b3822:
Remaining known limitation (P2): undoing a rig apply restores removed rig lights as root-level nodes rather than re-parenting into the old rig group — follow-up if we want full rig-group snapshot restore. |
Remove existing icon materials before recreate so a second MainWindow in tests does not crash, and resolve PropertiesPanel.qml via QTMESH_UT_SOURCE_ROOT in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/LightCommands.cpp`:
- Around line 290-295: The recreated rig and light names are not being refreshed
after redo, so undo can miss the new objects if name collisions caused suffixes
to be appended. In `LightCommands::redo()`, capture the actual names returned by
`LightRigLibrary::createRigGroupForRig()` and `restoreSnapshotUnderParent()` and
update `m_rigGroupNodeName` and each `m_addedLights[].name` accordingly so later
`undo()` targets the recreated rig/light nodes correctly.
🪄 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: da1208a7-6ae9-4967-998f-1a0df86c2491
📒 Files selected for processing (14)
qml/SceneTreeNode.qmlsrc/LightManager.cppsrc/LightManager.hsrc/LightPropertiesController.cppsrc/LightRigLibrary.cppsrc/LightRigLibrary.hsrc/LightVisualizer.cppsrc/LightsController.cppsrc/PropertiesPanel_qml_test.cppsrc/SceneLightingController.cppsrc/commands/LightCommands.cppsrc/commands/LightCommands.hsrc/mainwindow.cpptests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (11)
- src/PropertiesPanel_qml_test.cpp
- qml/SceneTreeNode.qml
- src/commands/LightCommands.h
- src/LightManager.h
- src/LightVisualizer.cpp
- src/SceneLightingController.cpp
- src/LightRigLibrary.cpp
- src/LightPropertiesController.cpp
- src/mainwindow.cpp
- src/LightsController.cpp
- src/LightManager.cpp
| Ogre::SceneNode* rigGroup = LightRigLibrary::createRigGroupForRig(m_rigId); | ||
| if (!rigGroup) | ||
| return; | ||
|
|
||
| for (const LightSnapshot& snapshot : m_addedLights) | ||
| lights->restoreSnapshotUnderParent(rigGroup, snapshot); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect rig-group node and light naming to confirm determinism across delete+recreate.
fd -a 'LightManager.cpp' src | xargs -r -I{} ast-grep run --pattern 'Ogre::SceneNode* $_::createRigGroupNode($$$) { $$$ }' --lang cpp {}
rg -nP -C3 '\bcreateRigGroupNode\b|\brestoreSnapshotUnderParent\b|\brestoreSnapshot\b' src/LightManager.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1496
🏁 Script executed:
#!/bin/bash
sed -n '272,430p' src/LightManager.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 6051
🏁 Script executed:
#!/bin/bash
sed -n '1,360p' src/commands/LightCommands.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 11841
🏁 Script executed:
#!/bin/bash
sed -n '1,260p' src/LightRigLibrary.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 10338
🏁 Script executed:
#!/bin/bash
sed -n '260,420p' src/LightRigLibrary.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 6408
Refresh the recreated rig/light names in redo() src/commands/LightCommands.cpp:290-295 — createRigGroupForRig() and restoreSnapshotUnderParent() can append suffixes on name collisions, but this command keeps the original m_rigGroupNodeName and m_addedLights[].name; capture the returned names so a later undo() still targets the recreated objects.
🤖 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/commands/LightCommands.cpp` around lines 290 - 295, The recreated rig and
light names are not being refreshed after redo, so undo can miss the new objects
if name collisions caused suffixes to be appended. In `LightCommands::redo()`,
capture the actual names returned by `LightRigLibrary::createRigGroupForRig()`
and `restoreSnapshotUnderParent()` and update `m_rigGroupNodeName` and each
`m_addedLights[].name` accordingly so later `undo()` targets the recreated
rig/light nodes correctly.
Fix LightVisualizer teardown exceptions, restore rig group parentage on undo, add keyboard/a11y for lighting controls, unify clearAllLights, and add missing ui.action breadcrumbs. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-up in db21018 (review + Sonar fixes):
Prior CI rerun: unit-tests-linux passed (32m). Watching new run for Sonar quality gate. |
canLoadMeshFiles() was checked before tryInitOgre(), so the suite always GTEST_SKIP'd under CI's no-skipped-tests policy. Co-authored-by: Cursor <cursoragent@cursor.com>
|
CI fix in d62720e: |
|
1 participant
Summary
Test plan
./build_local/bin/UnitTests --gtest_filter="Light*"(25 tests)Closes #483, #484, #485, #486, #487
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX