feat: add Material Preset Library panel to Inspector - #249
Conversation
Wire MaterialPresetLibrary into the Inspector's Properties panel: - Register MaterialPresetLibrary as a QML singleton under PropertiesPanel module (same pattern as MeshValidator/MeshLodController) - Add "Material Presets" CollapsibleSection (collapsed by default), visible whenever an entity is selected - Preset buttons grouped by category (Plastic, Metal, Wood, Glass, Other) in a Flow layout; clicking a button calls MaterialPresetLibrary.applyPreset() - Feedback text shows the last applied preset name via onPresetApplied signal - Closes #208 (7b) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Material Presets UI to the Properties Panel (QML), registers and lifecycles a QML singleton MaterialPresetLibrary in MainWindow, updates material creation/application logic (including compile and color/ambient changes) and entity iteration, updates tests to new assertions/expected alpha, and tweaks CMake sentry FetchContent settings. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant UI as PropertiesPanel (QML)
participant Lib as PropertiesPanel.MaterialPresetLibrary
participant Scene as Scene/Entities
participant FB as Feedback Text
User->>UI: click preset thumbnail
UI->>Lib: applyPreset(presetName)
Lib->>Scene: resolve selection / create or fetch material
Scene-->>Lib: selection / entities list
Lib->>Lib: create/compile material (if needed)
Lib->>Scene: apply compiled material to resolved entities / sub-entities
Lib->>FB: emit onPresetApplied(presetName)
FB-->>User: display "Applied: <name>"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: aaa3c3e67e
ℹ️ 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".
| model: modelData.presets | ||
|
|
||
| Rectangle { | ||
| width: Math.min(110, (parent.width - (modelData.presets.length - 1) * 4) / modelData.presets.length) |
There was a problem hiding this comment.
Inside the inner Repeater delegate, modelData is the preset name string, not the outer category object, so modelData.presets.length is undefined here. That makes the width binding fail at runtime (QML JS error) and the preset rectangles end up with invalid/zero width, so the new Material Presets buttons do not render or are not clickable when the section is opened. Capture the outer category (or preset count) in a named property and reference that instead.
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: 2
🧹 Nitpick comments (1)
qml/PropertiesPanel.qml (1)
598-604: Consider sourcing preset names from backend to avoid UI/backend drift.
The category list is hardcoded in QML whileMaterialPresetLibraryalready exposespresetNames; deriving from backend data would reduce duplication risk over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/PropertiesPanel.qml` around lines 598 - 604, The hardcoded property var categories should be derived from the backend MaterialPresetLibrary to avoid drift: replace the static "categories" array in PropertiesPanel.qml with a computed value built from MaterialPresetLibrary.presetNames (or the equivalent exposed API) by grouping preset names into the desired category objects ({ label, presets }) at runtime; update any bindings that read "categories" to rely on this computed/grouping logic so the UI reflects backend changes automatically and remove the duplicated hardcoded list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 635-638: PropertiesPanelController lacks a Q_PROPERTY for
buttonColor referenced by QML, so add a QColor property named buttonColor to the
PropertiesPanelController class (declare Q_PROPERTY(QColor buttonColor READ
buttonColor WRITE setButtonColor NOTIFY buttonColorChanged), implement the
getter QColor buttonColor() const, setter void setButtonColor(const QColor&),
and the signal void buttonColorChanged()), initialize it in the
PropertiesPanelController constructor (or mirror existing borderColor logic) and
emit buttonColorChanged() in the setter; alternatively, update the QML to use
the existing borderColor property instead of buttonColor if you prefer not to
add a new property.
- Around line 627-632: The Rectangle delegate inside the nested Repeater is
shadowing the outer category's modelData (the preset string becomes the inner
modelData), causing modelData.presets.length to be undefined; fix by capturing
the outer category into a named property on the enclosing Column (e.g., property
var category = modelData or property var outerCategory = modelData) and update
the Rectangle width binding to use category.presets.length (or
outerCategory.presets.length) instead of modelData.presets.length so the width
calculation references the correct object and avoids scope shadowing.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 598-604: The hardcoded property var categories should be derived
from the backend MaterialPresetLibrary to avoid drift: replace the static
"categories" array in PropertiesPanel.qml with a computed value built from
MaterialPresetLibrary.presetNames (or the equivalent exposed API) by grouping
preset names into the desired category objects ({ label, presets }) at runtime;
update any bindings that read "categories" to rely on this computed/grouping
logic so the UI reflects backend changes automatically and remove the duplicated
hardcoded list.
🪄 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: a542271c-e003-484c-a345-19abd741f722
📒 Files selected for processing (2)
qml/PropertiesPanel.qmlsrc/mainwindow.cpp
| ? Qt.darker(PropertiesPanelController.buttonColor, 1.3) | ||
| : presetMouse.containsMouse | ||
| ? Qt.lighter(PropertiesPanelController.buttonColor, 1.15) | ||
| : PropertiesPanelController.buttonColor |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declaration + definition of PropertiesPanelController::buttonColor
# Expected:
# 1) declaration in src/PropertiesPanelController.h
# 2) definition in src/PropertiesPanelController.cpp
# If only declaration exists, this is a blocker for this change.
rg -nP '\bbuttonColor\s*\(' src/PropertiesPanelController.h src/PropertiesPanelController.cpp
rg -nP 'QColor\s+PropertiesPanelController::buttonColor\s*\(' src/PropertiesPanelController.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
fd -type f -name "*PropertiesPanelController*"Repository: fernandotonon/QtMeshEditor
Length of output: 240
🏁 Script executed:
rg -l "buttonColor" --type cpp --type hRepository: fernandotonon/QtMeshEditor
Length of output: 324
🏁 Script executed:
rg -B2 -A2 "buttonColor" qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 632
🏁 Script executed:
fd PropertiesPanelControllerRepository: fernandotonon/QtMeshEditor
Length of output: 174
🏁 Script executed:
rg "class PropertiesPanelController"Repository: fernandotonon/QtMeshEditor
Length of output: 301
🏁 Script executed:
rg -B5 "PropertiesPanelController\." qml/PropertiesPanel.qml | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 634
🏁 Script executed:
cat -n src/PropertiesPanelController.hRepository: fernandotonon/QtMeshEditor
Length of output: 8418
🏁 Script executed:
rg "buttonColor" src/PropertiesPanelController.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
rg "borderColor" src/PropertiesPanelController.cppRepository: fernandotonon/QtMeshEditor
Length of output: 123
🏁 Script executed:
rg "buttonColor" qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 334
PropertiesPanelController.buttonColor is not implemented.
The property is not declared in PropertiesPanelController.h and not defined in PropertiesPanelController.cpp. While borderColor exists and is properly implemented, buttonColor does not. This will cause a QML runtime error: "Cannot read property 'buttonColor' of object."
Either add the missing buttonColor property to PropertiesPanelController or use an existing property instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/PropertiesPanel.qml` around lines 635 - 638, PropertiesPanelController
lacks a Q_PROPERTY for buttonColor referenced by QML, so add a QColor property
named buttonColor to the PropertiesPanelController class (declare
Q_PROPERTY(QColor buttonColor READ buttonColor WRITE setButtonColor NOTIFY
buttonColorChanged), implement the getter QColor buttonColor() const, setter
void setButtonColor(const QColor&), and the signal void buttonColorChanged()),
initialize it in the PropertiesPanelController constructor (or mirror existing
borderColor logic) and emit buttonColorChanged() in the setter; alternatively,
update the QML to use the existing borderColor property instead of buttonColor
if you prefer not to add a new property.
- Replace flat buttons with Canvas-rendered sphere previews per preset, grouped by category (Plastic / Metal / Wood / Glass / Other) - Each 44px sphere uses a radial gradient (spec highlight → diffuse → dark edge); wireframe draws green latitude/longitude grid lines; glass adds a refraction rim pass - Selected preset gets a highlighted border so the active one is visible - Fix apply logic: use getResolvedEntities() so node-selected meshes also get the material (was checking hasEntities()&&hasSubEntities() — both had to be true simultaneously, so nothing was ever applied) - Fix nested Repeater modelData shadowing by capturing outer data in property var catPresets on the delegate Column - Add mat->compile() after creating preset materials to force RTSS rebuild Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MaterialPresetLibrary.cpp (1)
44-50:⚠️ Potential issue | 🟡 MinorOnly signal success after at least one target was updated.
presetApplied(name)is emitted unconditionally, so an empty selection or a selection that resolves to zero targets still produces a green “Applied” message in QML. It also creates/reuses the preset material even though nothing receives it.Guard the zero-target path up front
void MaterialPresetLibrary::applyPreset(const QString& name) { auto* sel = SelectionSet::getSingleton(); + QList<Ogre::Entity*> entities; + QList<Ogre::SubEntity*> subEntities; + if (sel) { + entities = sel->getResolvedEntities(); + subEntities = sel->getSubEntitiesSelectionList(); + } + if (entities.isEmpty() && subEntities.isEmpty()) + return; SentryReporter::addBreadcrumb("ui.action", QString("Apply material preset: %1").arg(name)); @@ // Apply to resolved entities (handles node selection as well as direct entity/sub-entity selection) std::string stdMatName = matName.toStdString(); - for (Ogre::Entity* ent : sel->getResolvedEntities()) + for (Ogre::Entity* ent : entities) ent->setMaterialName(stdMatName); - for (Ogre::SubEntity* sub : sel->getSubEntitiesSelectionList()) + for (Ogre::SubEntity* sub : subEntities) sub->setMaterialName(stdMatName); emit presetApplied(name); }Also applies to: 105-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialPresetLibrary.cpp` around lines 44 - 50, The code currently emits presetApplied(name) and creates/reuses the preset material even when the selection yields zero targets; change the logic in MaterialPresetLibrary (the block using SelectionSet::getSingleton(), SentryReporter::addBreadcrumb(...), and Ogre::MaterialManager::getSingletonPtr()) to first resolve the selection and count targets, and if and only if at least one target will be updated proceed to create/reuse the material and call presetApplied(name); for the early-return path (and similarly in the other occurrence around lines handling the same flow at the 105-112 area) avoid creating materials or emitting presetApplied when targetCount == 0 and instead return/exit quietly.
🧹 Nitpick comments (1)
qml/PropertiesPanel.qml (1)
599-613: Avoid hardcoding the preset catalog in both QML and C++.The preset names now live here and again in
src/MaterialPresetLibrary.cpp::presetNames()/applyPreset(). A future rename or new preset can desync the grid from the backend, and the current backend path will still create/apply an unconfigured material for an unknown string. Expose one preset model fromMaterialPresetLibraryand drive the UI from that source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/PropertiesPanel.qml` around lines 599 - 613, The UI currently hardcodes property var presets and categories in PropertiesPanel.qml which duplicates and can desync from the backend MaterialPresetLibrary (presetNames(), applyPreset()); replace the hardcoded presets/categories by exposing a QAbstractListModel or QVariantList from MaterialPresetLibrary (e.g. add a Q_PROPERTY model/getPresetModel() or context property) and bind PropertiesPanel.qml to that model (drive labels, diff/spec/shin/alpha/wire/unlit and derived categories from the model) so the QML reads the canonical preset data; also ensure MaterialPresetLibrary::applyPreset() tolerates unknown names (no-op or fallback) and update presetNames() to reflect the model source rather than duplicated literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 745-753: The preset tile is only clickable via MouseArea (id:
sphereArea) and lacks keyboard focus and activation; make the tile
keyboard-accessible by enabling focus and tab navigation (e.g., set
activeFocusOnTab: true on a containing Item or FocusScope and ensure sphereArea
can accept focus), add Keys handlers on sphereArea (Keys.onEnterPressed and
Keys.onSpacePressed) to call
MaterialPresetLibrary.applyPreset(sphereItem.pdata.name) and set
presetsRoot.lastApplied = sphereItem.pdata.name, and add a visible focus
affordance (toggle a focused style on the tile when sphereArea.focus is true) so
keyboard users can see and activate presets.
---
Outside diff comments:
In `@src/MaterialPresetLibrary.cpp`:
- Around line 44-50: The code currently emits presetApplied(name) and
creates/reuses the preset material even when the selection yields zero targets;
change the logic in MaterialPresetLibrary (the block using
SelectionSet::getSingleton(), SentryReporter::addBreadcrumb(...), and
Ogre::MaterialManager::getSingletonPtr()) to first resolve the selection and
count targets, and if and only if at least one target will be updated proceed to
create/reuse the material and call presetApplied(name); for the early-return
path (and similarly in the other occurrence around lines handling the same flow
at the 105-112 area) avoid creating materials or emitting presetApplied when
targetCount == 0 and instead return/exit quietly.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 599-613: The UI currently hardcodes property var presets and
categories in PropertiesPanel.qml which duplicates and can desync from the
backend MaterialPresetLibrary (presetNames(), applyPreset()); replace the
hardcoded presets/categories by exposing a QAbstractListModel or QVariantList
from MaterialPresetLibrary (e.g. add a Q_PROPERTY model/getPresetModel() or
context property) and bind PropertiesPanel.qml to that model (drive labels,
diff/spec/shin/alpha/wire/unlit and derived categories from the model) so the
QML reads the canonical preset data; also ensure
MaterialPresetLibrary::applyPreset() tolerates unknown names (no-op or fallback)
and update presetNames() to reflect the model source rather than duplicated
literals.
🪄 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: aae8e818-c2d0-4d50-9e3d-455783759223
📒 Files selected for processing (2)
qml/PropertiesPanel.qmlsrc/MaterialPresetLibrary.cpp
| MouseArea { | ||
| id: sphereArea | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| cursorShape: Qt.PointingHandCursor | ||
| onClicked: { | ||
| MaterialPresetLibrary.applyPreset(sphereItem.pdata.name) | ||
| presetsRoot.lastApplied = sphereItem.pdata.name | ||
| } |
There was a problem hiding this comment.
Preset tiles need a keyboard path.
These items are clickable only via MouseArea. There is no tab focus, no focus affordance, and no Enter/Space activation, so keyboard users cannot apply presets.
Minimal fix
Column {
id: sphereItem
spacing: 2
property var pdata: modelData
+ activeFocusOnTab: true
+
+ function applyPreset() {
+ MaterialPresetLibrary.applyPreset(pdata.name)
+ presetsRoot.lastApplied = pdata.name
+ }
+
+ Keys.onReturnPressed: applyPreset()
+ Keys.onEnterPressed: applyPreset()
+ Keys.onSpacePressed: applyPreset()
// Sphere canvas
Rectangle {
width: 52; height: 52; radius: 4
color: sphereArea.containsMouse
? Qt.lighter(PropertiesPanelController.panelColor, 1.4)
: PropertiesPanelController.panelColor
- border.color: presetsRoot.lastApplied === sphereItem.pdata.name
- ? PropertiesPanelController.highlightColor
- : PropertiesPanelController.borderColor
+ border.color: (presetsRoot.lastApplied === sphereItem.pdata.name || sphereItem.activeFocus)
+ ? PropertiesPanelController.highlightColor
+ : PropertiesPanelController.borderColor
border.width: presetsRoot.lastApplied === sphereItem.pdata.name ? 2 : 1
@@
MouseArea {
id: sphereArea
anchors.fill: parent
hoverEnabled: true
cursorShape: Qt.PointingHandCursor
- onClicked: {
- MaterialPresetLibrary.applyPreset(sphereItem.pdata.name)
- presetsRoot.lastApplied = sphereItem.pdata.name
- }
+ onClicked: sphereItem.applyPreset()
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/PropertiesPanel.qml` around lines 745 - 753, The preset tile is only
clickable via MouseArea (id: sphereArea) and lacks keyboard focus and
activation; make the tile keyboard-accessible by enabling focus and tab
navigation (e.g., set activeFocusOnTab: true on a containing Item or FocusScope
and ensure sphereArea can accept focus), add Keys handlers on sphereArea
(Keys.onEnterPressed and Keys.onSpacePressed) to call
MaterialPresetLibrary.applyPreset(sphereItem.pdata.name) and set
presetsRoot.lastApplied = sphereItem.pdata.name, and add a visible focus
affordance (toggle a focused style on the tile when sphereArea.focus is true) so
keyboard users can see and activate presets.
…gnal emission - Fix GlassPresetConfiguresTransparentMaterialProperties: expected alpha was 0.3f but Glass (Tinted) sets 0.55f — caused CI unit-tests-linux failure - Replace deprecated Ogre::SharedPtr::isNull() with bool(mat) check - Guard presetApplied emission: only emit when at least one entity/sub-entity was actually updated (CodeRabbit review on PR #249) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MaterialPresetLibrary.cpp`:
- Around line 107-110: The selection path treating "node-only" selections as
name-sensitive fails because SelectionSet::getResolvedEntities() uses
sceneMgr->getEntity(node->getName()), which misses entities whose names differ
from their nodes; modify getResolvedEntities() to resolve the entity by
iterating the selected node's attachedObject(s) (or using
node->getAttachedObject(i)) and collect any attached Entity instances instead of
relying on sceneMgr lookup, then ensure MaterialPresetLibrary's check (where
resolvedEntities and subEntities are queried) will find entities for node-only
selections; also add a regression test that creates a node/entity pair with
mismatched names (e.g., "PlasticNode" node with "PlasticEntity" entity) and
verifies the node-selected mesh flow applies presets 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: 2fca7366-d7f6-4156-aabc-45911aab344c
📒 Files selected for processing (2)
src/MaterialPresetLibrary.cppsrc/MaterialPresetLibrary_test.cpp
| auto resolvedEntities = sel->getResolvedEntities(); | ||
| auto subEntities = sel->getSubEntitiesSelectionList(); | ||
|
|
||
| if (resolvedEntities.isEmpty() && subEntities.isEmpty()) |
There was a problem hiding this comment.
Node-only selection is still name-sensitive.
sel->getResolvedEntities() currently resolves selected nodes via sceneMgr->getEntity(node->getName()) in src/SelectionSet.cpp:403-425. That means this path still no-ops when a node and its attached entity use different names, so Line 110 returns without applying anything. The test helper already creates pairs like ("PlasticNode", "PlasticEntity"), so the advertised “node-selected mesh” flow is still brittle. Please resolve attached entities from the selected node itself and add a regression test with mismatched node/entity names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MaterialPresetLibrary.cpp` around lines 107 - 110, The selection path
treating "node-only" selections as name-sensitive fails because
SelectionSet::getResolvedEntities() uses sceneMgr->getEntity(node->getName()),
which misses entities whose names differ from their nodes; modify
getResolvedEntities() to resolve the entity by iterating the selected node's
attachedObject(s) (or using node->getAttachedObject(i)) and collect any attached
Entity instances instead of relying on sceneMgr lookup, then ensure
MaterialPresetLibrary's check (where resolvedEntities and subEntities are
queried) will find entities for node-only selections; also add a regression test
that creates a node/entity pair with mismatched names (e.g., "PlasticNode" node
with "PlasticEntity" entity) and verifies the node-selected mesh flow applies
presets correctly.
…oglesource.com sentry-native's crashpad submodule recursively fetches from chromium.googlesource.com, which rate-limits GitHub Actions (HTTP 429). Since we use SENTRY_BACKEND=inproc, crashpad is unused. Adding GIT_SUBMODULES_RECURSE FALSE stops the deep recursive clone. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
1 participant
Summary
MaterialPresetLibrary(previously unconnected backend) into the Inspector panelMaterialPresetLibrary.applyPreset(); feedback text confirms the applied presetTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit