Lights: Slice F — shadow controls (#488) - #822
Conversation
Wire ShadowController with PSSM directional shadows, per-light cast/receive toggles, and Object Mode Tools UI (Light + Shadow sections). Fix rig-group deletion crashes and show receive-shadows when a scene node owns a mesh. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 19 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 (7)
📝 WalkthroughWalkthroughThis PR adds a global Estimated code review effort: 4 (Complex) | ~75 minutes ChangesShadow Controls Feature
Sequence Diagram(s)sequenceDiagram
participant QML as shadowToolsComponent
participant LPC as LightPropertiesController
participant LM as LightManager
participant SC as ShadowController
participant Ogre as Ogre SceneManager
QML->>LPC: setCastShadows(value)
LPC->>LM: applySnapshotToHandle(castShadows)
LM->>SC: syncFromScene()
SC->>SC: anyUserLightCastsShadows()
SC->>Ogre: installSceneShadows() / uninstallSceneShadows()
SC->>SC: refreshViewports()
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2503f80f77
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mainwindow.cpp (1)
530-550: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
ShadowController::kill()is skipped whenManageris already null, unlike other guideline calls in this block.
ShadowController::kill()is nested insideif (manager) {...}, so ifManagerwas already destroyed by another test suite (the exact scenario this block's comment anticipates), theShadowControllersingleton is never killed. SinceShadowController's destructor already null-checksManager::getSingletonPtr()internally, it's safe to call unconditionally. Leaving it alive acrossMainWindowinstances means itsLightManager::lightChanged/lightDeletedconnections stay bound to the destroyedLightManager, and a subsequentMainWindow's newLightManagernever gets wired up to shadow sync.🔧 Suggested fix
EditorModeController::kill(); + ShadowController::kill(); Manager* manager = Manager::getSingletonPtr(); if (manager) { ... - SceneLightingController::kill(); - ShadowController::kill(); + SceneLightingController::kill(); IsometricSpritesController::kill();🤖 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/mainwindow.cpp` around lines 530 - 550, Move ShadowController::kill() out of the Manager null check in MainWindow teardown so it is always invoked alongside the other controller cleanup calls. Keep the existing manager guard for the controllers that require a live Manager, but call ShadowController::kill() unconditionally because its destructor already handles a missing Manager safely. Use the surrounding shutdown sequence in mainwindow.cpp to place it with the other singleton kill calls.src/Manager.cpp (1)
641-700: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep recursive scene-node deletion opt-in
src/Manager.h:83,src/Manager.cpp:641-700— defaultingdestroyChildrenFirsttotruemakes every existingdestroySceneNode(node)call recurse through the full subtree and emitsceneNodeDestroyedper descendant. That broadens the fix beyond rig-group cleanup and adds extra signal/work on larger hierarchies.🤖 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 641 - 700, The recursive child-deletion behavior in destroySceneNode is too broad because the default now affects every existing destroySceneNode(node) call. Update Manager::destroySceneNode and its declaration in Manager.h so recursion stays opt-in: keep the default non-recursive behavior unless callers explicitly pass destroyChildrenFirst, and limit the child-first path to the specific rig-group cleanup use case while preserving the existing sceneNodeDestroyed and destroyAllAttachedMovableObjects flow.
🧹 Nitpick comments (2)
src/PropertiesPanelController.cpp (2)
482-540: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate traversal logic between
receiveShadows()andmixedReceiveShadows().Both methods run the same entity/sub-entity traversal and first-value-divergence comparison; consider extracting a shared private helper (e.g. returning
{bool hasMixed, bool value}) to avoid maintaining two copies of this logic.🤖 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 482 - 540, The traversal and comparison logic is duplicated in PropertiesPanelController::receiveShadows() and PropertiesPanelController::mixedReceiveShadows(), so factor the shared entity/sub-entity scan into a single private helper and have both methods use it. Keep the behavior identical by preserving the current empty-selection defaults and the first-value/divergence check, and use the helper to return both whether values are mixed and the resolved value.
334-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant with
Manager::destroySceneNode's new built-in rig-light cleanup.
Manager::destroySceneNode(defaultdestroyChildrenFirst=true) now walks children and callsLightManager::deleteLightBySceneNodeper child automatically — proven by the newLightManager_test.cppDestroyingRigGroupRemovesChildLightstest which destroys a rig-group node directly and gets all 3 lights removed. This manual pre-deletion loop (enumerate lights, compare parent,deleteLight()one by one) duplicates that behavior. Worth consolidating to just fall through to the generic path below, keeping only the distinct breadcrumb message/explicitselectionChanged()emit if still desired.♻️ Possible simplification
- Ogre::SceneNode* node = Manager::getSingleton()->getSceneNode(nodeName); - if (node && LightRigLibrary::sceneNodeIsRigGroup(node)) - { - SentryReporter::addBreadcrumb("ui.action", "Scene tree: delete light rig group"); - QStringList lightNames; - if (auto* lights = LightManager::getSingletonPtr()) - { - for (const LightHandle& handle : lights->lights()) - { - if (!handle.sceneNode) - continue; - if (static_cast<Ogre::SceneNode*>(handle.sceneNode->getParent()) == node) - lightNames.append(handle.name); - } - for (const QString& lightName : lightNames) - lights->deleteLight(lightName); - } - Manager::getSingleton()->destroySceneNode(nodeName); - SelectionSet::getSingleton()->clearList(); - UndoManager::getSingleton()->clear(); - emit selectionChanged(); - return; - } + Ogre::SceneNode* node = Manager::getSingleton()->getSceneNode(nodeName); + if (node && LightRigLibrary::sceneNodeIsRigGroup(node)) + SentryReporter::addBreadcrumb("ui.action", "Scene tree: delete light rig group");Then let it fall through to the common
destroySceneNode/clearList/UndoManager::clearpath, addingemit selectionChanged();there if needed for both 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/PropertiesPanelController.cpp` around lines 334 - 357, The rig-group delete branch in PropertiesPanelController::deleteSceneNode is now duplicating cleanup already handled by Manager::destroySceneNode, so remove the manual LightManager::lights() scan and per-light deleteLight calls. Keep the ui.action breadcrumb and then let the code fall through to the shared destroySceneNode/SelectionSet::clearList/UndoManager::clear path, using emit selectionChanged() there if both branches should notify the UI.
🤖 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 24-60: The InspectorThemedCheckBox component currently renders
icon-only checkboxes with empty text, so the actual CheckBox has no accessible
label. Update InspectorThemedCheckBox in PropertiesPanel.qml to expose an
accessible name/label for assistive tech, and make sure the Enabled, Receive
shadows, and Cast shadows instances provide a meaningful label even when their
visible text stays in a separate sibling Text. Prefer wiring the label through
the CheckBox accessibility properties rather than relying on the decorative text
content.
- Around line 5416-5426: The Cast shadows checkbox is writing back during
binding refresh instead of only on explicit user changes. Update the
InspectorThemedCheckBox handler in PropertiesPanel.qml so
LightPropertiesController.castShadows is set only for a real user action, not
when checkState changes due to selection/state sync; use the existing checkState
logic in this row to ignore programmatic updates and only call setCastShadows
through the relevant user-driven signal or guard inside onCheckStateChanged.
In `@src/LightManager.cpp`:
- Around line 471-487: `LightManager::deleteLightBySceneNode` should resolve the
tracked light directly from the `Ogre::SceneNode*` instead of falling back to
`node->getName()`, since renames can break the cleanup path. Update the
pointer-based lookup used by `findLightBySceneNode`/`deleteLight(handle->name)`
so the teardown works even after a scene-node rename, and keep the user-light
guard in place for non-tracked nodes.
In `@src/LightPropertiesController.cpp`:
- Around line 747-748: The shadow-toggle breadcrumb in LightPropertiesController
should use an existing UI action category instead of introducing
scene.light.shadow_toggle. Update the SentryReporter::addBreadcrumb call in the
shadow toggle handling code to use the established ui.action category while
keeping the on/off message, so it groups correctly with other user-facing
actions.
In `@src/OgreWidget.cpp`:
- Around line 134-137: Avoid force-creating ShadowController during OgreWidget
teardown: OgreWidget::~OgreWidget currently calls
ShadowController::instance()->unregisterViewport(this), which can resurrect the
singleton after MainWindow has already shut it down. Update the destructor to
use a null-safe accessor or explicit guard before unregistering, while keeping
the HdrViewportController cleanup and the unregisterViewport path in
OgreWidget::~OgreWidget from touching a dead ShadowController.
In `@src/PropertiesPanelController.cpp`:
- Around line 460-478: The setEntityReceiveShadows helper is modifying shared
Ogre materials directly, so the Receive Shadows toggle is leaking to other
entities. Update this function to make a unique material copy for each subentity
before calling setReceiveShadows, and avoid recompiling the same material
multiple times when several subentities reference it by tracking
already-processed materials inside setEntityReceiveShadows.
In `@src/ShadowController.cpp`:
- Around line 268-323: Add a Sentry breadcrumb to each global shadow-setting
setter in ShadowController so user-facing changes are tracked: setQualityPreset,
setCascadeCount, setSplitLambda, and setSpotShadowResolution should call
SentryReporter::addBreadcrumb with an appropriate category and message when the
value actually changes, alongside the existing QSettings persistence, emit
settingsChanged, and syncFromScene flow. Keep the breadcrumb placement in these
methods so the shadow toggle/update action is recorded before returning.
- Around line 216-239: The preset initialization in ShadowController is
overwriting user-saved overrides for cascade count, split lambda, and spot
shadow resolution on startup. Update ShadowController::ShadowController so it
does not blindly call applyPresetDefaults(m_qualityPreset) after loading the
persisted values, or change applyPresetDefaults and setQualityPreset to preserve
the individually stored overrides unless the preset is explicitly meant to
replace them. Make sure the behavior around QSettings, applyPresetDefaults, and
setQualityPreset is consistent so custom per-property values survive app
relaunches and preset changes.
- Line 442: The scene-wide shadow setup in ShadowController::installSceneShadows
is applying PSSM globally, which unintentionally affects spot and point casters.
Update the shadow camera setup logic so only directional lights use the PSSM
setup, and ensure spot/point lights keep the default perspective shadow camera
unless they have an explicit custom setup. Use the installSceneShadows path and
any light-specific setup checks to scope the PSSM assignment correctly.
- Around line 77-126: The texture lookup in ensureOgreShadowResources() is
checking TextureManager::resourceExists() too early, so the bundled
spot_shadow_fade.dds may not be found from the file-system resource location and
the fallback is always created. Update the resource check to use
ResourceGroupManager::resourceExists() for the internal group, then load the
texture explicitly through TextureManager only when it is actually present; keep
the 4x4 manual texture creation as the fallback path.
---
Outside diff comments:
In `@src/mainwindow.cpp`:
- Around line 530-550: Move ShadowController::kill() out of the Manager null
check in MainWindow teardown so it is always invoked alongside the other
controller cleanup calls. Keep the existing manager guard for the controllers
that require a live Manager, but call ShadowController::kill() unconditionally
because its destructor already handles a missing Manager safely. Use the
surrounding shutdown sequence in mainwindow.cpp to place it with the other
singleton kill calls.
In `@src/Manager.cpp`:
- Around line 641-700: The recursive child-deletion behavior in destroySceneNode
is too broad because the default now affects every existing
destroySceneNode(node) call. Update Manager::destroySceneNode and its
declaration in Manager.h so recursion stays opt-in: keep the default
non-recursive behavior unless callers explicitly pass destroyChildrenFirst, and
limit the child-first path to the specific rig-group cleanup use case while
preserving the existing sceneNodeDestroyed and destroyAllAttachedMovableObjects
flow.
---
Nitpick comments:
In `@src/PropertiesPanelController.cpp`:
- Around line 482-540: The traversal and comparison logic is duplicated in
PropertiesPanelController::receiveShadows() and
PropertiesPanelController::mixedReceiveShadows(), so factor the shared
entity/sub-entity scan into a single private helper and have both methods use
it. Keep the behavior identical by preserving the current empty-selection
defaults and the first-value/divergence check, and use the helper to return both
whether values are mixed and the resolved value.
- Around line 334-357: The rig-group delete branch in
PropertiesPanelController::deleteSceneNode is now duplicating cleanup already
handled by Manager::destroySceneNode, so remove the manual
LightManager::lights() scan and per-light deleteLight calls. Keep the ui.action
breadcrumb and then let the code fall through to the shared
destroySceneNode/SelectionSet::clearList/UndoManager::clear path, using emit
selectionChanged() there if both branches should notify the UI.
🪄 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: 5b0486a6-022f-4a5b-852d-997ed1daa20c
📒 Files selected for processing (24)
qml/PropertiesPanel.qmlqml/TransformField.qmlsrc/AppSettingsKeys.hsrc/CMakeLists.txtsrc/LightManager.cppsrc/LightManager.hsrc/LightManager_test.cppsrc/LightPropertiesController.cppsrc/LightPropertiesController.hsrc/LightRigLibrary.cppsrc/LightVisualizer_test.cppsrc/Manager.cppsrc/Manager.hsrc/OgreWidget.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/PropertiesPanelController_test.cppsrc/ShadowController.cppsrc/ShadowController.hsrc/ShadowController_test.cppsrc/commands/LightCommands.cppsrc/commands/LightCommands.hsrc/mainwindow.cpptests/CMakeLists.txt
Stop applyPresetDefaults from overwriting saved cascade/split/spot settings on startup, resync shadows after light restore/create, clone per-submesh materials for receive-shadow edits, and clear PSSM when no directionals cast. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid the PropertiesPanelQmlTest ThemedCheckBox substring guard while keeping the themed 16px checkbox component for shadow/light controls. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove redundant branches in hasActiveRenderContext, guard checkbox bindings from spurious writes, use ui.action breadcrumbs, and always tear down ShadowController even when Manager was already destroyed. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pushed
Earlier commit |
|
1 participant
Summary
ShadowControllerwith global shadow quality (Off/Low/Medium/High), PSSM cascades, spot map resolution, and lazy install/uninstall of Ogre texture-modulative shadows when any user light casts.LightPropertiesController, per-mesh receive shadows viaPropertiesPanelController, and reorganize UI into Object Mode Tools (Light + Shadow sections) with themed checkboxes.hasMeshInSelection).Test plan
UnitTests --gtest_filter="ShadowController*:LightManager*:PropertiesPanelControllerTests.HasMeshInSelection*"Closes #488
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Style