Skip to content

Fix SD crash on repeated generation, improve material pipeline UX#201

Merged
fernandotonon merged 1 commit intomasterfrom
fix/sd-second-generation-crash
Mar 20, 2026
Merged

Fix SD crash on repeated generation, improve material pipeline UX#201
fernandotonon merged 1 commit intomasterfrom
fix/sd-second-generation-crash

Conversation

@fernandotonon
Copy link
Owner

@fernandotonon fernandotonon commented Mar 20, 2026

Summary

  • Fix SD crash on second generationgenerate_image() corrupts sd.cpp internal state on macOS Metal after first call. Fix: recreate sd_ctx before each generation.
  • Remove img2img — crashes on Metal regardless of workarounds. Edit panel removed.
  • Deferred material apply — material is only applied after SD finishes generating the texture, avoiding broken intermediate state with missing textures.
  • Unified progress bar — single bar shows LLM progress ("LLM: X%") then SD progress ("Texture: X%") seamlessly.
  • Startup resource registrationgenerated_textures/ dir registered as Ogre resource at startup so textures from previous sessions load correctly.
  • LLM prompt update — now encourages texture_unit with descriptive filenames for realistic materials (was "Do NOT add texture_unit").
  • Dark theme fixes — themed labels/backgrounds on all GroupBoxes and CheckBoxes in Pass and Texture properties panels. Fixed placeholder text color.
  • Template update — PBR → Normal Map template. Updated prompt placeholder.
  • Preview cache-busting — counter appended to image URL forces QML to reload when texture is regenerated with same filename.

Test plan

  • SD tests pass (11 SDWorker + 14 SDManager + 8 LLMSettingsWidget = 33 tests)
  • Builds with SD OFF (CI scenario)
  • Generate texture, then generate again — no crash
  • LLM generates material with texture → waits for SD → applies material with texture
  • Previously generated textures load on app restart
  • Dark theme: all labels readable in Pass/Texture properties
  • CI: Linux/macOS/Windows builds pass

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Automatic texture generation: when AI output references missing textures, the app now generates them and completes the material once ready.
    • Enhanced status/progress: UI shows "Generating texture..." and switches progress labeling between LLM and Texture.
  • Bug Fixes

    • Fixed stale texture previews via cache-busting reloads.
  • UI/UX Improvements

    • Themed, consistent panel and control styling.
    • Updated material prompt placeholder and Normal Map material template.
  • Removals

    • Image-to-image (texture edit) workflow removed.

@coderabbitai
Copy link

coderabbitai bot commented Mar 20, 2026

📝 Walkthrough

Walkthrough

Deferred Stable Diffusion texture generation was integrated into the LLM-driven material workflow: missing textures detected in LLM output now trigger SD generation and delay final material completion. Img2img editing APIs and UI were removed; QML styling and preview cache-busting were added.

Changes

Cohort / File(s) Summary
Material Editor Core
src/MaterialEditorQML.h, src/MaterialEditorQML.cpp
Added sdPendingForMaterial property and pending-script state; defer aiGenerationCompleted until SD finishes/fails; register generated_textures resource group at startup; removed editTextureFromPrompt(); added onSDGenerationStopped() handler.
Stable Diffusion Backend
src/SDManager.h, src/SDManager.cpp, src/SDWorker.h, src/SDWorker.cpp
Removed img2img/editTexture APIs and implementations; changed prompt suffix to normal map compatible; added SDWorker::recreateContext() to reinit model context before generation; updated error messaging.
LLM Prompt Rules
src/LLMManager.cpp
Adjusted system prompt rules to conditionally add texture_unit for realistic materials when textures are listed, removing prior blanket suppression.
Material Editor QML UI
qml/MaterialEditorWindow.qml
Added handler for sdPendingForMaterial to show "Generating texture..." and updated progress row to reflect combined LLM+SD flow; changed Templates snippet to NormalMapMaterial; updated AI prompt placeholder text.
Texture Properties UI
qml/TexturePropertiesPanel.qml
Removed "AI Texture Edit (img2img)" GroupBox; added cache-busting preview (cacheBuster, reloadPreview()), call reload on texture/name/SD events; themed GroupBox backgrounds and ThemedLabel usage.
Pass Properties UI
qml/PassPropertiesPanel.qml
Replaced GroupBox header labels with themed background and ThemedLabel; customized CheckBox contentItem for correct padding/alignment and theme colors.
Theme Placeholder
qml/ThemedTextField.qml
Switched placeholderTextColor to MaterialEditorQML.disabledTextColor for theme-consistent placeholder rendering.

