Lights: Slice I — light linking (#491) - #828
Conversation
Map light linking to Ogre light masks with inspector UI, scene JSON persistence, and unit tests for include/exclude semantics. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 43 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 (5)
📝 WalkthroughWalkthroughThis PR implements light linking (Slice I ChangesLight Linking Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PropertiesPanel
participant LightPropertiesController
participant LightManager
participant LightLinking
participant OgreEntity
User->>PropertiesPanel: select linkMode / add linked entity
PropertiesPanel->>LightPropertiesController: setLinkMode / addLinkedEntity
LightPropertiesController->>LightManager: update LightSnapshot (linkMode, linkedEntityNames)
LightManager->>LightLinking: applyFromSnapshot(snapshot)
LightLinking->>LightLinking: allocate channel bit, update rule map
LightLinking->>OgreEntity: update entity light mask
LightManager->>LightManager: applySnapshotToHandle writes link_* bindings
User->>PropertiesPanel: create new mesh entity
PropertiesPanel->>LightManager: Manager::createEntity
LightManager->>LightLinking: onEntityCreated(entity)
LightLinking->>OgreEntity: apply existing rules to mask
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: 9835c87dee
ℹ️ 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".
| LightSnapshot linkSnap = snapshot; | ||
| LightLinking::applyFromSnapshot(linkSnap); |
There was a problem hiding this comment.
When restoreSnapshotUnderParent() imports a saved light, undoes a delete, or duplicates a light, it calls applySnapshotToHandle() before the new handle is appended to m_lights; this new call then uses LightManager::findLight(snapshot.name) inside LightLinking::applyFromSnapshot(), so the just-created light is not found and its include/exclude mask is never applied. As a result, linked lights restored from saved scenes or undo/duplicate operations come back with the default mask and affect all meshes until some later edit reapplies linking.
Useful? React with 👍 / 👎.
| for (Ogre::Entity* entity : allEntities()) | ||
| entity->setLightMask(entity->getLightMask() & ~channelBit); |
There was a problem hiding this comment.
For an exclude rule, excluded entities are set to exactly channelBit; clearing the channel with mask & ~channelBit therefore turns those entities' masks into 0 when the rule is deleted or changed to an include rule. A zero entity light mask no longer intersects ordinary/default lights, so deleting or retargeting an exclude-linked light can leave the formerly excluded meshes completely unlit.
Useful? React with 👍 / 👎.
| if (rule.channelBit == 0) | ||
| rule.channelBit = allocateChannelBit(snapshot.name); |
There was a problem hiding this comment.
This only allocates a channel when the snapshot has linkChannelBit == 0, but duplicateLight() and restored snapshots copy the source light's persisted channel into a different light. Once two active lights share the same bit, editing one linked list clears/reassigns that shared bit on entities and changes which meshes the other light affects, so duplicated linked lights cannot be edited independently.
Useful? React with 👍 / 👎.
… picker Use controlBgColor/highlightColor like other inspector buttons (buttonColor was undefined on first paint). Exclude already-linked entities from the dropdown and clamp the combo index after add/remove. Co-authored-by: Cursor <cursoragent@cursor.com>
Light linking belongs with light properties, not under Shadow. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/LightLinking.h (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
<cstdint>foruint32_t.This header uses
uint32_t(Line 18, and downstream.cpp) but only includes<QStringList>.::uint32_trequires<cstdint>; relying on it being pulled in transitively via Qt is fragile and can break on stricter standard-library/compiler configurations across platforms.♻️ Add the header
`#include` <QStringList> +#include <cstdint>🤖 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/LightLinking.h` around lines 3 - 4, The header currently relies on a transitive Qt include for uint32_t, which is fragile across compilers and platforms. Update the LightLinking header to explicitly include <cstdint> alongside the existing QStringList include so the uint32_t type used in this header and its downstream implementation is declared directly.Source: Learnings
🤖 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 5700-5708: The helper text in the light linking section overstates
the available mask channels; update the Text element in PropertiesPanel.qml to
match LightLinking.h’s limit of 31 usable link rules with bit 0 reserved. Adjust
the qsTr copy in the light-properties help text so it no longer says “32 mask
channels” and instead reflects the actual maximum supported channels, keeping
the rest of the warning about RTSS PBR unchanged.
- Around line 5721-5822: The linked-entity action controls in PropertiesPanel
are mouse-only: `removeBtn` and `addLinkBtn` need keyboard accessibility to
match the existing action-button pattern used by controls like `diffuseColorBtn`
and `applyRigBtn`. Update the `Rectangle`/`MouseArea` delegates to be focusable
via tab, expose the appropriate accessibility role, and handle Space/Return to
trigger `LightPropertiesController.removeLinkedEntity(modelData)` and
`LightPropertiesController.addLinkedEntity(linkTargetPicker.currentText)`. Keep
the visual and enabled-state logic unchanged, but ensure both actions can be
reached and activated without a mouse.
In `@src/LightLinking_test.cpp`:
- Around line 17-24: The SetUp for the LightLinking test fixture is missing the
required Ogre runtime prerequisite check, so add an
ASSERT_TRUE(canLoadMeshFiles()) alongside the existing
ASSERT_TRUE(tryInitOgre()) in SetUp(). Keep the failure behavior loud in CI by
asserting both prerequisites before calling createStandardOgreMaterials(),
LightManager::getSingleton()->tryConnectToManager(), and
LightLinking::clearAllRules(), and do not replace this with any skip or
conditional fallback.
In `@src/LightLinking.cpp`:
- Around line 119-167: Exclude handling is overwriting existing entity
light-mask bits instead of preserving them. Update `applyExcludeRule` in
`LightLinking.cpp` so the excluded entity only has the current `channelBit`
cleared or applied without replacing the full mask, and make the same
mask-preserving change in the matching `onEntityCreated()` logic. Keep the rest
of the mask unchanged so include/exclude rules can compose correctly across
multiple lights.
In `@src/LightPropertiesController.cpp`:
- Around line 891-923: Add Sentry breadcrumbs for the user-facing linking
actions in LightPropertiesController: setLinkMode, addLinkedEntity, and
removeLinkedEntity currently update snapshot state without reporting UI
activity. Update these methods to call SentryReporter::addBreadcrumb(...) with a
ui.action category and a message/context that identifies the specific linking
action, using the same breadcrumb pattern already used elsewhere in
LightPropertiesController before the pushImmediateEdit calls.
- Around line 918-923: Reset the link mode state when the last linked entity is
removed in LightPropertiesController::removeLinkedEntity. After removing the
entity from snapshot.linkedEntityNames inside the
pushImmediateEdit(LightPropertyClass::Linking, ...) callback, check whether the
list is empty and, if so, clear snapshot.linkMode and snapshot.linkChannelBit to
match the behavior of setLinkMode(None).
---
Nitpick comments:
In `@src/LightLinking.h`:
- Around line 3-4: The header currently relies on a transitive Qt include for
uint32_t, which is fragile across compilers and platforms. Update the
LightLinking header to explicitly include <cstdint> alongside the existing
QStringList include so the uint32_t type used in this header and its downstream
implementation is declared directly.
🪄 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: 0392bc0d-fc7a-4e31-8c1a-c76847133ba9
📒 Files selected for processing (15)
CLAUDE.mdqml/PropertiesPanel.qmlsrc/CMakeLists.txtsrc/LightLinking.cppsrc/LightLinking.hsrc/LightLinking_test.cppsrc/LightManager.cppsrc/LightManager.hsrc/LightPropertiesController.cppsrc/LightPropertiesController.hsrc/Manager.cppsrc/SceneLightsIO.cppsrc/commands/LightCommands.cppsrc/commands/LightCommands.htests/CMakeLists.txt
| Text { | ||
| visible: LightPropertiesController.hasLightSelection | ||
| width: parent.width - 16 | ||
| wrapMode: Text.WordWrap | ||
| text: qsTr("Limit which meshes this light affects (32 mask channels). RTSS PBR may not honour masks on all passes.") | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 10 | ||
| opacity: 0.8 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Helper text overstates usable link channels.
LightLinking.h's own doc comment reserves bit 0 and caps rules at 31: "We allocate bits 1..31 per linked light (bit 0 reserved). At most 31 simultaneous link rules". The panel text says "(32 mask channels)", which could lead users to expect one more usable channel than actually exists.
📝 Suggested wording fix
- text: qsTr("Limit which meshes this light affects (32 mask channels). RTSS PBR may not honour masks on all passes.")
+ text: qsTr("Limit which meshes this light affects (up to 31 linked channels; channel 0 is reserved). RTSS PBR may not honour masks on all passes.")📝 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.
| Text { | |
| visible: LightPropertiesController.hasLightSelection | |
| width: parent.width - 16 | |
| wrapMode: Text.WordWrap | |
| text: qsTr("Limit which meshes this light affects (32 mask channels). RTSS PBR may not honour masks on all passes.") | |
| color: PropertiesPanelController.textColor | |
| font.pixelSize: 10 | |
| opacity: 0.8 | |
| } | |
| Text { | |
| visible: LightPropertiesController.hasLightSelection | |
| width: parent.width - 16 | |
| wrapMode: Text.WordWrap | |
| text: qsTr("Limit which meshes this light affects (up to 31 linked channels; channel 0 is reserved). RTSS PBR may not honour masks on all passes.") | |
| color: PropertiesPanelController.textColor | |
| font.pixelSize: 10 | |
| opacity: 0.8 | |
| } |
🤖 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 `@qml/PropertiesPanel.qml` around lines 5700 - 5708, The helper text in the
light linking section overstates the available mask channels; update the Text
element in PropertiesPanel.qml to match LightLinking.h’s limit of 31 usable link
rules with bit 0 reserved. Adjust the qsTr copy in the light-properties help
text so it no longer says “32 mask channels” and instead reflects the actual
maximum supported channels, keeping the rest of the warning about RTSS PBR
unchanged.
| Column { | ||
| visible: LightPropertiesController.hasLightSelection | ||
| && LightPropertiesController.linkMode !== 0 | ||
| && !LightPropertiesController.mixedLinkMode | ||
| spacing: 4 | ||
| width: parent.width - 16 | ||
| Repeater { | ||
| model: LightPropertiesController.linkedEntityNames | ||
| delegate: Row { | ||
| width: parent.width | ||
| spacing: 6 | ||
| Text { | ||
| width: parent.width - removeBtn.width - 6 | ||
| text: modelData | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 11 | ||
| elide: Text.ElideRight | ||
| } | ||
| Rectangle { | ||
| id: removeBtn | ||
| width: 18 | ||
| height: 18 | ||
| radius: 3 | ||
| color: removeMouse.pressed | ||
| ? Qt.darker(PropertiesPanelController.inputColor, 1.2) | ||
| : PropertiesPanelController.inputColor | ||
| border.color: PropertiesPanelController.borderColor | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: "×" | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 12 | ||
| } | ||
| MouseArea { | ||
| id: removeMouse | ||
| anchors.fill: parent | ||
| onClicked: LightPropertiesController.removeLinkedEntity(modelData) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Row { | ||
| spacing: 6 | ||
| width: parent.width | ||
| ThemedComboBox { | ||
| id: linkTargetPicker | ||
| width: parent.width - addLinkBtn.width - 6 | ||
| height: 22 | ||
| font.pixelSize: 11 | ||
| model: LightPropertiesController.availableLinkTargets | ||
| enabled: count > 0 | ||
| Connections { | ||
| target: LightPropertiesController | ||
| function onPropertiesChanged() { | ||
| if (linkTargetPicker.count === 0) | ||
| linkTargetPicker.currentIndex = -1 | ||
| else if (linkTargetPicker.currentIndex < 0 | ||
| || linkTargetPicker.currentIndex >= linkTargetPicker.count) | ||
| linkTargetPicker.currentIndex = 0 | ||
| } | ||
| } | ||
| } | ||
| Rectangle { | ||
| id: addLinkBtn | ||
| width: 44 | ||
| height: 22 | ||
| radius: 3 | ||
| opacity: linkTargetPicker.count > 0 && linkTargetPicker.currentIndex >= 0 | ||
| ? 1.0 | ||
| : 0.45 | ||
| color: addLinkMouse.containsMouse | ||
| && linkTargetPicker.count > 0 | ||
| && linkTargetPicker.currentIndex >= 0 | ||
| ? PropertiesPanelController.highlightColor | ||
| : PropertiesPanelController.controlBgColor | ||
| border.color: PropertiesPanelController.borderColor | ||
| border.width: 1 | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: qsTr("Add") | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 11 | ||
| } | ||
| MouseArea { | ||
| id: addLinkMouse | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| cursorShape: linkTargetPicker.count > 0 | ||
| && linkTargetPicker.currentIndex >= 0 | ||
| ? Qt.PointingHandCursor | ||
| : Qt.ArrowCursor | ||
| enabled: linkTargetPicker.count > 0 | ||
| && linkTargetPicker.currentIndex >= 0 | ||
| onClicked: { | ||
| if (linkTargetPicker.currentIndex >= 0) | ||
| LightPropertiesController.addLinkedEntity( | ||
| linkTargetPicker.currentText) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add/Remove linked-entity controls aren't keyboard accessible.
removeBtn and addLinkBtn are raw Rectangle + MouseArea with no activeFocusOnTab, Accessible.role, or Keys.onSpacePressed/onReturnPressed handlers. A keyboard-only user can tab into linkTargetPicker (native ComboBox, keyboard-accessible) and pick a target, but then has no keyboard path to actually press "Add" — same for removing an already-linked entity. Several sibling action controls in this same file (e.g. diffuseColorBtn, applyRigBtn) already follow the accessible pattern; the new controls should match it.
♿ Suggested fix
Rectangle {
id: removeBtn
width: 18
height: 18
radius: 3
+ activeFocusOnTab: true
+ Accessible.role: Accessible.Button
+ Accessible.name: qsTr("Remove linked entity")
color: removeMouse.pressed
? Qt.darker(PropertiesPanelController.inputColor, 1.2)
: PropertiesPanelController.inputColor
border.color: PropertiesPanelController.borderColor
+ Keys.onSpacePressed: LightPropertiesController.removeLinkedEntity(modelData)
+ Keys.onReturnPressed: LightPropertiesController.removeLinkedEntity(modelData)
Text { ... }
MouseArea {
id: removeMouse
anchors.fill: parent
onClicked: LightPropertiesController.removeLinkedEntity(modelData)
}
} Rectangle {
id: addLinkBtn
width: 44
height: 22
radius: 3
+ activeFocusOnTab: linkTargetPicker.count > 0 && linkTargetPicker.currentIndex >= 0
+ Accessible.role: Accessible.Button
+ Accessible.name: qsTr("Add linked entity")
+ Keys.onSpacePressed: if (linkTargetPicker.currentIndex >= 0)
+ LightPropertiesController.addLinkedEntity(linkTargetPicker.currentText)
+ Keys.onReturnPressed: if (linkTargetPicker.currentIndex >= 0)
+ LightPropertiesController.addLinkedEntity(linkTargetPicker.currentText)
...
}📝 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.
| Column { | |
| visible: LightPropertiesController.hasLightSelection | |
| && LightPropertiesController.linkMode !== 0 | |
| && !LightPropertiesController.mixedLinkMode | |
| spacing: 4 | |
| width: parent.width - 16 | |
| Repeater { | |
| model: LightPropertiesController.linkedEntityNames | |
| delegate: Row { | |
| width: parent.width | |
| spacing: 6 | |
| Text { | |
| width: parent.width - removeBtn.width - 6 | |
| text: modelData | |
| color: PropertiesPanelController.textColor | |
| font.pixelSize: 11 | |
| elide: Text.ElideRight | |
| } | |
| Rectangle { | |
| id: removeBtn | |
| width: 18 | |
| height: 18 | |
| radius: 3 | |
| color: removeMouse.pressed | |
| ? Qt.darker(PropertiesPanelController.inputColor, 1.2) | |
| : PropertiesPanelController.inputColor | |
| border.color: PropertiesPanelController.borderColor | |
| Text { | |
| anchors.centerIn: parent | |
| text: "×" | |
| color: PropertiesPanelController.textColor | |
| font.pixelSize: 12 | |
| } | |
| MouseArea { | |
| id: removeMouse | |
| anchors.fill: parent | |
| onClicked: LightPropertiesController.removeLinkedEntity(modelData) | |
| } | |
| } | |
| } | |
| } | |
| Row { | |
| spacing: 6 | |
| width: parent.width | |
| ThemedComboBox { | |
| id: linkTargetPicker | |
| width: parent.width - addLinkBtn.width - 6 | |
| height: 22 | |
| font.pixelSize: 11 | |
| model: LightPropertiesController.availableLinkTargets | |
| enabled: count > 0 | |
| Connections { | |
| target: LightPropertiesController | |
| function onPropertiesChanged() { | |
| if (linkTargetPicker.count === 0) | |
| linkTargetPicker.currentIndex = -1 | |
| else if (linkTargetPicker.currentIndex < 0 | |
| || linkTargetPicker.currentIndex >= linkTargetPicker.count) | |
| linkTargetPicker.currentIndex = 0 | |
| } | |
| } | |
| } | |
| Rectangle { | |
| id: addLinkBtn | |
| width: 44 | |
| height: 22 | |
| radius: 3 | |
| opacity: linkTargetPicker.count > 0 && linkTargetPicker.currentIndex >= 0 | |
| ? 1.0 | |
| : 0.45 | |
| color: addLinkMouse.containsMouse | |
| && linkTargetPicker.count > 0 | |
| && linkTargetPicker.currentIndex >= 0 | |
| ? PropertiesPanelController.highlightColor | |
| : PropertiesPanelController.controlBgColor | |
| border.color: PropertiesPanelController.borderColor | |
| border.width: 1 | |
| Text { | |
| anchors.centerIn: parent | |
| text: qsTr("Add") | |
| color: PropertiesPanelController.textColor | |
| font.pixelSize: 11 | |
| } | |
| MouseArea { | |
| id: addLinkMouse | |
| anchors.fill: parent | |
| hoverEnabled: true | |
| cursorShape: linkTargetPicker.count > 0 | |
| && linkTargetPicker.currentIndex >= 0 | |
| ? Qt.PointingHandCursor | |
| : Qt.ArrowCursor | |
| enabled: linkTargetPicker.count > 0 | |
| && linkTargetPicker.currentIndex >= 0 | |
| onClicked: { | |
| if (linkTargetPicker.currentIndex >= 0) | |
| LightPropertiesController.addLinkedEntity( | |
| linkTargetPicker.currentText) | |
| } | |
| } | |
| } | |
| } | |
| } | |
| Column { | |
| visible: LightPropertiesController.hasLightSelection | |
| && LightPropertiesController.linkMode !== 0 | |
| && !LightPropertiesController.mixedLinkMode | |
| spacing: 4 | |
| width: parent.width - 16 | |
| Repeater { | |
| model: LightPropertiesController.linkedEntityNames | |
| delegate: Row { | |
| width: parent.width | |
| spacing: 6 | |
| Text { | |
| width: parent.width - removeBtn.width - 6 | |
| text: modelData | |
| color: PropertiesPanelController.textColor | |
| font.pixelSize: 11 | |
| elide: Text.ElideRight | |
| } | |
| Rectangle { | |
| id: removeBtn | |
| width: 18 | |
| height: 18 | |
| radius: 3 | |
| activeFocusOnTab: true | |
| Accessible.role: Accessible.Button | |
| Accessible.name: qsTr("Remove linked entity") | |
| color: removeMouse.pressed | |
| ? Qt.darker(PropertiesPanelController.inputColor, 1.2) | |
| : PropertiesPanelController.inputColor | |
| border.color: PropertiesPanelController.borderColor | |
| Keys.onSpacePressed: LightPropertiesController.removeLinkedEntity(modelData) | |
| Keys.onReturnPressed: LightPropertiesController.removeLinkedEntity(modelData) | |
| Text { | |
| anchors.centerIn: parent | |
| text: "×" | |
| color: PropertiesPanelController.textColor | |
| font.pixelSize: 12 | |
| } | |
| MouseArea { | |
| id: removeMouse | |
| anchors.fill: parent | |
| onClicked: LightPropertiesController.removeLinkedEntity(modelData) | |
| } | |
| } | |
| } | |
| } | |
| Row { | |
| spacing: 6 | |
| width: parent.width | |
| ThemedComboBox { | |
| id: linkTargetPicker | |
| width: parent.width - addLinkBtn.width - 6 | |
| height: 22 | |
| font.pixelSize: 11 | |
| model: LightPropertiesController.availableLinkTargets | |
| enabled: count > 0 | |
| Connections { | |
| target: LightPropertiesController | |
| function onPropertiesChanged() { | |
| if (linkTargetPicker.count === 0) | |
| linkTargetPicker.currentIndex = -1 | |
| else if (linkTargetPicker.currentIndex < 0 | |
| || linkTargetPicker.currentIndex >= linkTargetPicker.count) | |
| linkTargetPicker.currentIndex = 0 | |
| } | |
| } | |
| } | |
| Rectangle { | |
| id: addLinkBtn | |
| width: 44 | |
| height: 22 | |
| radius: 3 | |
| activeFocusOnTab: linkTargetPicker.count > 0 && linkTargetPicker.currentIndex >= 0 | |
| Accessible.role: Accessible.Button | |
| Accessible.name: qsTr("Add linked entity") | |
| Keys.onSpacePressed: if (linkTargetPicker.currentIndex >= 0) | |
| LightPropertiesController.addLinkedEntity(linkTargetPicker.currentText) | |
| Keys.onReturnPressed: if (linkTargetPicker.currentIndex >= 0) | |
| LightPropertiesController.addLinkedEntity(linkTargetPicker.currentText) | |
| opacity: linkTargetPicker.count > 0 && linkTargetPicker.currentIndex >= 0 | |
| ? 1.0 | |
| : 0.45 | |
| color: addLinkMouse.containsMouse | |
| && linkTargetPicker.count > 0 | |
| && linkTargetPicker.currentIndex >= 0 | |
| ? PropertiesPanelController.highlightColor | |
| : PropertiesPanelController.controlBgColor | |
| border.color: PropertiesPanelController.borderColor | |
| border.width: 1 | |
| Text { | |
| anchors.centerIn: parent | |
| text: qsTr("Add") | |
| color: PropertiesPanelController.textColor | |
| font.pixelSize: 11 | |
| } | |
| MouseArea { | |
| id: addLinkMouse | |
| anchors.fill: parent | |
| hoverEnabled: true | |
| cursorShape: linkTargetPicker.count > 0 | |
| && linkTargetPicker.currentIndex >= 0 | |
| ? Qt.PointingHandCursor | |
| : Qt.ArrowCursor | |
| enabled: linkTargetPicker.count > 0 | |
| && linkTargetPicker.currentIndex >= 0 | |
| onClicked: { | |
| if (linkTargetPicker.currentIndex >= 0) | |
| LightPropertiesController.addLinkedEntity( | |
| linkTargetPicker.currentText) | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 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 `@qml/PropertiesPanel.qml` around lines 5721 - 5822, The linked-entity action
controls in PropertiesPanel are mouse-only: `removeBtn` and `addLinkBtn` need
keyboard accessibility to match the existing action-button pattern used by
controls like `diffuseColorBtn` and `applyRigBtn`. Update the
`Rectangle`/`MouseArea` delegates to be focusable via tab, expose the
appropriate accessibility role, and handle Space/Return to trigger
`LightPropertiesController.removeLinkedEntity(modelData)` and
`LightPropertiesController.addLinkedEntity(linkTargetPicker.currentText)`. Keep
the visual and enabled-state logic unchanged, but ensure both actions can be
reached and activated without a mouse.
| void SetUp() override | ||
| { | ||
| Manager::kill(); | ||
| ASSERT_TRUE(tryInitOgre()); | ||
| createStandardOgreMaterials(); | ||
| LightManager::getSingleton()->tryConnectToManager(); | ||
| LightLinking::clearAllRules(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add ASSERT_TRUE(canLoadMeshFiles()) to SetUp.
The fixture calls ASSERT_TRUE(tryInitOgre()) but is missing ASSERT_TRUE(canLoadMeshFiles()). Based on learnings, QtMeshEditor Ogre-dependent tests must use both checks to fail loudly in CI rather than silently skipping. Even though these tests create manual meshes, canLoadMeshFiles() validates the Ogre runtime environment is properly configured.
Based on learnings, "fixture SetUp must fail loudly in CI by using ASSERT_TRUE(tryInitOgre()) and ASSERT_TRUE(canLoadMeshFiles()). Do not use GTEST_SKIP() or any silent/conditional skipping for these Ogre-dependent prerequisites."
🛡️ Proposed fix
void SetUp() override
{
Manager::kill();
ASSERT_TRUE(tryInitOgre());
+ ASSERT_TRUE(canLoadMeshFiles());
createStandardOgreMaterials();
LightManager::getSingleton()->tryConnectToManager();
LightLinking::clearAllRules();
}📝 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 SetUp() override | |
| { | |
| Manager::kill(); | |
| ASSERT_TRUE(tryInitOgre()); | |
| createStandardOgreMaterials(); | |
| LightManager::getSingleton()->tryConnectToManager(); | |
| LightLinking::clearAllRules(); | |
| } | |
| void SetUp() override | |
| { | |
| Manager::kill(); | |
| ASSERT_TRUE(tryInitOgre()); | |
| ASSERT_TRUE(canLoadMeshFiles()); | |
| createStandardOgreMaterials(); | |
| LightManager::getSingleton()->tryConnectToManager(); | |
| LightLinking::clearAllRules(); | |
| } |
🤖 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/LightLinking_test.cpp` around lines 17 - 24, The SetUp for the
LightLinking test fixture is missing the required Ogre runtime prerequisite
check, so add an ASSERT_TRUE(canLoadMeshFiles()) alongside the existing
ASSERT_TRUE(tryInitOgre()) in SetUp(). Keep the failure behavior loud in CI by
asserting both prerequisites before calling createStandardOgreMaterials(),
LightManager::getSingleton()->tryConnectToManager(), and
LightLinking::clearAllRules(), and do not replace this with any skip or
conditional fallback.
Source: Learnings
| void applyExcludeRule(uint32_t channelBit, const QStringList& excludedNames) | ||
| { | ||
| QSet<QString> excluded; | ||
| for (const QString& name : excludedNames) | ||
| excluded.insert(name); | ||
|
|
||
| for (Ogre::Entity* entity : allEntities()) | ||
| { | ||
| const QString nodeName = | ||
| QString::fromStdString(entity->getParentSceneNode()->getName()); | ||
| if (excluded.contains(nodeName)) | ||
| { | ||
| // Mask must not overlap the light's (~channelBit) mask — only the bit itself. | ||
| entity->setLightMask(channelBit); | ||
| } | ||
| else if (entity->getLightMask() == channelBit) | ||
| { | ||
| entity->setLightMask(LightLinking::kDefaultMask); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void applyRule(const QString& lightName, const ActiveRule& rule) | ||
| { | ||
| auto* lights = LightManager::getSingletonPtr(); | ||
| if (!lights) | ||
| return; | ||
|
|
||
| const LightHandle* handle = lights->findLight(lightName); | ||
| if (!handle || !handle->isValid()) | ||
| return; | ||
|
|
||
| if (rule.mode == LightLinking::Mode::None || rule.channelBit == 0) | ||
| { | ||
| handle->light->setLightMask(LightLinking::kDefaultMask); | ||
| return; | ||
| } | ||
|
|
||
| if (rule.mode == LightLinking::Mode::Include) | ||
| { | ||
| handle->light->setLightMask(rule.channelBit); | ||
| applyIncludeRule(rule.channelBit, rule.entityNames); | ||
| } | ||
| else if (rule.mode == LightLinking::Mode::Exclude) | ||
| { | ||
| handle->light->setLightMask(LightLinking::kDefaultMask & ~rule.channelBit); | ||
| applyExcludeRule(rule.channelBit, rule.entityNames); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and nearby light-linking code.
git ls-files 'src/LightLinking.cpp' 'src/*LightLinking*' 'src/**/*LightLinking*' || true
# Map the structure first.
ast-grep outline src/LightLinking.cpp --view expanded || true
# Inspect the relevant ranges around the reported functions and related call sites.
sed -n '1,380p' src/LightLinking.cppRepository: fernandotonon/QtMeshEditor
Length of output: 9353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the tests and public API around the light-linking contract.
ast-grep outline src/LightLinking_test.cpp --view expanded || true
printf '\n--- HEADER ---\n'
cat -n src/LightLinking.h | sed -n '1,260p'
printf '\n--- TESTS ---\n'
cat -n src/LightLinking_test.cpp | sed -n '1,320p'Repository: fernandotonon/QtMeshEditor
Length of output: 6349
Exclude must preserve existing channel bits in src/LightLinking.cpp:119-167 and the matching onEntityCreated() path: assigning channelBit overwrites any previously accumulated masks, so multi-light include/exclude combinations stop composing correctly. Make the exclude path update only its own bit and leave the rest of the entity mask unchanged.
🤖 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/LightLinking.cpp` around lines 119 - 167, Exclude handling is overwriting
existing entity light-mask bits instead of preserving them. Update
`applyExcludeRule` in `LightLinking.cpp` so the excluded entity only has the
current `channelBit` cleared or applied without replacing the full mask, and
make the same mask-preserving change in the matching `onEntityCreated()` logic.
Keep the rest of the mask unchanged so include/exclude rules can compose
correctly across multiple lights.
| void LightPropertiesController::setLinkMode(int mode) | ||
| { | ||
| const LightLinking::Mode linkMode = static_cast<LightLinking::Mode>(std::clamp(mode, 0, 2)); | ||
| pushImmediateEdit(LightPropertyClass::Linking, [linkMode](LightSnapshot& snapshot) { | ||
| snapshot.linkMode = linkMode; | ||
| if (linkMode == LightLinking::Mode::None) | ||
| { | ||
| snapshot.linkedEntityNames.clear(); | ||
| snapshot.linkChannelBit = 0; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| void LightPropertiesController::addLinkedEntity(const QString& entityName) | ||
| { | ||
| const QString trimmed = entityName.trimmed(); | ||
| if (trimmed.isEmpty()) | ||
| return; | ||
|
|
||
| pushImmediateEdit(LightPropertyClass::Linking, [trimmed](LightSnapshot& snapshot) { | ||
| if (snapshot.linkMode == LightLinking::Mode::None) | ||
| snapshot.linkMode = LightLinking::Mode::Include; | ||
| if (!snapshot.linkedEntityNames.contains(trimmed)) | ||
| snapshot.linkedEntityNames.append(trimmed); | ||
| }); | ||
| } | ||
|
|
||
| void LightPropertiesController::removeLinkedEntity(const QString& entityName) | ||
| { | ||
| pushImmediateEdit(LightPropertyClass::Linking, [entityName](LightSnapshot& snapshot) { | ||
| snapshot.linkedEntityNames.removeAll(entityName); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing Sentry breadcrumbs for new user-facing linking actions.
setLinkMode, addLinkedEntity, and removeLinkedEntity are all invoked directly from the QML Light-linking UI (mode dropdown, Add/Remove buttons) but none of them record a breadcrumb, unlike what the repo's guideline requires for user-facing actions.
Based on coding guidelines: src/**/*.{h,cpp}: "Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb(...), with categories such as ui.action, ai.tool_call, file.import, and file.export."
🍞 Proposed fix (illustrative — match the exact call signature used elsewhere in the class)
void LightPropertiesController::setLinkMode(int mode)
{
const LightLinking::Mode linkMode = static_cast<LightLinking::Mode>(std::clamp(mode, 0, 2));
+ SentryReporter::addBreadcrumb("ui.action", "Set light link mode");
pushImmediateEdit(LightPropertyClass::Linking, [linkMode](LightSnapshot& snapshot) {
...
});
}
void LightPropertiesController::addLinkedEntity(const QString& entityName)
{
const QString trimmed = entityName.trimmed();
if (trimmed.isEmpty())
return;
+ SentryReporter::addBreadcrumb("ui.action", "Add light-linked entity");
...
}
void LightPropertiesController::removeLinkedEntity(const QString& entityName)
{
+ SentryReporter::addBreadcrumb("ui.action", "Remove light-linked entity");
pushImmediateEdit(LightPropertyClass::Linking, [entityName](LightSnapshot& snapshot) {
...
});
}📝 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 LightPropertiesController::setLinkMode(int mode) | |
| { | |
| const LightLinking::Mode linkMode = static_cast<LightLinking::Mode>(std::clamp(mode, 0, 2)); | |
| pushImmediateEdit(LightPropertyClass::Linking, [linkMode](LightSnapshot& snapshot) { | |
| snapshot.linkMode = linkMode; | |
| if (linkMode == LightLinking::Mode::None) | |
| { | |
| snapshot.linkedEntityNames.clear(); | |
| snapshot.linkChannelBit = 0; | |
| } | |
| }); | |
| } | |
| void LightPropertiesController::addLinkedEntity(const QString& entityName) | |
| { | |
| const QString trimmed = entityName.trimmed(); | |
| if (trimmed.isEmpty()) | |
| return; | |
| pushImmediateEdit(LightPropertyClass::Linking, [trimmed](LightSnapshot& snapshot) { | |
| if (snapshot.linkMode == LightLinking::Mode::None) | |
| snapshot.linkMode = LightLinking::Mode::Include; | |
| if (!snapshot.linkedEntityNames.contains(trimmed)) | |
| snapshot.linkedEntityNames.append(trimmed); | |
| }); | |
| } | |
| void LightPropertiesController::removeLinkedEntity(const QString& entityName) | |
| { | |
| pushImmediateEdit(LightPropertyClass::Linking, [entityName](LightSnapshot& snapshot) { | |
| snapshot.linkedEntityNames.removeAll(entityName); | |
| }); | |
| } | |
| void LightPropertiesController::setLinkMode(int mode) | |
| { | |
| const LightLinking::Mode linkMode = static_cast<LightLinking::Mode>(std::clamp(mode, 0, 2)); | |
| SentryReporter::addBreadcrumb("ui.action", "Set light link mode"); | |
| pushImmediateEdit(LightPropertyClass::Linking, [linkMode](LightSnapshot& snapshot) { | |
| snapshot.linkMode = linkMode; | |
| if (linkMode == LightLinking::Mode::None) | |
| { | |
| snapshot.linkedEntityNames.clear(); | |
| snapshot.linkChannelBit = 0; | |
| } | |
| }); | |
| } | |
| void LightPropertiesController::addLinkedEntity(const QString& entityName) | |
| { | |
| const QString trimmed = entityName.trimmed(); | |
| if (trimmed.isEmpty()) | |
| return; | |
| SentryReporter::addBreadcrumb("ui.action", "Add light-linked entity"); | |
| pushImmediateEdit(LightPropertyClass::Linking, [trimmed](LightSnapshot& snapshot) { | |
| if (snapshot.linkMode == LightLinking::Mode::None) | |
| snapshot.linkMode = LightLinking::Mode::Include; | |
| if (!snapshot.linkedEntityNames.contains(trimmed)) | |
| snapshot.linkedEntityNames.append(trimmed); | |
| }); | |
| } | |
| void LightPropertiesController::removeLinkedEntity(const QString& entityName) | |
| { | |
| SentryReporter::addBreadcrumb("ui.action", "Remove light-linked entity"); | |
| pushImmediateEdit(LightPropertyClass::Linking, [entityName](LightSnapshot& snapshot) { | |
| snapshot.linkedEntityNames.removeAll(entityName); | |
| }); | |
| } |
🤖 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/LightPropertiesController.cpp` around lines 891 - 923, Add Sentry
breadcrumbs for the user-facing linking actions in LightPropertiesController:
setLinkMode, addLinkedEntity, and removeLinkedEntity currently update snapshot
state without reporting UI activity. Update these methods to call
SentryReporter::addBreadcrumb(...) with a ui.action category and a
message/context that identifies the specific linking action, using the same
breadcrumb pattern already used elsewhere in LightPropertiesController before
the pushImmediateEdit calls.
Source: Coding guidelines
| void LightPropertiesController::removeLinkedEntity(const QString& entityName) | ||
| { | ||
| pushImmediateEdit(LightPropertyClass::Linking, [entityName](LightSnapshot& snapshot) { | ||
| snapshot.linkedEntityNames.removeAll(entityName); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant definitions and uses
rg -n "removeLinkedEntity|addLinkedEntity|setLinkMode|linkedEntityNames|linkMode|linkChannelBit|LightLinking" src/LightPropertiesController.cpp src -g '!**/build/**'
# Show the relevant section of LightPropertiesController.cpp with line numbers
sed -n '860,980p' src/LightPropertiesController.cpp
# Find the LightLinking logic that consumes linkMode / linkedEntityNames
rg -n "linkedEntityNames|linkMode|linkChannelBit|Include|None" src -g '!**/build/**'Repository: fernandotonon/QtMeshEditor
Length of output: 41116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the link-application logic and helper conversions
sed -n '90,230p' src/LightLinking.cpp
sed -n '230,320p' src/LightLinking.cpp
# Inspect the snapshot serialization/deserialization around link fields
sed -n '250,290p' src/SceneLightsIO.cpp
# Read the relevant tests for include/exclude behavior
sed -n '1,140p' src/LightLinking_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 11695
Reset link mode when the last linked entity is removed
removeLinkedEntity() can leave linkMode == Include with an empty linkedEntityNames list. That still applies an active include rule and clears the channel from all entities, so the light ends up affecting nothing. Mirror setLinkMode(None) here by clearing linkMode and linkChannelBit when the list becomes empty.
🤖 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/LightPropertiesController.cpp` around lines 918 - 923, Reset the link
mode state when the last linked entity is removed in
LightPropertiesController::removeLinkedEntity. After removing the entity from
snapshot.linkedEntityNames inside the
pushImmediateEdit(LightPropertyClass::Linking, ...) callback, check whether the
list is empty and, if so, clear snapshot.linkMode and snapshot.linkChannelBit to
match the behavior of setLinkMode(None).
Pass the live Ogre light into LightLinking::applyFromSnapshot so masks apply before the handle is registered (undo/restore/duplicate). Reallocate channel bits when a copied snapshot collides with another active light. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
Light::setLightMask/Entity::setLightMask(31 channel bits; bit 0 reserved).LightPropertyClass::Linking.linkMode,linkedEntities, andlinkChannelBitinqtmesh.scene.lightsJSON and light user bindings.Part 1 of #491 — remaining sub-features (IES, area lights, groups, per-viewport solo) will follow as separate PRs per issue guidance.
Test plan
xvfb-run ./build_local/bin/UnitTests --gtest_filter="LightLinkingOgreTest.*"(3/3 pass)Closes #491 partially (light linking only).
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes