feat(materials): Inspector preview pane + Material Editor redesign (Phase 5 slice I) - #492
Conversation
…hase 5 slice I) Adds an interactive material preview to the Inspector's Material Mode panel and redesigns the Material Editor window around a single Form-or- Script toggle with Pass | Texture sub-tabs. - MaterialPreviewRenderer gains renderInteractivePreview(size, shape, yaw) with a Sphere/Cube/Plane Q_ENUM, an env-light yaw control, and a separate resizable RTT (cached thumbnail path is unchanged). - MaterialEditorQML exposes inputColor + headerColor so themed QML controls share the Inspector's palette vocabulary, plus a thin interactiveMaterialPreview wrapper. - PropertiesPanelController.applyMaterialToSelection touches selected sub-entities (or every sub-entity of selected entities) and refreshes the scene-tree material columns via selectionChanged. - Material Editor: viewMode "form"|"script" toggle, sticky Technique / Pass context strip with "+ Add new…" sentinel rows, Pass | Texture sub-tab bar, Texture-Unit row below the tabs (no layout shift), form ColumnLayout fills the ScrollView width. - Inspector: Material Library grid (deferred 100 ms first paint to avoid Ogre RTT avalanche) and interactive preview Sphere/Cube switch with drag-yaw envmap. - New ThemedCheckBox QML primitive, GroupBox flat-header restyling across PassPropertiesPanel / TexturePropertiesPanel. Tests: - BoneDragRelease_test: 3 new tests driving multi-event setUpdate sequences (5/drag, 20/drag×3, zero-delta clear). - MaterialPreviewRenderer_test: 7 new tests covering size clamping, yaw wrap, shape switch, out-of-range fallback, RTT reuse, resize. - MaterialEditorQML_test: 3 new theme-color tests + 3 interactive preview tests. - PropertiesPanelController_test: 5 new applyMaterialToSelection tests (empty name, no selection, sub-entity priority, all-submeshes, selectionChanged signal). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR implements an inline Material Library with interactive, rotation-controlled material previews. It shifts UI theming from Qt's SystemPalette to MaterialEditorQML-provided colors, redesigns the Material Editor into a compact Form/Script-mode window, and updates all UI components to a flat Inspector aesthetic with themed variants (ThemedCheckBox, restyled button/field/spinbox). The Material Library tool allows users to browse, select, apply, edit, and export materials with live interactive shape- and yaw-adjustable previews. ChangesMaterial Library & Interactive Previews
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5c1084fdf7
ℹ️ 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_sphere = m_sceneMgr->createEntity("PreviewSphereEntity", meshName); | ||
| m_sphereNode->attachObject(m_sphere); | ||
| m_interactiveCurrentShape = requestedShape; |
There was a problem hiding this comment.
renderInteractivePreview mutates the shared preview entity/light state (m_sphere mesh and yawed light direction), but renderPreview still assumes a fixed sphere + default lighting and caches only by material name. After a user switches to Cube (or drags yaw), the next uncached thumbnail render can be generated with the wrong shape/lighting, producing inconsistent material cards and violating the documented “always Sphere + yaw=0” behavior. Use separate scene objects for interactive rendering or restore canonical sphere/light state before thumbnail rendering.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
qml/MaterialEditorWindow.qml (2)
48-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
statusTextis only defined inside the Script pane — Form mode silently drops AI status updates.The
Connectionshandlers above assign tostatusText.text/statusText.color, butstatusTextlives at line 860 inside the script-viewRectangle. WhenviewMode === "form", the Script pane is hidden (visible: falseat line 672), so the user never sees AI generation/error feedback. The QML reference still resolves (the Item exists in the tree), so there's no runtime error, but functionally the status surface is invisible during the default Form view.Consider hoisting the status indicator out of the Script pane (e.g., into the top toolbar next to the Form | Script toggle) so AI/SD progress remains visible regardless of
viewMode.
1064-1072:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHeuristic material-text guard yields false positives.
This check rejects any
materialTextthat contains the substring"import","Item", or"Rectangle"anywhere — including inside comments, string literals, or material names. A perfectly valid material with// imports basecolor mapor a texture filename likeItem_albedo.pngwill be silently replaced by the hard-coded default.If the goal is to catch QML accidentally ending up in this field, anchor the pattern (e.g., a regex matching
^\s*import\s+\wat line start) rather than a free substring scan.🤖 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/MaterialEditorWindow.qml` around lines 1064 - 1072, The current guard in the text property replaces any materialText containing the substrings "import", "Item", or "Rectangle", causing false positives; update the check in the text block that reads MaterialEditorQML.materialText to use anchored/multiline regexes instead of indexOf: test for QML imports only at the start of a line (e.g. /^\s*import\s+\w/m) and for QML root types only when they appear as a standalone token at the start of a line (e.g. /^\s*(Item|Rectangle)\b/m); keep returning the hard-coded default only if one of these anchored regex tests matches or the trimmed materialText is empty.
🧹 Nitpick comments (8)
src/MaterialPreviewRenderer_test.cpp (1)
261-426: ⚡ Quick winAdd a thumbnail-after-interactive regression case.
These tests never exercise
renderPreview()orrenderPreviewAsDataUri()after switching shape or yaw, so the shared-state leak between the interactive and cached preview paths would still pass this suite. One test that renders a cube/plane first and then asserts the thumbnail path returns the default sphere/default-light output would lock that down.🤖 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/MaterialPreviewRenderer_test.cpp` around lines 261 - 426, The test suite misses a regression case where state from renderInteractivePreview leaks into the cached thumbnail path; add a test in MaterialPreviewRendererTests that first calls MaterialPreviewRenderer::renderInteractivePreview with a non-sphere shape (e.g., ShapeCube or ShapePlane) and/or non-zero yaw, then calls the thumbnail path (MaterialPreviewRenderer::renderPreview or renderPreviewAsDataUri) for the same material and asserts the thumbnail equals the canonical sphere/default-light thumbnail (obtained by calling renderPreview/renderPreviewAsDataUri with ShapeSphere/zero yaw), and ensure both results are non-empty before comparing; this locks down that renderInteractivePreview does not contaminate the cached preview state.qml/PassPropertiesPanel.qml (3)
6-46: 💤 Low valueLocal theme color properties duplicated across every panel.
PassPropertiesPanel,TexturePropertiesPanel, andMaterialEditorWindoweach re-declare the same eightreadonly property colorpass-throughs toMaterialEditorQML.*. Consider exposing a tinyThemePalette.qmlsingleton (or a single inline component) so the wiring lives in one place and the panels just referenceTheme.borderColor/Theme.textColoretc. This isn't blocking, but with the same block duplicated three times in this slice it's already drifting (e.g. some innerGroupBoxes bind directly toMaterialEditorQML.borderColorat lines 69, 203, 406, … while others rely on the root's local alias — see line 22).🤖 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/PassPropertiesPanel.qml` around lines 6 - 46, Extract the duplicated readonly property color pass-throughs into a single shared theme object (e.g. create a ThemePalette.qml singleton or an inline Component named ThemePalette exposing borderColor, backgroundColor, panelColor, textColor, highlightColor, buttonColor, buttonTextColor, disabledTextColor), then remove the eight readonly property color lines from PassPropertiesPanel and the similar blocks in TexturePropertiesPanel and MaterialEditorWindow and update their bindings to reference ThemePalette.borderColor / ThemePalette.textColor / etc. Ensure any places that currently bind directly to MaterialEditorQML.* are reconciled to use the new ThemePalette API (or keep MaterialEditorQML delegating to ThemePalette) so all panels consistently reference the same theme singleton.
792-834: ⚡ Quick winInline
indicator+contentItemoverrides cancel theThemedCheckBoxmigration.
alphaRejectionEnabledCheckwas migrated fromCheckBoxtoThemedCheckBox, but the entireindicator:Rectangle +contentItem:Text block from the old definition was left in place. The same pattern is duplicated at:
alphaToCoverageCheck(lines 892-934)colourWriteRedCheck/Green/Blue/Alpha(lines 970-1144)pointSpritesCheck(lines 1251-1293)fogOverrideCheck(lines 1408-1450)This is ~360 lines of copy-pasted styling that the new themed primitive should encapsulate, and any future styling tweak to
ThemedCheckBoxwill silently not apply to these eight instances. Worth a follow-up pass to strip the overrides and letThemedCheckBoxprovide indicator/contentItem (matching the simpler migrations earlier in the same file).♻️ Example cleanup for `alphaRejectionEnabledCheck`
ThemedCheckBox { id: alphaRejectionEnabledCheck text: "Enable Alpha Rejection" checked: MaterialEditorQML.alphaRejectionEnabled onCheckedChanged: { if (checked !== MaterialEditorQML.alphaRejectionEnabled) { MaterialEditorQML.alphaRejectionEnabled = checked } } Connections { target: MaterialEditorQML function onAlphaRejectionEnabledChanged() { alphaRejectionEnabledCheck.checked = MaterialEditorQML.alphaRejectionEnabled } } - indicator: Rectangle { - implicitWidth: 16 - ... - } - contentItem: Text { - text: alphaRejectionEnabledCheck.text - ... - } }🤖 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/PassPropertiesPanel.qml` around lines 792 - 834, ThemedCheckBox instances still include inline indicator: and contentItem: overrides copied from the old CheckBox (e.g., alphaRejectionEnabledCheck, alphaToCoverageCheck, colourWriteRedCheck/Green/Blue/Alpha, pointSpritesCheck, fogOverrideCheck), which prevents shared styling in ThemedCheckBox from applying; remove the custom indicator: Rectangle and contentItem: Text blocks from each of these named controls so they rely on ThemedCheckBox's built-in indicator/contentItem, and if any custom behavior is required migrate only the minimal differences into ThemedCheckBox or expose a small property (e.g., accent/spacing) rather than duplicating full markup.
88-146: ⚡ Quick winPartial
ThemedCheckBoxmigration leaves redundantcontentItemoverrides.These
ThemedCheckBoxinstances inline acontentItem: Text { ... }block that re-implements (likely identical) text styling. IfThemedCheckBoxis meant to centralize the themed visuals (as the AI summary states), the overrides here defeat the migration: the new primitive's contentItem is replaced by ad-hoc copies that all repeat the samecolor: textColor/leftPadding: parent.indicator.width + parent.spacingboilerplate. The same pattern repeats below at lines 241-259, 283-301, 325-343, and 367-385.Drop the per-instance
contentItemoverride and letThemedCheckBoxprovide it. If a particular instance needs a tweak, add it as a property onThemedCheckBoxinstead of overriding here.♻️ Suggested simplification (apply to all "Use Vertex Color" + Lighting/Depth toggles)
- ThemedCheckBox { - - text: "Lighting" - - contentItem: Text { - - text: parent.text - - color: textColor - - leftPadding: parent.indicator.width + parent.spacing - - verticalAlignment: Text.AlignVCenter - - } - - checked: MaterialEditorQML.lightingEnabled - onCheckedChanged: MaterialEditorQML.setLightingEnabled(checked) - } + ThemedCheckBox { + text: "Lighting" + checked: MaterialEditorQML.lightingEnabled + onCheckedChanged: MaterialEditorQML.setLightingEnabled(checked) + }🤖 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/PassPropertiesPanel.qml` around lines 88 - 146, Several ThemedCheckBox instances (e.g., the ones bound to MaterialEditorQML.lightingEnabled, depthWriteEnabled, depthCheckEnabled) unnecessarily override contentItem with duplicated Text blocks; remove these per-instance contentItem: Text { ... } overrides and rely on ThemedCheckBox's built-in contentItem styling instead, and if a specific instance needs a tweak expose a property on ThemedCheckBox (or pass a simple text/leftPadding property) rather than reimplementing the contentItem; apply this change to the other repeated occurrences (lines referenced in the review: the "Use Vertex Color" + Lighting/Depth toggles blocks) so each checkbox only sets text, checked and onCheckedChanged.qml/MaterialEditorWindow.qml (2)
532-555: 💤 Low valueStrip
console.logdebug breadcrumbs before merging.
Component.onCompletedand severalMouseArea.onClickedhandlers (e.g., lines 235, 277, 318, 361, 500, 1500) log to the console. These are useful during development but become noise in release logs and slow down the inspector when the editor is opened repeatedly. Consider gating them behind a debug flag or removing the ones that no longer serve a purpose.🤖 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/MaterialEditorWindow.qml` around lines 532 - 555, Remove or gate development console logging in Component.onCompleted and in the various MouseArea.onClicked handlers so debug breadcrumbs don't ship in release builds; specifically, eliminate or wrap console.log/console.error calls that reference MaterialEditorQML (seen in Component.onCompleted) and the MouseArea.onClicked handlers so they either check a central debug flag (e.g., DEBUG or MaterialEditorDebug) before logging or are removed entirely, preserving functional calls like MaterialEditorQML.createNewMaterial(...) and the isLoading state changes.
1289-1482: ⚖️ Poor tradeoffHidden GroupBoxes kept as binding shims will accumulate technical debt.
The "Techniques", "Passes", and "Texture Units"
GroupBoxes (lines 1298-1350, 1354-1409, 1420-1474) are nowvisible: falsebut still construct themselves, instantiateThemedComboBox+ThemedButton, and wire change handlers (e.g.,techniqueCombo.onCurrentIndexChanged: MaterialEditorQML.setSelectedTechniqueIndex(currentIndex)). They also lack the sentinel-aware behavior of the new sticky context strip, so if anything ever causes them to be re-shown they will silently fight the context-strip combos for the same backend selection.The comments mention they're kept "for binding stability" / "existing tests / external tools" — once the slice is merged, please plan a follow-up to either remove them outright or extract the binding shim into a non-visual
Itemso deadGroupBoxchrome doesn't keep being rendered/restyled in every subsequent theming pass.🤖 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/MaterialEditorWindow.qml` around lines 1289 - 1482, Hidden GroupBoxes (Techniques, Passes, Texture Units) are still instantiating UI chrome and controls; replace each GroupBox with a non-visual binding shim Item (e.g., TechniqueBindingShim, PassBindingShim, TextureUnitBindingShim) that contains only the ThemedComboBox/ThemedButton IDs (techniqueCombo, passCombo, textureUnit combo IDs) and their onCurrentIndexChanged handlers (MaterialEditorQML.setSelectedTechniqueIndex/setSelectedPassIndex/setSelectedTextureUnitIndex) and newXDialog references, so bindings remain but no GroupBox/Rectangle/label styling is created; keep Layout.fillWidth/disabled logic if needed but ensure the shim is non-visual (no background/label) to avoid extra styling passes and potential conflicts with the sticky context strip.src/PropertiesPanelController_test.cpp (1)
991-1070: 💤 Low valueGood coverage for the new
applyMaterialToSelectionbehavior.The four new tests cleanly exercise: empty-name no-op, no-selection return value, sub-entity precedence over parent entity, full-entity fan-out across sub-entities, and the
selectionChangedemission contract. The Xvfb gate viacanLoadMeshFiles()is correctly placed before every test that creates an entity, matching the existing pattern in this fixture.A couple of tiny opportunities (not blocking):
createStandardOgreMaterials()is already called once inSetUp()(line 42); the re-invocations at lines 1015, 1037, and 1057 are redundant — fine for idempotency but easy to drop.sub0->getMaterial()->getName()andentity->getSubEntity(0)->getMaterial()->getName()will crash if the material pointer is ever null. Today the priorEXPECT_EQ(..., 1)makes that unreachable, but anASSERT_TRUE(sub0->getMaterial())(orASSERT_FALSE(...->isNull())forMaterialPtr) before dereferencing would give a clean test failure instead of a segfault ifapplyMaterialToSelectionregresses in the future.🤖 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_test.cpp` around lines 991 - 1070, Remove the redundant createStandardOgreMaterials() calls in the tests ApplyMaterialToSelection_SubEntityTakesPrecedence, ApplyMaterialToSelection_EntitySelectionTouchesAllSubmeshes and ApplyMaterialToSelection_EmitsSelectionChangedSignal (it's already called in SetUp via createStandardOgreMaterials()); and add assertions to guard dereferencing of material pointers before calling getName(), e.g. in ApplyMaterialToSelection_SubEntityTakesPrecedence assert that sub0->getMaterial() is non-null (ASSERT_TRUE(sub0->getMaterial()) or ASSERT_FALSE(sub0->getMaterial().isNull())) and similarly assert entity->getSubEntity(0)->getMaterial() is non-null in tests that inspect that material to avoid segfaults if applyMaterialToSelection regresses.qml/PropertiesPanel.qml (1)
1819-1930: ⚡ Quick winRemove
ScrollViewwrapping and attachScrollBar.verticaldirectly to theGridView.
GridViewis aFlickable, and wrapping it inScrollViewcreates a nested-Flickable issue that can cause gesture handling conflicts and scrollbar synchronization problems. The Qt 6 documentation and the established pattern used elsewhere in this codebase (e.g.,AISettingsDialog.qml,AssetBrowser.qml) show thatScrollBar.verticalshould be attached directly to the grid instead.♻️ Suggested restructure
- ScrollView { - anchors.fill: parent - anchors.margins: 1 - clip: true - visible: materialToolCol.gridReady - - GridView { - id: matGrid - cellWidth: Math.max(90, - (width - 4) / Math.max(1, Math.floor((width - 4) / 90))) - cellHeight: 100 - model: materialToolCol.materialNames - delegate: Item { - ... - } - } - } + GridView { + id: matGrid + anchors.fill: parent + anchors.margins: 1 + clip: true + visible: materialToolCol.gridReady + cellWidth: Math.max(90, + (width - 4) / Math.max(1, Math.floor((width - 4) / 90))) + cellHeight: 100 + model: materialToolCol.materialNames + ScrollBar.vertical: ScrollBar {} + delegate: Item { + ... + } + }🤖 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 1819 - 1930, The ScrollView wrapper creates a nested Flickable problem; remove the ScrollView and make the GridView (id: matGrid) the direct child of the Rectangle, set its anchors.fill (and anchors.margins: 1) and clip: true, keep its visible bound to materialToolCol.gridReady, and then attach a ScrollBar.vertical to matGrid (using ScrollBar.vertical: ScrollBar { } or ScrollBar { id: matGridVScroll; orientation: Qt.Vertical; /* styling */ } declared as the GridView's ScrollBar.vertical) so the flicking and scrollbar are handled by the GridView itself; ensure the loading Text remains visible when !materialToolCol.gridReady and MouseArea/delegate behavior is unchanged.
🤖 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 1875-1902: The per-card grid Image is missing cache: false which
lets QML serve stale bitmaps from
MaterialPreviewRenderer::renderPreviewAsDataUri(materialName); update the Image
block inside PropertiesPanel.qml (the grid card thumbnail Image that uses
MaterialEditorQML.materialPreview(modelData)) to set cache: false so it always
reloads previews after material edits; ensure this mirrors the existing
previewImage behavior and leaves MaterialEditorQML.materialPreview and
refreshMaterialList() unchanged.
In `@src/BoneDragRelease_test.cpp`:
- Around line 387-391: The test uses std::array for the variable named targets
(const std::array<Ogre::Vector3, 3> targets = {...}) but the file lacks a direct
`#include` <array>, relying on transitive includes; add a direct include for
<array> near the other standard headers at the top of the file so std::array is
always defined and the targets declaration compiles reliably.
In `@src/MaterialPreviewRenderer.cpp`:
- Around line 304-342: The shared preview scene state (entity, node orientation,
light direction, m_interactiveCurrentShape) can be left mutated by
renderInteractivePreview() which causes renderPreview()/renderPreviewAsDataUri()
to cache wrong thumbnails; fix by resetting the preview to the documented
default before generating cached thumbnails: in renderPreview() and
renderPreviewAsDataUri() (or a small helper called e.g. resetPreviewToDefaults)
ensure the mesh is the Sphere (use ensureShapeMesh(ShapeSphere) and
create/attach m_sphere if null or different), set
m_sphereNode->setOrientation(Ogre::Quaternion::IDENTITY), set
m_interactiveCurrentShape = ShapeSphere, and set m_lightNode direction to the
default baseDir (-1,-1,-1).normalisedCopy() (i.e. yaw 0) so thumbnails are
generated from the canonical Sphere + yaw 0 state.
In `@src/PropertiesPanelController.cpp`:
- Around line 262-297: applyMaterialToSelection currently mutates
Ogre::SubEntity material names directly; capture previous material names and
make the change undoable by creating and pushing an undo command/macro that
stores a list of affected sub-entities and their old material names and restores
them on undo. Specifically, before changing any sub-entity in
applyMaterialToSelection (both the per-sub-entity path using
SelectionSet::getSubEntitiesSelectionList and the entity/subentity loop using
SelectionSet::getResolvedEntities and ent->getSubEntity(i)), record pairs of
(Ogre::SubEntity*, oldMaterialName), then create/instantiate an UndoCommand (or
macro) whose redo sets the new materialName (stdName) on each recorded SubEntity
and whose undo sets the saved oldMaterialName back; push that command to the
app’s undo stack instead of performing direct writes, and ensure you still emit
selectionChanged() after the command executes if touched > 0.
---
Outside diff comments:
In `@qml/MaterialEditorWindow.qml`:
- Around line 1064-1072: The current guard in the text property replaces any
materialText containing the substrings "import", "Item", or "Rectangle", causing
false positives; update the check in the text block that reads
MaterialEditorQML.materialText to use anchored/multiline regexes instead of
indexOf: test for QML imports only at the start of a line (e.g.
/^\s*import\s+\w/m) and for QML root types only when they appear as a standalone
token at the start of a line (e.g. /^\s*(Item|Rectangle)\b/m); keep returning
the hard-coded default only if one of these anchored regex tests matches or the
trimmed materialText is empty.
---
Nitpick comments:
In `@qml/MaterialEditorWindow.qml`:
- Around line 532-555: Remove or gate development console logging in
Component.onCompleted and in the various MouseArea.onClicked handlers so debug
breadcrumbs don't ship in release builds; specifically, eliminate or wrap
console.log/console.error calls that reference MaterialEditorQML (seen in
Component.onCompleted) and the MouseArea.onClicked handlers so they either check
a central debug flag (e.g., DEBUG or MaterialEditorDebug) before logging or are
removed entirely, preserving functional calls like
MaterialEditorQML.createNewMaterial(...) and the isLoading state changes.
- Around line 1289-1482: Hidden GroupBoxes (Techniques, Passes, Texture Units)
are still instantiating UI chrome and controls; replace each GroupBox with a
non-visual binding shim Item (e.g., TechniqueBindingShim, PassBindingShim,
TextureUnitBindingShim) that contains only the ThemedComboBox/ThemedButton IDs
(techniqueCombo, passCombo, textureUnit combo IDs) and their
onCurrentIndexChanged handlers
(MaterialEditorQML.setSelectedTechniqueIndex/setSelectedPassIndex/setSelectedTextureUnitIndex)
and newXDialog references, so bindings remain but no GroupBox/Rectangle/label
styling is created; keep Layout.fillWidth/disabled logic if needed but ensure
the shim is non-visual (no background/label) to avoid extra styling passes and
potential conflicts with the sticky context strip.
In `@qml/PassPropertiesPanel.qml`:
- Around line 6-46: Extract the duplicated readonly property color pass-throughs
into a single shared theme object (e.g. create a ThemePalette.qml singleton or
an inline Component named ThemePalette exposing borderColor, backgroundColor,
panelColor, textColor, highlightColor, buttonColor, buttonTextColor,
disabledTextColor), then remove the eight readonly property color lines from
PassPropertiesPanel and the similar blocks in TexturePropertiesPanel and
MaterialEditorWindow and update their bindings to reference
ThemePalette.borderColor / ThemePalette.textColor / etc. Ensure any places that
currently bind directly to MaterialEditorQML.* are reconciled to use the new
ThemePalette API (or keep MaterialEditorQML delegating to ThemePalette) so all
panels consistently reference the same theme singleton.
- Around line 792-834: ThemedCheckBox instances still include inline indicator:
and contentItem: overrides copied from the old CheckBox (e.g.,
alphaRejectionEnabledCheck, alphaToCoverageCheck,
colourWriteRedCheck/Green/Blue/Alpha, pointSpritesCheck, fogOverrideCheck),
which prevents shared styling in ThemedCheckBox from applying; remove the custom
indicator: Rectangle and contentItem: Text blocks from each of these named
controls so they rely on ThemedCheckBox's built-in indicator/contentItem, and if
any custom behavior is required migrate only the minimal differences into
ThemedCheckBox or expose a small property (e.g., accent/spacing) rather than
duplicating full markup.
- Around line 88-146: Several ThemedCheckBox instances (e.g., the ones bound to
MaterialEditorQML.lightingEnabled, depthWriteEnabled, depthCheckEnabled)
unnecessarily override contentItem with duplicated Text blocks; remove these
per-instance contentItem: Text { ... } overrides and rely on ThemedCheckBox's
built-in contentItem styling instead, and if a specific instance needs a tweak
expose a property on ThemedCheckBox (or pass a simple text/leftPadding property)
rather than reimplementing the contentItem; apply this change to the other
repeated occurrences (lines referenced in the review: the "Use Vertex Color" +
Lighting/Depth toggles blocks) so each checkbox only sets text, checked and
onCheckedChanged.
In `@qml/PropertiesPanel.qml`:
- Around line 1819-1930: The ScrollView wrapper creates a nested Flickable
problem; remove the ScrollView and make the GridView (id: matGrid) the direct
child of the Rectangle, set its anchors.fill (and anchors.margins: 1) and clip:
true, keep its visible bound to materialToolCol.gridReady, and then attach a
ScrollBar.vertical to matGrid (using ScrollBar.vertical: ScrollBar { } or
ScrollBar { id: matGridVScroll; orientation: Qt.Vertical; /* styling */ }
declared as the GridView's ScrollBar.vertical) so the flicking and scrollbar are
handled by the GridView itself; ensure the loading Text remains visible when
!materialToolCol.gridReady and MouseArea/delegate behavior is unchanged.
In `@src/MaterialPreviewRenderer_test.cpp`:
- Around line 261-426: The test suite misses a regression case where state from
renderInteractivePreview leaks into the cached thumbnail path; add a test in
MaterialPreviewRendererTests that first calls
MaterialPreviewRenderer::renderInteractivePreview with a non-sphere shape (e.g.,
ShapeCube or ShapePlane) and/or non-zero yaw, then calls the thumbnail path
(MaterialPreviewRenderer::renderPreview or renderPreviewAsDataUri) for the same
material and asserts the thumbnail equals the canonical sphere/default-light
thumbnail (obtained by calling renderPreview/renderPreviewAsDataUri with
ShapeSphere/zero yaw), and ensure both results are non-empty before comparing;
this locks down that renderInteractivePreview does not contaminate the cached
preview state.
In `@src/PropertiesPanelController_test.cpp`:
- Around line 991-1070: Remove the redundant createStandardOgreMaterials() calls
in the tests ApplyMaterialToSelection_SubEntityTakesPrecedence,
ApplyMaterialToSelection_EntitySelectionTouchesAllSubmeshes and
ApplyMaterialToSelection_EmitsSelectionChangedSignal (it's already called in
SetUp via createStandardOgreMaterials()); and add assertions to guard
dereferencing of material pointers before calling getName(), e.g. in
ApplyMaterialToSelection_SubEntityTakesPrecedence assert that
sub0->getMaterial() is non-null (ASSERT_TRUE(sub0->getMaterial()) or
ASSERT_FALSE(sub0->getMaterial().isNull())) and similarly assert
entity->getSubEntity(0)->getMaterial() is non-null in tests that inspect that
material to avoid segfaults if applyMaterialToSelection regresses.
🪄 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: cafc3de0-ee77-4057-adba-d6b171c8c93f
📒 Files selected for processing (21)
qml/MaterialEditorWindow.qmlqml/PassPropertiesPanel.qmlqml/PropertiesPanel.qmlqml/TexturePropertiesPanel.qmlqml/ThemedButton.qmlqml/ThemedCheckBox.qmlqml/ThemedLabel.qmlqml/ThemedSpinBox.qmlqml/ThemedTextField.qmlqml/qmldirsrc/BoneDragRelease_test.cppsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/MaterialEditorQML_test.cppsrc/MaterialPreviewRenderer.cppsrc/MaterialPreviewRenderer.hsrc/MaterialPreviewRenderer_test.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/PropertiesPanelController_test.cppsrc/qml_resources.qrc
- src/BoneDragRelease_test.cpp: add direct #include <array> for std::array
(was previously relying on transitive includes); rewrite the stress test
so it re-applies parent rotation per iteration (_updateAnimation runs
Skeleton::reset which wipes manual orientations on other bones) and
drops the brittle derived-position equality check — local-equals-baseline
is what guards the actual teleport bug.
- src/MaterialPreviewRenderer.{h,cpp}: add resetToCanonicalThumbnailState()
called at the top of renderPreview() so the cached thumbnail path
always renders the canonical Sphere + default-light pose regardless of
what shape/yaw the interactive preview just rendered. Also make
ensureScene() defensive against Ogre::Root being destroyed under us
(Manager::kill() in test fixtures): clear cached pointers and re-init.
Adds a regression test ThumbnailIsCanonicalAfterInteractivePreview.
- qml/PropertiesPanel.qml: set `cache: false` on the material library
grid card thumbnails so QML doesn't serve stale bitmaps after a
material edit (mirrors the existing previewImage behaviour).
- src/commands/ApplyMaterialCommand.{h,cpp}: new QUndoCommand that
records (sub-entity, oldMaterialName) pairs and restores them on
undo. PropertiesPanelController::applyMaterialToSelection now
captures pre-apply bindings and pushes the command on the undo stack
instead of mutating sub-entities directly. New test
ApplyMaterialToSelection_IsUndoable exercises undo/redo round-trip.
- src/CMakeLists.txt, tests/CMakeLists.txt: register the new command
files in the QtMeshEditor and UnitTests source lists.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-material-preview-env
…OgreTest The Linux Xvfb runner SIGSEGVs between InteractivePreview_UnknownMaterialReturnsEmpty and InteractivePreview_KnownMaterialReturnsDataUri because MaterialEditorQMLWithOgreTest's SetUp calls Manager::kill() to tear down Ogre::Root, but the global MaterialPreviewRenderer singleton isn't reset and ends up pointing at a destroyed SceneManager. The ensureScene() defensive reset is in place, but Ogre's internal state across Root recreation still trips a deeper crash. The wrapper itself is a one-line forward to MaterialPreviewRenderer::renderInteractivePreview, which has full coverage in MaterialPreviewRenderer_test.cpp (whose TearDown calls MaterialPreviewRenderer::kill() so each test starts fresh). Move the comment so future readers know why these cases live there only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
ThemedCheckBoxQML primitive and flat-header GroupBox restyle across Pass/Texture property panels.MaterialPreviewRenderer::renderInteractivePreview(name, size, shape, yaw)withShapeQ_ENUM (Sphere/Cube/Plane), env-light yaw, and a separate resizable RTT (thumbnail cache untouched).MaterialEditorQML::inputColor/headerColor(Inspector palette parity) +interactiveMaterialPreview(...)wrapper.PropertiesPanelController::applyMaterialToSelection(name)— touches selected sub-entities (or every sub-entity of selected entities), emitsselectionChangedso the scene-tree material column refreshes.Test plan
BoneDragReleaseTest: 3 new tests driving multi-event setUpdate sequences (5/drag, 20-event × 3-drag stress, zero-deltamanualControlledclear).MaterialPreviewRendererTests: 7 new tests — size clamping (32 floor / 1024 cap), yaw wrap-around (0 ≡ 360, ≠ −90), shape switch produces distinct bytes, out-of-range shape falls back to Sphere, RTT reuse on same size, RTT resize between calls.MaterialEditorQMLTest: 3 new theme-color tests (panelColor/inputColor/headerColormatch the expected palette roles) + 3interactiveMaterialPreviewtests (unknown material, known data URL, shape switch).PropertiesPanelControllerTests: 5 newapplyMaterialToSelectiontests — empty name is a no-op, no selection returns 0, sub-entity selection takes precedence over entity selection, entity selection touches every submesh,selectionChangedis emitted on success.tryInitOgre()blocks on missing GL plugins per the documented project constraint).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style
Tests