Sequence Diagram

sequenceDiagram
    participant User
    participant QML as MaterialEditor (QML)
    participant LLM as LLMManager
    participant Core as MaterialEditorQML
    participant SD as SDManager
    participant Worker as SDWorker
    participant Ogre as Ogre Engine

    User->>QML: Request material generation
    QML->>LLM: Send generation request
    LLM->>Core: onLLMGenerationCompleted(script)
    Core->>Core: Detect missing texture refs (regex)
    alt Missing textures detected
        Core->>Core: set m_sdPendingForMaterial = true
        Core->>QML: sdPendingForMaterialChanged()
        QML->>QML: Show "Generating texture..." + progress
        Core->>SD: generateTexture(textureName, prompt)
        SD->>Worker: generateTexture()
        Worker->>Worker: recreateContext()
        Worker->>Ogre: Save generated texture file
        Worker->>SD: generationCompleted(filePath)
        SD->>Core: onSDGenerationCompleted(filePath)
        Core->>Ogre: Apply texture to material
        Core->>Core: clear m_sdPendingForMaterial
        Core->>QML: aiGenerationCompleted(finalScript)
    else No missing textures
        Core->>QML: aiGenerationCompleted(script)
    end
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly Related PRs

Poem

🐰
I nibbled code in moonlit rows,
Found missing maps where nobody knows,
LLM asks, SD hums and grows,
Previews refresh in tidy rows,
Img2img hopped off — new workflow glows.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main bug fix (SD crash on repeated generation) and primary UX improvement (material pipeline), accurately reflecting the core changes in the changeset.
Description check ✅ Passed The description comprehensively covers all major changes including bug fixes, features, and UI improvements, with a detailed test plan, though CI build verification is still pending.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sd-second-generation-crash
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bccc7afbc5

ℹ️ 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".

Comment on lines 2947 to 2951
if (!isOgreAvailable()) {
// Update texture name for preview even without Ogre
m_textureName = fileName;
emit textureNameChanged();
emit sdTextureGenerated(outputPath);
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't skip deferred AI completion in the no-Ogre return

When onLLMGenerationCompleted() auto-starts SD, the new m_sdPendingForMaterial flow is only cleared at the bottom of onSDGenerationCompleted(). This early return exits before that cleanup, so in any run where isOgreAvailable() is false — or Ogre::Root::getSingletonPtr() hits the similar return a few lines later — aiGenerationCompleted() never fires and sdPendingForMaterial never resets. The result is a stuck "Generating texture..." state and a material that is never applied in non-Ogre/headless setups.

Useful? React with 👍 / 👎.

Comment on lines +2822 to 2825
m_pendingMaterialScript = cleanedText;
m_sdPendingForMaterial = true;
emit sdPendingForMaterialChanged();
sdManager->generateTexture(cleanPrompt, 0, 0, texName);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle canceled SD runs for deferred material generation

This new pending path assumes the auto-triggered SD request will end in onSDGenerationCompleted() or onSDGenerationError(), but MaterialEditorQML still has no generationStopped handler even though the texture panel can call stopTextureGeneration() while sdIsGenerating is true. If a user cancels an LLM-triggered texture run, m_sdPendingForMaterial remains true forever and the deferred aiGenerationCompleted() never happens, leaving the editor blocked on the texture step and dropping the generated script.

Useful? React with 👍 / 👎.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/SDWorker.cpp (1)

197-212: ⚠️ Potential issue | 🟠 Major

Honor stop requests before entering generate_image().

m_stopRequested is reset before recreateContext(), but it is not checked again until after generate_image() returns. A stop fired while the context is being rebuilt still launches a full generation, and the UI does not enter the generating state until that rebuild is done. Bail out immediately after recreateContext() if stop was requested before calling generate_image().

Suggested guard
     // sd.cpp crashes on second generate_image() call with the same context.
     // Recreate the context before each generation to ensure clean state.
     recreateContext();
+    if (m_stopRequested.load()) {
+        m_isGenerating.store(false);
+        emit generationStopped();
+        return;
+    }
 
     if (!m_ctx) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/SDWorker.cpp` around lines 197 - 212, After calling recreateContext()
inside the generation routine, check m_stopRequested.load() and bail out
immediately if true: set m_isGenerating.store(false) and return before emitting
generationStarted() or calling generate_image(); optionally emit a suitable stop
signal (e.g. generationStopped()) if your codebase expects it. This ensures a
stop requested while rebuilding the SD context prevents starting a full
generation; locate the check near recreateContext(), m_ctx handling, and before
the call to generate_image().
src/MaterialEditorQML.cpp (3)

3002-3016: ⚠️ Potential issue | 🟠 Major

Don't emit the normal AI completion signal after SD fails.

qml/MaterialEditorWindow.qml treats aiGenerationCompleted as the happy path: it validates, applies, and sets a success status. Emitting it unconditionally after sdGenerationError turns a texture-generation failure into a false positive in the main workflow.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/MaterialEditorQML.cpp` around lines 3002 - 3016, In
onSDGenerationError(...) remove the unconditional emit of
aiGenerationCompleted(m_pendingMaterialScript) so a failed SD does not signal
the happy-path completion; instead just clear the pending state: set
m_sdPendingForMaterial = false, emit sdPendingForMaterialChanged(), and clear
m_pendingMaterialScript without emitting aiGenerationCompleted from
MaterialEditorQML::onSDGenerationError; keep the existing emits
sdGenerationError(...) and sdIsGeneratingChanged() so the UI can handle the
failure path.

2971-2991: ⚠️ Potential issue | 🔴 Critical

Don't overwrite m_materialText in the deferred SD path.

When m_sdPendingForMaterial is true, this edits the currently loaded Ogre material and updateMaterialText() serializes that material back into m_materialText. The QML completion handler then calls applyMaterial() without pushing m_pendingMaterialScript back first, so the old material can be applied instead of the LLM-generated script.

💡 Suggested fix
-        // Apply texture to current material pass
-        Ogre::Pass *pass = getCurrentPass();
-        if (pass) {
-            Ogre::TextureUnitState *texUnit = getCurrentTextureUnit();
-            if (!texUnit) {
-                texUnit = pass->createTextureUnitState();
-                updateTextureUnitList();
-                if (m_textureUnitList.size() > 0) {
-                    setSelectedTextureUnitIndex(m_textureUnitList.size() - 1);
-                }
-            }
-            if (texUnit) {
-                texUnit->setTextureName(fileName.toStdString());
-            }
-        }
+        if (!m_sdPendingForMaterial) {
+            // Manual SD generation updates the currently edited material immediately.
+            Ogre::Pass *pass = getCurrentPass();
+            if (pass) {
+                Ogre::TextureUnitState *texUnit = getCurrentTextureUnit();
+                if (!texUnit) {
+                    texUnit = pass->createTextureUnitState();
+                    updateTextureUnitList();
+                    if (!m_textureUnitList.isEmpty()) {
+                        setSelectedTextureUnitIndex(m_textureUnitList.size() - 1);
+                    }
+                }
+                if (texUnit) {
+                    texUnit->setTextureName(fileName.toStdString());
+                }
+            }
+        }
@@
-    updateMaterialText();
+    if (!m_sdPendingForMaterial) {
+        updateMaterialText();
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/MaterialEditorQML.cpp` around lines 2971 - 2991, The current flow writes
the edited Ogre material back into m_materialText via updateMaterialText() even
when m_sdPendingForMaterial is set, which can overwrite the pending
LLM-generated script; modify the code in the texture-apply/completion path to
guard against this by skipping the updateMaterialText() and any assignment to
m_materialText when m_sdPendingForMaterial is true (or, alternatively, restore
m_pendingMaterialScript into m_materialText immediately before
updateMaterialText() if you prefer to serialize the pending script); reference
m_sdPendingForMaterial, m_materialText, m_pendingMaterialScript,
updateMaterialText(), and applyMaterial() to locate and fix the logic so the
deferred SD material is not overwritten.

2793-2827: ⚠️ Potential issue | 🟠 Major

Generate every missing texture before resuming the deferred apply.

This loop stops after the first unresolved texture reference. With the new normal-map flow, it's easy to produce scripts that reference both diffuse and normal textures, so the later unresolved references still reach aiGenerationCompleted() as broken material inputs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/MaterialEditorQML.cpp` around lines 2793 - 2827, The loop in
MaterialEditorQML.cpp currently stops after the first missing texture
(breaking), which leaves other referenced textures unresolved; modify the logic
around sdManager->generateTexture so it collects all missing texture names found
in cleanedText (while iterating QRegularExpression matches) and triggers
generation for each (calling sdManager->generateTexture(cleanPrompt, 0, 0,
texName) per missing texture) instead of breaking after the first; keep setting
m_pendingMaterialScript = cleanedText and m_sdPendingForMaterial = true (and
emit sdPendingForMaterialChanged()) once if at least one texture was scheduled,
set sdTriggered = true when any are scheduled, and remove the break so all
missing textures are enqueued for generation.
🤖 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/LLMManager.cpp`:
- Around line 242-243: The instruction that tells the model to invent texture
filenames must be made runtime-aware: in LLMManager.cpp where the material
prompt is composed, wrap the texture-generation sentence so it is only included
when sdManager->isModelLoaded() (or under an ENABLE_SD guard), otherwise either
remove it or replace it with "fall back to AVAILABLE TEXTURES" wording;
alternatively move this sentence out of the static prompt and into the runtime
user prompt path used by MaterialEditorQML (which already checks
sdManager->isModelLoaded()), so references to invented files are never emitted
when SD is unavailable (look for code that composes prompts in LLMManager::...
and MaterialEditorQML and update the logic around sdManager->isModelLoaded()).

In `@src/SDWorker.cpp`:
- Around line 161-179: recreateContext() currently sets m_isModelLoaded to false
on failure but does not emit model state signals or return failure, so callers
(e.g. SDManager) never get modelUnloaded/modelLoadError and QML stays stuck;
change recreateContext() to return a boolean status, set
m_isModelLoaded.store(false) on failure, emit the appropriate signal(s)
(modelUnloaded() or modelLoadError(QString) as used by SDManager) immediately
when creation fails, and return false so callers can surface the error before
any generation error is emitted; update callers to check the boolean return and
avoid proceeding with generation when false.

---

Outside diff comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 3002-3016: In onSDGenerationError(...) remove the unconditional
emit of aiGenerationCompleted(m_pendingMaterialScript) so a failed SD does not
signal the happy-path completion; instead just clear the pending state: set
m_sdPendingForMaterial = false, emit sdPendingForMaterialChanged(), and clear
m_pendingMaterialScript without emitting aiGenerationCompleted from
MaterialEditorQML::onSDGenerationError; keep the existing emits
sdGenerationError(...) and sdIsGeneratingChanged() so the UI can handle the
failure path.
- Around line 2971-2991: The current flow writes the edited Ogre material back
into m_materialText via updateMaterialText() even when m_sdPendingForMaterial is
set, which can overwrite the pending LLM-generated script; modify the code in
the texture-apply/completion path to guard against this by skipping the
updateMaterialText() and any assignment to m_materialText when
m_sdPendingForMaterial is true (or, alternatively, restore
m_pendingMaterialScript into m_materialText immediately before
updateMaterialText() if you prefer to serialize the pending script); reference
m_sdPendingForMaterial, m_materialText, m_pendingMaterialScript,
updateMaterialText(), and applyMaterial() to locate and fix the logic so the
deferred SD material is not overwritten.
- Around line 2793-2827: The loop in MaterialEditorQML.cpp currently stops after
the first missing texture (breaking), which leaves other referenced textures
unresolved; modify the logic around sdManager->generateTexture so it collects
all missing texture names found in cleanedText (while iterating
QRegularExpression matches) and triggers generation for each (calling
sdManager->generateTexture(cleanPrompt, 0, 0, texName) per missing texture)
instead of breaking after the first; keep setting m_pendingMaterialScript =
cleanedText and m_sdPendingForMaterial = true (and emit
sdPendingForMaterialChanged()) once if at least one texture was scheduled, set
sdTriggered = true when any are scheduled, and remove the break so all missing
textures are enqueued for generation.

In `@src/SDWorker.cpp`:
- Around line 197-212: After calling recreateContext() inside the generation
routine, check m_stopRequested.load() and bail out immediately if true: set
m_isGenerating.store(false) and return before emitting generationStarted() or
calling generate_image(); optionally emit a suitable stop signal (e.g.
generationStopped()) if your codebase expects it. This ensures a stop requested
while rebuilding the SD context prevents starting a full generation; locate the
check near recreateContext(), m_ctx handling, and before the call to
generate_image().

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2c2209df-0718-4033-b265-ff137cae4ad2

📥 Commits

Reviewing files that changed from the base of the PR and between c83aecb and bccc7af.

📒 Files selected for processing (11)
  • qml/MaterialEditorWindow.qml
  • qml/PassPropertiesPanel.qml
  • qml/TexturePropertiesPanel.qml
  • qml/ThemedTextField.qml
  • src/LLMManager.cpp
  • src/MaterialEditorQML.cpp
  • src/MaterialEditorQML.h
  • src/SDManager.cpp
  • src/SDManager.h
  • src/SDWorker.cpp
  • src/SDWorker.h

SD crash fix:
- Recreate sd_ctx before each generate_image() call — sd.cpp corrupts
  internal state after first generation on macOS Metal backend
- Remove img2img (generateFromImage/editTexture) entirely — crashes on
  Metal regardless of context recreation
- Remove edit texture UI panel (was using img2img)

Material generation pipeline:
- Defer material apply until SD finishes generating texture, avoiding
  intermediate state where material references nonexistent texture
- Unified progress bar: shows LLM progress then SD texture progress
- Register generated_textures dir as Ogre resource location at startup
  so textures from previous sessions are found immediately
- LLM system prompt now encourages texture_unit for realistic materials
  (was "Do NOT add texture_unit")

Dark theme fixes:
- Add themed background/label to all inner GroupBoxes in PassProperties
  and TextureProperties panels
- Add themed contentItem to all CheckBoxes in PassPropertiesPanel
- Fix ThemedTextField placeholder color to use disabledTextColor
- Fix Information/Preview/Texture Selection GroupBox labels

Other:
- PBR template → Normal Map template (only normal maps supported)
- Updated AI prompt placeholder text
- Preview cache-busting for regenerated textures with same filename

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@fernandotonon fernandotonon force-pushed the fix/sd-second-generation-crash branch from bccc7af to 54a76c3 Compare March 20, 2026 04:55
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/SDWorker.cpp (1)

205-213: Context recreation on every generation is expensive but necessary for Metal stability.

The workaround is well-documented. However, note that there's no debouncing in the call chain (MaterialEditorQML::generateTextureFromPromptSDManager::generateTextureSDWorker::generateTexture), so rapid successive calls will each trigger a full context free/reallocate cycle.

Consider adding a guard or debounce mechanism at the SDManager level if users can trigger rapid generation requests via QML. This is a performance optimization rather than a correctness issue.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/SDWorker.cpp` around lines 205 - 213, Rapid successive calls to
MaterialEditorQML::generateTextureFromPrompt → SDManager::generateTexture →
SDWorker::generateTexture will each recreate the expensive Metal context; add a
debounce/guard in SDManager::generateTexture to coalesce or reject rapid
requests (e.g., an atomic "isPending" flag or a short QTimer debounce window) so
that repeated calls within the debounce interval are ignored or merged and only
one call forwards to SDWorker::generateTexture; ensure the guard is thread-safe,
clears when the generation completes or errors, and preserves existing
error/success signaling to the caller.
src/MaterialEditorQML.cpp (1)

2794-2840: Only the first missing texture triggers SD generation.

The break at line 2828 means only the first missing texture in the material script will trigger SD generation. If the LLM generates a material with multiple missing textures (e.g., diffuse + normal map), only one will be auto-generated.

This may be intentional to simplify the workflow, but consider documenting this limitation or queueing multiple texture generations if multiple missing textures are common.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/MaterialEditorQML.cpp` around lines 2794 - 2840, The loop over texture
matches only triggers SD generation for the first missing texture because of the
break; remove the break and instead collect all missing texture names (or
immediately call SDManager::generateTexture for each missing texName) while
setting m_pendingMaterialScript = cleanedText and m_sdPendingForMaterial = true
once before triggering generation(s), set sdTriggered = true if any were queued,
and ensure onSDGenerationCompleted handles multiple completions (or tracks
remaining jobs) so aiGenerationCompleted is emitted only after all
generateTexture calls finish; key symbols: the QRegularExpression loop, texName,
sdManager->generateTexture(...), m_pendingMaterialScript,
m_sdPendingForMaterial, sdTriggered and onSDGenerationCompleted.
qml/PassPropertiesPanel.qml (1)

76-86: Consider extracting a reusable themed checkbox label pattern.

The same contentItem: Text block is repeated in multiple changed checkboxes. A small reusable component (e.g., ThemedCheckBox.qml) would reduce duplication and keep future theme changes centralized.

Also applies to: 96-106, 116-126, 217-227, 259-269, 301-311, 343-353

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@qml/PassPropertiesPanel.qml` around lines 76 - 86, Duplicate contentItem:
Text blocks in multiple checkbox instances should be replaced with a small
reusable component (e.g., ThemedCheckBox.qml) that exposes properties for text,
textColor, indicatorWidth and spacing and internally implements the shared
contentItem behavior (using parent.text, color, leftPadding: indicatorWidth +
spacing, verticalAlignment: Text.AlignVCenter). Create ThemedCheckBox.qml and
update each checkbox instance that uses the repeated block (the blocks around
contentItem: Text referencing parent.text and parent.indicator.width +
parent.spacing) to use this component and bind the appropriate properties so
theme changes are centralized.
🤖 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/PassPropertiesPanel.qml`:
- Around line 76-86: Replace usages of the SystemPalette-derived textColor bound
on the label Text elements with the app theme color from MaterialEditorQML (e.g.
change bindings that read "color: textColor" in the Text contentItem blocks to
use "color: MaterialEditorQML.textColor" or the appropriate MaterialEditorQML
label color property); update every matching Text instance inside the checkbox
rows (the contentItem: Text blocks that reference parent.text and leftPadding:
parent.indicator.width + parent.spacing) so labels follow the app theme rather
than the OS palette.

---

Nitpick comments:
In `@qml/PassPropertiesPanel.qml`:
- Around line 76-86: Duplicate contentItem: Text blocks in multiple checkbox
instances should be replaced with a small reusable component (e.g.,
ThemedCheckBox.qml) that exposes properties for text, textColor, indicatorWidth
and spacing and internally implements the shared contentItem behavior (using
parent.text, color, leftPadding: indicatorWidth + spacing, verticalAlignment:
Text.AlignVCenter). Create ThemedCheckBox.qml and update each checkbox instance
that uses the repeated block (the blocks around contentItem: Text referencing
parent.text and parent.indicator.width + parent.spacing) to use this component
and bind the appropriate properties so theme changes are centralized.

In `@src/MaterialEditorQML.cpp`:
- Around line 2794-2840: The loop over texture matches only triggers SD
generation for the first missing texture because of the break; remove the break
and instead collect all missing texture names (or immediately call
SDManager::generateTexture for each missing texName) while setting
m_pendingMaterialScript = cleanedText and m_sdPendingForMaterial = true once
before triggering generation(s), set sdTriggered = true if any were queued, and
ensure onSDGenerationCompleted handles multiple completions (or tracks remaining
jobs) so aiGenerationCompleted is emitted only after all generateTexture calls
finish; key symbols: the QRegularExpression loop, texName,
sdManager->generateTexture(...), m_pendingMaterialScript,
m_sdPendingForMaterial, sdTriggered and onSDGenerationCompleted.

In `@src/SDWorker.cpp`:
- Around line 205-213: Rapid successive calls to
MaterialEditorQML::generateTextureFromPrompt → SDManager::generateTexture →
SDWorker::generateTexture will each recreate the expensive Metal context; add a
debounce/guard in SDManager::generateTexture to coalesce or reject rapid
requests (e.g., an atomic "isPending" flag or a short QTimer debounce window) so
that repeated calls within the debounce interval are ignored or merged and only
one call forwards to SDWorker::generateTexture; ensure the guard is thread-safe,
clears when the generation completes or errors, and preserves existing
error/success signaling to the caller.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 57984d87-fbd1-4215-9885-434846abeb0b

📥 Commits

Reviewing files that changed from the base of the PR and between bccc7af and 54a76c3.

📒 Files selected for processing (11)
  • qml/MaterialEditorWindow.qml
  • qml/PassPropertiesPanel.qml
  • qml/TexturePropertiesPanel.qml
  • qml/ThemedTextField.qml
  • src/LLMManager.cpp
  • src/MaterialEditorQML.cpp
  • src/MaterialEditorQML.h
  • src/SDManager.cpp
  • src/SDManager.h
  • src/SDWorker.cpp
  • src/SDWorker.h
✅ Files skipped from review due to trivial changes (3)
  • qml/ThemedTextField.qml
  • src/LLMManager.cpp
  • qml/TexturePropertiesPanel.qml
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/SDManager.h
  • src/MaterialEditorQML.h
  • qml/MaterialEditorWindow.qml

Comment on lines +76 to +86
contentItem: Text {

text: parent.text

color: textColor

leftPadding: parent.indicator.width + parent.spacing

verticalAlignment: Text.AlignVCenter

}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use app theme colors for checkbox labels to avoid palette drift.

On Line 80, Line 100, Line 120, Line 221, Line 263, Line 305, and Line 347, checkbox label text is bound to textColor (SystemPalette), while the rest of the panel largely uses MaterialEditorQML.*. If app theme and OS palette diverge, label contrast can regress in dark mode.

🎨 Suggested fix
 contentItem: Text {
     text: parent.text
-    color: textColor
+    font: parent.font
+    color: parent.enabled
+           ? MaterialEditorQML.textColor
+           : MaterialEditorQML.disabledTextColor
     leftPadding: parent.indicator.width + parent.spacing
     verticalAlignment: Text.AlignVCenter
 }

Also applies to: 96-106, 116-126, 217-227, 259-269, 301-311, 343-353

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@qml/PassPropertiesPanel.qml` around lines 76 - 86, Replace usages of the
SystemPalette-derived textColor bound on the label Text elements with the app
theme color from MaterialEditorQML (e.g. change bindings that read "color:
textColor" in the Text contentItem blocks to use "color:
MaterialEditorQML.textColor" or the appropriate MaterialEditorQML label color
property); update every matching Text instance inside the checkbox rows (the
contentItem: Text blocks that reference parent.text and leftPadding:
parent.indicator.width + parent.spacing) so labels follow the app theme rather
than the OS palette.

@sonarqubecloud
Copy link

@fernandotonon fernandotonon merged commit 06c9438 into master Mar 20, 2026
17 checks passed
@fernandotonon fernandotonon deleted the fix/sd-second-generation-crash branch March 20, 2026 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant