Add AI texture generation with stable-diffusion.cpp#199
Merged
fernandotonon merged 3 commits intomasterfrom Mar 18, 2026
Merged
Add AI texture generation with stable-diffusion.cpp#199fernandotonon merged 3 commits intomasterfrom
fernandotonon merged 3 commits intomasterfrom
Conversation
Integrate stable-diffusion.cpp for AI-powered texture generation in the material editor. Users can generate textures from text prompts and the LLM material generator automatically triggers SD when it references textures that don't exist. Includes img2img support for editing existing textures with a strength slider. New files: SDWorker (worker thread wrapping sd.cpp C API), SDManager (QML singleton managing models/settings/generation), plus unit tests. Also improves UX: auto-load SD model on startup, texture-specific prompt enhancement for 3D PBR materials, SD model type auto-detection (Turbo/ Lightning/SDXL), progress bar during generation, dark gray viewport background, AI Settings dialog with scrollable tabs and SD model management in both QML and Widget implementations. Feature is opt-in via -DENABLE_STABLE_DIFFUSION=ON (OFF by default). Bumps version to 2.15.0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds optional Stable Diffusion-based AI texture generation behind CMake flag Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (QML UI)
participant MEQ as MaterialEditorQML
participant SDMgr as SDManager
participant SDWrk as SDWorker (Thread)
participant SD as stable-diffusion
User->>MEQ: generateTextureFromPrompt(prompt, width, height)
MEQ->>SDMgr: generateTexture(prompt, width, height, outputPath)
SDMgr->>SDMgr: enhanceTexturePrompt()
SDMgr->>SDWrk: generateTexture(enhancedPrompt, outputPath)
SDWrk->>SD: sd_generate_image(...)
loop Progress Updates
SD-->>SDWrk: progressCallback(step, total)
SDWrk-->>SDMgr: generationProgress(step, total)
SDMgr-->>MEQ: onSDGenerationProgress()
MEQ-->>User: update UI progress
end
SD-->>SDWrk: generation complete (image buffer)
SDWrk->>SDWrk: save PNG output
SDWrk-->>SDMgr: generationCompleted(outputPath)
SDMgr-->>MEQ: onSDGenerationCompleted(outputPath)
MEQ->>MEQ: register texture / apply to material
MEQ-->>User: sdTextureGenerated(filePath)
sequenceDiagram
participant User as User (Settings UI)
participant LLM as LLMSettingsWidget
participant SDMgr as SDManager
participant DL as ModelDownloader
participant SDWrk as SDWorker
User->>LLM: Click "Load SD Model"
LLM->>SDMgr: loadModel(modelName)
SDMgr->>SDWrk: loadModel(fullPath)
SDWrk-->>SDMgr: modelLoaded(modelPath)
SDMgr-->>LLM: modelLoadCompleted(modelName)
LLM-->>User: show "Model Loaded"
User->>LLM: Click "Download"
LLM->>DL: downloadModel(url, dest)
DL-->>LLM: downloadProgress(...)
DL-->>LLM: downloadCompleted()
LLM->>SDMgr: scanForModels()
SDMgr-->>LLM: availableModelsChanged()
LLM-->>User: refreshed model list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 5b46010dae
ℹ️ 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
+383
to
+387
| !fileName.endsWith(".jpg", Qt::CaseInsensitive)) { | ||
| fileName.replace(QRegularExpression(R"(\.\w+$)"), ""); | ||
| fileName += ".png"; | ||
| } | ||
| outputPath = outputDir.filePath(fileName); |
There was a problem hiding this comment.
When outputFileName contains path separators, outputDir.filePath(fileName) will happily resolve ../... segments or nested subdirectories outside generated_textures. MaterialEditorQML::onLLMGenerationCompleted() passes texture tokens from the LLM output straight into this parameter, so a material script like texture ../../foo.png or texture textures/albedo.png can make auto-generation overwrite files outside the intended cache directory instead of creating a local generated texture.
Useful? React with 👍 / 👎.
Comment on lines
+2793
to
+2797
| QStringList searchDirs = { | ||
| "media/materials/textures/" + texName, | ||
| "../media/materials/textures/" + texName, | ||
| }; | ||
| for (const auto &p : searchDirs) { |
There was a problem hiding this comment.
This existence check only covers generated_textures and two hard-coded media/materials/textures paths. The editor also loads textures from arbitrary resource groups (for example mainwindow.cpp:1166 and Manager.cpp:602), and getTexturePreviewPath() already relies on Ogre to find their real origin. With the current logic, an LLM-generated material that references an existing texture from an imported model/resource directory will be treated as missing, and the SD flow will generate and apply a replacement with the same name.
Useful? React with 👍 / 👎.
Comment on lines
+206
to
+210
| result = generate_image(m_ctx, &img_params); | ||
| } catch (const std::exception &e) { | ||
| // Clear callback before returning | ||
| sd_set_progress_callback(nullptr, nullptr); | ||
| m_isGenerating.store(false); |
There was a problem hiding this comment.
generate_image() is still running
Both stopGeneration() and model unloads only set m_stopRequested, but neither txt2img nor img2img consult that flag until after generate_image() returns. Because there is no in-flight cancellation path here, clicking Stop (or closing the app while generating) will still block until the full inference finishes, which makes long generations effectively uninterruptible.
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: 13
🧹 Nitpick comments (3)
src/LLMSettingsWidget.h (1)
17-17: Consider feature-gating SD-specific declarations in this header.Guarding SD-specific API/declarations with
#ifdef ENABLE_STABLE_DIFFUSIONkeeps the optional dependency boundary explicit and reduces accidental coupling in non-SD builds.As per coding guidelines: "
src/**/*.{h,cpp}: Guard features that depend on optional components (e.g., local LLM / llama.cpp) with#ifdefENABLE_LOCAL_LLM or skip gracefully in tests".Also applies to: 48-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LLMSettingsWidget.h` at line 17, Summary: SD-specific include and declarations (e.g., the `#include` "SDManager.h" and the SD-related declarations around the LLMSettingsWidget) should be feature-gated. Fix: wrap the SD-specific include and any declarations that reference SD types (like SDManager or SDManager* members, methods, or signals) in `#ifdef` ENABLE_STABLE_DIFFUSION / `#endif` blocks so non-SD builds don't depend on SD headers; for symbols used unconditionally, replace direct types with forward declarations inside the guard or use opaque pointers/conditional member presence (e.g., make SDManager* a member only inside the guard) and provide alternative code paths or no-op stubs when ENABLE_STABLE_DIFFUSION is not defined to keep compilation and tests working; update declarations in the same guarded region that currently span the SD-related block referenced in the review.src/SDManager.cpp (2)
380-386: Minor: QRegularExpression constructed on every call.Creating a
QRegularExpressionobject for each generation incurs compilation overhead. Consider using a static pattern orQString::section()for simple extension replacement.♻️ Proposed fix using static regex or QString methods
+namespace { + static const QRegularExpression s_extRegex(R"(\.\w+$)"); +} + void SDManager::generateTexture(const QString &prompt, int width, int height, const QString &outputFileName) { // ... earlier code ... if (!fileName.endsWith(".png", Qt::CaseInsensitive) && !fileName.endsWith(".jpg", Qt::CaseInsensitive)) { - fileName.replace(QRegularExpression(R"(\.\w+$)"), ""); + fileName.replace(s_extRegex, ""); fileName += ".png"; }Apply the same pattern at line 427.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SDManager.cpp` around lines 380 - 386, The code creates a QRegularExpression each call when stripping extensions for fileName; change it to use a static QRegularExpression instance (e.g. static const QRegularExpression extRe(R"(\.\w+$)");) or use QString::section/split to remove the extension, then append ".png"; update the same replacement logic wherever the same pattern is used so all extension-removal spots use the static regex or QString methods instead of constructing a new QRegularExpression per call.
280-298: Model type auto-detection may produce false positives.Concatenating
modelName.toLower() + modelPath.toLower()and checking for substrings like "turbo" or "lightning" could match unintended filenames (e.g., a custom model namedmy_turbo_test.safetensors). Consider being more specific or allowing user override.💡 Consider more specific pattern matching
QString lowerName = modelName.toLower() + modelPath.toLower(); - if (lowerName.contains("turbo")) { + // Match known turbo model patterns more specifically + if (lowerName.contains("sdxl_turbo") || lowerName.contains("sdxl-turbo") || + lowerName.contains("sd_turbo") || lowerName.contains("sd-turbo")) {Or add a "Use auto-detected settings" checkbox in UI so users can override when needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SDManager.cpp` around lines 280 - 298, The auto-detection concatenates modelName and modelPath into lowerName and uses substring checks which can yield false positives; update SDManager to detect only the actual model filename or explicit model identifier (use modelPath's basename or modelName alone) and replace naive contains(...) checks with stricter pattern matching (e.g., regex with word boundaries or exact token matching for "turbo", "lightning", "sdxl"/"sd_xl") before mutating m_settings and emitting settingsChanged; also add a user-controllable override flag/property (e.g., m_useAutoDetect or a "Use auto-detected settings" setting exposed in the UI) so users can disable/ignore auto-adjustments when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 132: The CLAUDE.md line claiming "sd.cpp has `if (NOT TARGET ggml)`
guard" is incorrect; either update the documentation to remove/correct this
claim or add the missing guard to the build so the claim is true. Locate the
ENABLE_STABLE_DIFFUSION block in CMakeLists.txt (the snippet around the current
217-257 range) and either (A) add an explicit guard using `if (NOT TARGET ggml)`
(or equivalent target check) around the sd.cpp target so it will reuse
llama.cpp's ggml when both features are enabled, or (B) edit CLAUDE.md to remove
the assertion and instead describe the actual behavior of the
ENABLE_STABLE_DIFFUSION block and how ggml targets are handled; reference sd.cpp
and the ENABLE_STABLE_DIFFUSION CMake block in your change so readers can find
the implementation.
In `@CMakeLists.txt`:
- Around line 241-245: The FetchContent_Declare blocks for the external deps
(notably the stable_diffusion_cpp declaration named "stable_diffusion_cpp" and
the similar declaration for "llama_cpp") are pinned to the floating branch via
GIT_TAG master; replace each GIT_TAG master with a specific immutable release
tag or commit SHA (for example a release like vX.Y.Z or a full commit hash)
taken from the upstream releases/commits for leejet/stable-diffusion.cpp and
ggerganov/llama.cpp so builds become reproducible and stable.
In `@src/LLMSettingsWidget.cpp`:
- Around line 511-516: The SD download flow lacks proper cancel/error recovery:
in onSDDownloadModelClicked() you disable only m_sdDownloadButton but never
re-enable the shared cancel control or restore the SD button on cancel/error;
update the download start logic to also enable the shared cancel button and
register failure/cancellation callbacks (or connect to SDManager's error/cancel
signals) that re-enable m_sdDownloadButton and the shared cancel button and
restore any UI state; ensure the same recovery is applied where
updateSDModelList()/updateSDRecommendedModelsList() and
SDManager::scanForModels() are used so both success and failure/cancel paths
consistently restore the UI.
- Around line 66-75: The new SD settings were added as QWidget tabs using
ENABLE_STABLE_DIFFUSION, setupSDModelsTab, setupSDSettingsTab and m_tabWidget,
which duplicates the QML UI; remove the QWidget-based SDModels/SDSettings tab
creation and instead open or reuse the existing QML settings surface (e.g.,
qml/AISettingsDialog.qml) from this code path; replace the
m_tabWidget->addTab(...) calls and the setupSD*Tab(...) invocations with logic
to invoke the QML dialog (or emit a signal that causes the QML dialog to open)
so the SD settings remain implemented in QML per the project guideline.
In `@src/LLMSettingsWidget.h`:
- Around line 48-66: The new SD Widgets additions in LLMSettingsWidget (slots:
onSDLoadModelClicked, onSDUnloadModelClicked, onSDRefreshModelsClicked,
onSDDownloadModelClicked, onSDModelLoadCompleted, onSDModelLoadError,
onSDModelUnloaded, onSDSettingsChanged, onSDApplySettings and private methods
setupUI, setupModelsTab, setupSettingsTab, setupDownloadTab, setupSDModelsTab,
setupSDSettingsTab) must not create QWidget UI; move the SD UI into QML and keep
this header/class as a minimal bridge: remove any Widget construction
responsibilities (delete/disable setup* methods that build widgets), expose only
the necessary Q_INVOKABLEs, signals, and properties that QML will call (e.g.,
keep the onSD* methods as thin wrappers or rename to invokable methods), and
wire these to the SD backend/controller instead of instantiating widgets; ensure
registration for QML (if needed) and update callers to load a QML component for
models/settings instead of calling the removed setupSD* widget builders.
In `@src/main.cpp`:
- Line 19: The SDManager include and the SD singleton
registration/initialization are unconditional; wrap the SD-related code
(including the `#include` "SDManager.h" and the SD singleton wiring/initialization
code referenced around the SD registration block and the code at lines
corresponding to the SD setup) with an `#ifdef` ENABLE_STABLE_DIFFUSION / `#endif`
guard so that SDManager and any SD singleton creation only compile and run when
ENABLE_STABLE_DIFFUSION is defined; locate symbols SDManager and the SD
singleton/registration calls in main.cpp and enclose both the include and the
initialization/registration logic in the guard (skip or provide a no-op path
when the macro is not defined) to prevent startup side effects for builds that
opt out.
In `@src/MaterialEditorQML.cpp`:
- Around line 83-89: The stop path never triggers the sdIsGeneratingChanged
notify, so add handling for the SD stop signal: connect the SDManager
stop/cancel signal (e.g., SDManager::generationStopped or generationCanceled) to
a new or existing handler in MaterialEditorQML (e.g.,
onSDGenerationStopped/onSDGenerationCanceled) and in that handler emit
sdIsGeneratingChanged() (and update any internal state if needed) so QML
refreshes the computed sdIsGenerating property when the user stops a generation.
Ensure the connect call is added alongside the other SDManager connections and
the handler emits the notify signal.
- Around line 2777-2810: The code only checks
SDManager::instance()->isModelLoaded() before calling
SDManager->generateTexture, so if a generation is already running the SDWorker
will reject it; add a guard that checks whether the SD subsystem is currently
busy (e.g., call an existing status method like
SDManager::isGenerationInProgress() or SDManager::isBusy(), or query SDWorker's
busy flag) before calling generateTexture and skip/queue the auto-trigger when
busy; place this check immediately before the
sdManager->generateTexture(cleanPrompt, 0, 0, texName) call (inside the same
block after verifying cleanPrompt), and do not call generateTexture if the busy
check returns true to avoid the “Generation already in progress” rejection.
- Around line 20-22: Include the missing QRegularExpression header so usages of
QRegularExpression in MaterialEditorQML (e.g., the occurrences referenced at
lines using QRegularExpression) don't rely on transitive includes; add an
explicit `#include` <QRegularExpression> among the other Qt includes in
MaterialEditorQML.cpp so the symbol is always available under stricter
toolchains.
In `@src/SDManager_test.cpp`:
- Around line 113-127: The tests assert signal emission synchronously and can
flake; replace the immediate ASSERT_EQ checks with a Qt wait-style assertion so
the test waits for queued delivery (e.g., use QTRY_COMPARE or QTRY_VERIFY with a
timeout) for the QSignalSpy counts used in SDManagerTest for errorSpy on signals
modelLoadError and generationError after calling loadModel and generateTexture
respectively; ensure you reference the same QSignalSpy instances (errorSpy) and
the methods loadModel/generateTexture and signals modelLoadError/generationError
when updating the assertions.
- Around line 14-24: Tests share the SDManager singleton causing order-dependent
failures; modify the test fixture so SetUp ensures a fresh SDManager state and
TearDown clears it and any persisted settings: call a reset hook (e.g.,
SDManager::resetForTests() or delete and recreate the singleton) before
obtaining manager in SetUp (the SetUp function) and implement TearDown to call
the same reset and clear QSettings or any stored configuration so each test
starts with default state; update tests that reference manager accordingly.
In `@src/SDWorker_test.cpp`:
- Around line 123-130: The test GenerateWithoutModel is Unix-specific and races
on the signal; change it to use a Qt-generated temp path (e.g., create a
QTemporaryFile or use QDir::tempPath() + unique name) instead of
"/tmp/test.png", and make the assertion async-safe by waiting for the
generationError signal (use QSignalSpy::wait(timeout) or
QTRY_COMPARE/QTRY_VERIFY) before checking the spy; locate the test body in
SDWorkerTest, reference SDWorker::generateTexture and the generationError signal
when making these changes.
In `@src/SDWorker.cpp`:
- Around line 149-152: The requestStop() currently only sets m_stopRequested but
does not interrupt ongoing inference; modify generate_image() (and any inference
loop inside SDWorker) and the progress callback to periodically check
m_stopRequested and abort the generation immediately when set, ensuring the
inference loop returns early; upon detecting the stop flag, perform the same
cleanup and emit generationStopped() (or call the existing stop/cleanup helper)
so unload/load can preempt running jobs; keep requestStop() as the setter for
m_stopRequested and ensure any worker thread or blocking API used by
generate_image() also checks or is made interruptible so the stop takes effect
promptly.
---
Nitpick comments:
In `@src/LLMSettingsWidget.h`:
- Line 17: Summary: SD-specific include and declarations (e.g., the `#include`
"SDManager.h" and the SD-related declarations around the LLMSettingsWidget)
should be feature-gated. Fix: wrap the SD-specific include and any declarations
that reference SD types (like SDManager or SDManager* members, methods, or
signals) in `#ifdef` ENABLE_STABLE_DIFFUSION / `#endif` blocks so non-SD builds
don't depend on SD headers; for symbols used unconditionally, replace direct
types with forward declarations inside the guard or use opaque
pointers/conditional member presence (e.g., make SDManager* a member only inside
the guard) and provide alternative code paths or no-op stubs when
ENABLE_STABLE_DIFFUSION is not defined to keep compilation and tests working;
update declarations in the same guarded region that currently span the
SD-related block referenced in the review.
In `@src/SDManager.cpp`:
- Around line 380-386: The code creates a QRegularExpression each call when
stripping extensions for fileName; change it to use a static QRegularExpression
instance (e.g. static const QRegularExpression extRe(R"(\.\w+$)");) or use
QString::section/split to remove the extension, then append ".png"; update the
same replacement logic wherever the same pattern is used so all
extension-removal spots use the static regex or QString methods instead of
constructing a new QRegularExpression per call.
- Around line 280-298: The auto-detection concatenates modelName and modelPath
into lowerName and uses substring checks which can yield false positives; update
SDManager to detect only the actual model filename or explicit model identifier
(use modelPath's basename or modelName alone) and replace naive contains(...)
checks with stricter pattern matching (e.g., regex with word boundaries or exact
token matching for "turbo", "lightning", "sdxl"/"sd_xl") before mutating
m_settings and emitting settingsChanged; also add a user-controllable override
flag/property (e.g., m_useAutoDetect or a "Use auto-detected settings" setting
exposed in the UI) so users can disable/ignore auto-adjustments when needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbb584e1-dc4f-4378-8314-0867cca23045
📒 Files selected for processing (18)
CLAUDE.mdCMakeLists.txtqml/AISettingsDialog.qmlqml/TexturePropertiesPanel.qmlsrc/CMakeLists.txtsrc/LLMSettingsWidget.cppsrc/LLMSettingsWidget.hsrc/LLMSettingsWidget_test.cppsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/OgreWidget.cppsrc/SDManager.cppsrc/SDManager.hsrc/SDManager_test.cppsrc/SDWorker.cppsrc/SDWorker.hsrc/SDWorker_test.cppsrc/main.cpp
Comment on lines
+241
to
+245
| FetchContent_Declare( | ||
| stable_diffusion_cpp | ||
| GIT_REPOSITORY https://github.com/leejet/stable-diffusion.cpp.git | ||
| GIT_TAG master | ||
| GIT_SHALLOW TRUE |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current stable-diffusion pin:"
rg -n 'stable_diffusion_cpp|GIT_TAG' CMakeLists.txt
echo
echo "Recent upstream tags:"
gh api repos/leejet/stable-diffusion.cpp/tags --paginate -q '.[0:20].[].name'Repository: fernandotonon/QtMeshEditor
Length of output: 507
🏁 Script executed:
# Verify exact code at the flagged lines
sed -n '240,250p' CMakeLists.txtRepository: fernandotonon/QtMeshEditor
Length of output: 361
🏁 Script executed:
# Check all floating GIT_TAG master references
rg -n 'GIT_TAG\s+master' CMakeLists.txt -A 2 -B 2Repository: fernandotonon/QtMeshEditor
Length of output: 416
🏁 Script executed:
# Try to find stable-diffusion.cpp release information via web or documentation
find . -name "*.md" -o -name "*.txt" | xargs grep -l "stable-diffusion" 2>/dev/null | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 119
🏁 Script executed:
# Check CLAUDE.md for any guidance on dependencies or versions
cat CLAUDE.md | grep -i -A 3 -B 3 'stable-diffusion\|llama\|fetchcontent\|git_tag'Repository: fernandotonon/QtMeshEditor
Length of output: 2853
🏁 Script executed:
# Also check src/CMakeLists.txt in case there are version hints
cat src/CMakeLists.txt | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 1108
Pin stable_diffusion_cpp and llama_cpp to immutable commit SHAs or release tags (avoid floating master branch).
Both dependencies at lines 201 and 244 use GIT_TAG master, making builds non-reproducible and exposing the project to unexpected upstream changes. Pinning to a specific commit or release tag is essential for build stability and supply-chain security.
To fix:
- Identify the current stable versions from upstream repositories:
- Replace both
GIT_TAG masterentries with a stable commit SHA or release tag (e.g.,GIT_TAG v1.2.3orGIT_TAG abc123def456).
Example CMake changes
FetchContent_Declare(
llama_cpp
GIT_REPOSITORY https://github.com/ggerganov/llama.cpp.git
- GIT_TAG master
+ GIT_TAG <pinned_tag_or_commit_sha>
GIT_SHALLOW TRUE
) FetchContent_Declare(
stable_diffusion_cpp
GIT_REPOSITORY https://github.com/leejet/stable-diffusion.cpp.git
- GIT_TAG master
+ GIT_TAG <pinned_tag_or_commit_sha>
GIT_SHALLOW TRUE
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 241 - 245, The FetchContent_Declare blocks for
the external deps (notably the stable_diffusion_cpp declaration named
"stable_diffusion_cpp" and the similar declaration for "llama_cpp") are pinned
to the floating branch via GIT_TAG master; replace each GIT_TAG master with a
specific immutable release tag or commit SHA (for example a release like vX.Y.Z
or a full commit hash) taken from the upstream releases/commits for
leejet/stable-diffusion.cpp and ggerganov/llama.cpp so builds become
reproducible and stable.
Comment on lines
+66
to
+75
| #ifdef ENABLE_STABLE_DIFFUSION | ||
| QWidget *sdModelsTab = new QWidget(); | ||
| QWidget *sdSettingsTab = new QWidget(); | ||
|
|
||
| setupSDModelsTab(sdModelsTab); | ||
| setupSDSettingsTab(sdSettingsTab); | ||
|
|
||
| m_tabWidget->addTab(sdModelsTab, "SD Models"); | ||
| m_tabWidget->addTab(sdSettingsTab, "SD Settings"); | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Please keep the new SD settings surface in QML instead of expanding the QWidget path.
This adds a second full SD UI alongside qml/AISettingsDialog.qml, which increases maintenance during the migration and goes against the repo rule for new UI. Prefer opening/reusing the QML settings dialog from here rather than growing the QWidget implementation further.
As per coding guidelines, "New UI should be built in QML (Qt Quick), not Qt Widgets. The project is migrating from Widgets to QML."
Also applies to: 574-717
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LLMSettingsWidget.cpp` around lines 66 - 75, The new SD settings were
added as QWidget tabs using ENABLE_STABLE_DIFFUSION, setupSDModelsTab,
setupSDSettingsTab and m_tabWidget, which duplicates the QML UI; remove the
QWidget-based SDModels/SDSettings tab creation and instead open or reuse the
existing QML settings surface (e.g., qml/AISettingsDialog.qml) from this code
path; replace the m_tabWidget->addTab(...) calls and the setupSD*Tab(...)
invocations with logic to invoke the QML dialog (or emit a signal that causes
the QML dialog to open) so the SD settings remain implemented in QML per the
project guideline.
Comment on lines
+511
to
+516
| #ifdef ENABLE_STABLE_DIFFUSION | ||
| SDManager::instance()->scanForModels(); | ||
| updateSDModelList(); | ||
| updateSDRecommendedModelsList(); | ||
| m_sdDownloadButton->setEnabled(true); | ||
| #endif |
There was a problem hiding this comment.
SD downloads do not get a complete cancel/error recovery path.
onSDDownloadModelClicked() disables only m_sdDownloadButton, but it never enables the shared cancel button, and the SD button is explicitly restored only on the success path here. If an SD download is cancelled or errors, the user can end up with no working cancel control and a permanently disabled SD download button.
Also applies to: 809-837
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LLMSettingsWidget.cpp` around lines 511 - 516, The SD download flow lacks
proper cancel/error recovery: in onSDDownloadModelClicked() you disable only
m_sdDownloadButton but never re-enable the shared cancel control or restore the
SD button on cancel/error; update the download start logic to also enable the
shared cancel button and register failure/cancellation callbacks (or connect to
SDManager's error/cancel signals) that re-enable m_sdDownloadButton and the
shared cancel button and restore any UI state; ensure the same recovery is
applied where updateSDModelList()/updateSDRecommendedModelsList() and
SDManager::scanForModels() are used so both success and failure/cancel paths
consistently restore the UI.
Comment on lines
+48
to
66
| // SD slots | ||
| void onSDLoadModelClicked(); | ||
| void onSDUnloadModelClicked(); | ||
| void onSDRefreshModelsClicked(); | ||
| void onSDDownloadModelClicked(); | ||
| void onSDModelLoadCompleted(const QString &modelName); | ||
| void onSDModelLoadError(const QString &error); | ||
| void onSDModelUnloaded(); | ||
| void onSDSettingsChanged(); | ||
| void onSDApplySettings(); | ||
|
|
||
| private: | ||
| void setupUI(); | ||
| void setupModelsTab(QWidget *parent); | ||
| void setupSettingsTab(QWidget *parent); | ||
| void setupDownloadTab(QWidget *parent); | ||
| void setupSDModelsTab(QWidget *parent); | ||
| void setupSDSettingsTab(QWidget *parent); | ||
|
|
There was a problem hiding this comment.
New SD UI surface in LLMSettingsWidget conflicts with QML-first migration.
These additions introduce new UI in the Widgets layer. Please move SD models/settings UI construction to QML and keep this class as a minimal bridge if still required.
As per coding guidelines: "{qml/**/*.qml,src/**/*[Ww]idget*.{h,cpp},src/**/ui_*.h,ui_files/**/*}: New UI should be built in QML (Qt Quick), not Qt Widgets. The project is migrating from Widgets to QML."
Also applies to: 110-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LLMSettingsWidget.h` around lines 48 - 66, The new SD Widgets additions
in LLMSettingsWidget (slots: onSDLoadModelClicked, onSDUnloadModelClicked,
onSDRefreshModelsClicked, onSDDownloadModelClicked, onSDModelLoadCompleted,
onSDModelLoadError, onSDModelUnloaded, onSDSettingsChanged, onSDApplySettings
and private methods setupUI, setupModelsTab, setupSettingsTab, setupDownloadTab,
setupSDModelsTab, setupSDSettingsTab) must not create QWidget UI; move the SD UI
into QML and keep this header/class as a minimal bridge: remove any Widget
construction responsibilities (delete/disable setup* methods that build
widgets), expose only the necessary Q_INVOKABLEs, signals, and properties that
QML will call (e.g., keep the onSD* methods as thin wrappers or rename to
invokable methods), and wire these to the SD backend/controller instead of
instantiating widgets; ensure registration for QML (if needed) and update
callers to load a QML component for models/settings instead of calling the
removed setupSD* widget builders.
Comment on lines
+14
to
+24
| void SetUp() override | ||
| { | ||
| app = qobject_cast<QApplication*>(QCoreApplication::instance()); | ||
| ASSERT_NE(app, nullptr); | ||
| manager = SDManager::instance(); | ||
| ASSERT_NE(manager, nullptr); | ||
| } | ||
|
|
||
| QApplication* app = nullptr; | ||
| SDManager* manager = nullptr; | ||
| }; |
There was a problem hiding this comment.
Isolate SDManager singleton state per test to prevent order-dependent failures.
The suite shares one singleton and mutates settings; default-state assertions can become flaky if prior tests or persisted settings leak state.
🧪 Suggested stabilization pattern
class SDManagerTest : public ::testing::Test
{
protected:
void SetUp() override
{
app = qobject_cast<QApplication*>(QCoreApplication::instance());
ASSERT_NE(app, nullptr);
manager = SDManager::instance();
ASSERT_NE(manager, nullptr);
+ // Normalize singleton state for deterministic tests
+ manager->unloadModel();
+ manager->setImageWidth(512);
+ manager->setImageHeight(512);
+ manager->setSteps(20);
+ manager->setCfgScale(7.0f);
+ manager->setNegativePrompt("");
}Also applies to: 33-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SDManager_test.cpp` around lines 14 - 24, Tests share the SDManager
singleton causing order-dependent failures; modify the test fixture so SetUp
ensures a fresh SDManager state and TearDown clears it and any persisted
settings: call a reset hook (e.g., SDManager::resetForTests() or delete and
recreate the singleton) before obtaining manager in SetUp (the SetUp function)
and implement TearDown to call the same reset and clear QSettings or any stored
configuration so each test starts with default state; update tests that
reference manager accordingly.
Comment on lines
+113
to
+127
| TEST_F(SDManagerTest, LoadModelNotFound) | ||
| { | ||
| QSignalSpy errorSpy(manager, &SDManager::modelLoadError); | ||
| manager->loadModel("nonexistent_model"); | ||
| ASSERT_EQ(errorSpy.count(), 1); | ||
| EXPECT_TRUE(errorSpy.first().first().toString().contains("not found")); | ||
| } | ||
|
|
||
| TEST_F(SDManagerTest, GenerateWithoutModel) | ||
| { | ||
| QSignalSpy errorSpy(manager, &SDManager::generationError); | ||
| manager->generateTexture("test prompt"); | ||
| ASSERT_EQ(errorSpy.count(), 1); | ||
| EXPECT_TRUE(errorSpy.first().first().toString().contains("No SD model loaded")); | ||
| } |
There was a problem hiding this comment.
Make signal assertions tolerant to queued delivery.
ASSERT_EQ(errorSpy.count(), 1) immediately after action can flake if emission is asynchronous. Wait conditionally before asserting.
✅ Suggested async-safe assertion
TEST_F(SDManagerTest, LoadModelNotFound)
{
QSignalSpy errorSpy(manager, &SDManager::modelLoadError);
manager->loadModel("nonexistent_model");
+ if (errorSpy.count() == 0) {
+ ASSERT_TRUE(errorSpy.wait(1000));
+ }
ASSERT_EQ(errorSpy.count(), 1);
EXPECT_TRUE(errorSpy.first().first().toString().contains("not found"));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SDManager_test.cpp` around lines 113 - 127, The tests assert signal
emission synchronously and can flake; replace the immediate ASSERT_EQ checks
with a Qt wait-style assertion so the test waits for queued delivery (e.g., use
QTRY_COMPARE or QTRY_VERIFY with a timeout) for the QSignalSpy counts used in
SDManagerTest for errorSpy on signals modelLoadError and generationError after
calling loadModel and generateTexture respectively; ensure you reference the
same QSignalSpy instances (errorSpy) and the methods loadModel/generateTexture
and signals modelLoadError/generationError when updating the assertions.
Comment on lines
+123
to
+130
| TEST_F(SDWorkerTest, GenerateWithoutModel) | ||
| { | ||
| SDWorker worker; | ||
| QSignalSpy errorSpy(&worker, &SDWorker::generationError); | ||
| worker.generateTexture("test prompt", "/tmp/test.png"); | ||
| ASSERT_EQ(errorSpy.count(), 1); | ||
| EXPECT_TRUE(errorSpy.first().first().toString().contains("No SD model loaded")); | ||
| } |
There was a problem hiding this comment.
Make GenerateWithoutModel platform-neutral and async-safe.
Using "/tmp/test.png" is Unix-specific, and asserting count() immediately can flake if emission becomes queued. Prefer a temp path from Qt and wait conditionally for the signal.
✅ Suggested test hardening
+#include <QDir>
...
TEST_F(SDWorkerTest, GenerateWithoutModel)
{
SDWorker worker;
QSignalSpy errorSpy(&worker, &SDWorker::generationError);
- worker.generateTexture("test prompt", "/tmp/test.png");
+ const QString outPath = QDir::temp().filePath("sdworker_test.png");
+ worker.generateTexture("test prompt", outPath);
+ if (errorSpy.count() == 0) {
+ ASSERT_TRUE(errorSpy.wait(1000));
+ }
ASSERT_EQ(errorSpy.count(), 1);
EXPECT_TRUE(errorSpy.first().first().toString().contains("No SD model loaded"));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TEST_F(SDWorkerTest, GenerateWithoutModel) | |
| { | |
| SDWorker worker; | |
| QSignalSpy errorSpy(&worker, &SDWorker::generationError); | |
| worker.generateTexture("test prompt", "/tmp/test.png"); | |
| ASSERT_EQ(errorSpy.count(), 1); | |
| EXPECT_TRUE(errorSpy.first().first().toString().contains("No SD model loaded")); | |
| } | |
| TEST_F(SDWorkerTest, GenerateWithoutModel) | |
| { | |
| SDWorker worker; | |
| QSignalSpy errorSpy(&worker, &SDWorker::generationError); | |
| const QString outPath = QDir::temp().filePath("sdworker_test.png"); | |
| worker.generateTexture("test prompt", outPath); | |
| if (errorSpy.count() == 0) { | |
| ASSERT_TRUE(errorSpy.wait(1000)); | |
| } | |
| ASSERT_EQ(errorSpy.count(), 1); | |
| EXPECT_TRUE(errorSpy.first().first().toString().contains("No SD model loaded")); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SDWorker_test.cpp` around lines 123 - 130, The test GenerateWithoutModel
is Unix-specific and races on the signal; change it to use a Qt-generated temp
path (e.g., create a QTemporaryFile or use QDir::tempPath() + unique name)
instead of "/tmp/test.png", and make the assertion async-safe by waiting for the
generationError signal (use QSignalSpy::wait(timeout) or
QTRY_COMPARE/QTRY_VERIFY) before checking the spy; locate the test body in
SDWorkerTest, reference SDWorker::generateTexture and the generationError signal
when making these changes.
Comment on lines
+149
to
+152
| void SDWorker::requestStop() | ||
| { | ||
| m_stopRequested.store(true); | ||
| } |
There was a problem hiding this comment.
requestStop() never interrupts inference; it only drops the finished image.
m_stopRequested is inspected only after generate_image() returns, and the progress callback never consults it. The Stop action therefore waits for the full generation to finish before emitting generationStopped(), which also means unload/load cannot preempt a running job.
Also applies to: 223-227, 346-350, 388-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SDWorker.cpp` around lines 149 - 152, The requestStop() currently only
sets m_stopRequested but does not interrupt ongoing inference; modify
generate_image() (and any inference loop inside SDWorker) and the progress
callback to periodically check m_stopRequested and abort the generation
immediately when set, ensuring the inference loop returns early; upon detecting
the stop flag, perform the same cleanup and emit generationStopped() (or call
the existing stop/cleanup helper) so unload/load can preempt running jobs; keep
requestStop() as the setter for m_stopRequested and ensure any worker thread or
blocking API used by generate_image() also checks or is made interruptible so
the stop takes effect promptly.
- Guard all SDManager usage with #ifdef ENABLE_STABLE_DIFFUSION in MaterialEditorQML.cpp, LLMSettingsWidget.h/cpp, and main.cpp to fix linker errors when building without SD (CI scenario) - Sanitize SD output filenames with QFileInfo::fileName() to prevent path traversal (P1 security fix) - Emit sdIsGeneratingChanged() in stopTextureGeneration() so QML updates - Guard LLM->SD auto-trigger with !isGenerating() check - Add LCOV_EXCL markers on SD code paths requiring real model files - Add QCoreApplication::processEvents() in test for async signal safety - Fix CLAUDE.md incorrect claim about ggml guard - Move all SD widget declarations/definitions under #ifdef guards Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/MaterialEditorQML.cpp (2)
22-24:⚠️ Potential issue | 🟠 MajorAdd explicit
<QRegularExpression>include for symbols used in this.cpp.This TU directly uses
QRegularExpressionbut does not include its header, relying on transitive includes.#!/bin/bash # Verify direct usage vs explicit include in MaterialEditorQML rg -n 'QRegularExpression|QRegularExpressionMatchIterator' src/MaterialEditorQML.cpp rg -n '^#include <QRegularExpression>' src/MaterialEditorQML.cppAlso applies to: 2785-2786
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialEditorQML.cpp` around lines 22 - 24, The TU uses QRegularExpression (and possibly QRegularExpressionMatchIterator) without a direct include, so add an explicit `#include` <QRegularExpression> at the top of MaterialEditorQML.cpp (near the other Qt includes) to avoid relying on transitive includes; ensure the include is added alongside `#include` <QUrl>, `#include` <QFile>, `#include` <QPalette> so references to QRegularExpression compile independently.
85-93:⚠️ Potential issue | 🟠 Major
sdIsGeneratingChangedstill misses true stop completion events.
MaterialEditorQMLis not connected toSDManager::generationStopped. Emitting instopTextureGeneration()only covers that one call path and can notify too early.Suggested signal wiring fix
`#ifdef` ENABLE_STABLE_DIFFUSION SDManager *sdManager = SDManager::instance(); connect(sdManager, &SDManager::generationStarted, this, &MaterialEditorQML::onSDGenerationStarted); connect(sdManager, &SDManager::generationProgressChanged, this, &MaterialEditorQML::onSDGenerationProgress); connect(sdManager, &SDManager::generationCompleted, this, &MaterialEditorQML::onSDGenerationCompleted); connect(sdManager, &SDManager::generationError, this, &MaterialEditorQML::onSDGenerationError); + connect(sdManager, &SDManager::generationStopped, this, [this]() { + m_sdGenerationProgress = 0.0f; + emit sdGenerationProgressChanged(); + emit sdIsGeneratingChanged(); + }); connect(sdManager, &SDManager::modelLoadedChanged, this, &MaterialEditorQML::onSDModelLoadedChanged); `#endif`Also applies to: 2948-2953
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialEditorQML.cpp` around lines 85 - 93, MaterialEditorQML is not listening for SDManager::generationStopped, so sdIsGenerating can miss true stop events; add a connection for SDManager::generationStopped in the same block where other signals are connected (SDManager::instance()) and implement or wire it to a handler (e.g., onSDGenerationStopped) that mirrors onSDGenerationCompleted behavior (clear generating state, update progress/UI) instead of relying on stopTextureGeneration() to emit the change; update any duplicate connection blocks (e.g., the one around lines 2948-2953) to include the same generationStopped -> onSDGenerationStopped mapping.src/LLMSettingsWidget.cpp (2)
70-79:⚠️ Potential issue | 🟠 MajorPlease keep new SD settings UI in QML, not additional QWidget tabs.
This expands the Widget path with new SD tabs instead of reusing/opening the existing QML settings surface, which increases migration debt.
As per coding guidelines, "New UI should be built in QML (Qt Quick), not Qt Widgets. The project is migrating from Widgets to QML."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LLMSettingsWidget.cpp` around lines 70 - 79, The new SD UI was added as QWidget tabs under the ENABLE_STABLE_DIFFUSION block (creating sdModelsTab/sdSettingsTab, calling setupSDModelsTab/setupSDSettingsTab, and adding them to m_tabWidget), but per guidelines this must be implemented in QML; remove or revert adding QWidget-based SD tabs and instead wire the existing QML settings surface to expose the SD Models and SD Settings pages (e.g., remove creation/registration of sdModelsTab/sdSettingsTab and the calls to setupSDModelsTab/setupSDSettingsTab and m_tabWidget->addTab) and implement the SD UI as QML components accessible from the QML settings view or by invoking the QML settings page from C++ when ENABLE_STABLE_DIFFUSION is defined.
817-820:⚠️ Potential issue | 🟠 MajorSD download button can stay disabled after cancel/error.
onSDDownloadModelClicked()disablesm_sdDownloadButton, but cancel/error paths do not reliably restore it. Success restores it, cancel/error do not.Minimal state-recovery fix
void LLMSettingsWidget::onSDDownloadModelClicked() { @@ m_sdDownloadButton->setEnabled(false); + m_cancelDownloadButton->setEnabled(true); m_downloadStatusLabel->setText(QString("Downloading %1...").arg(modelName)); @@ } void LLMSettingsWidget::onCancelDownloadClicked() { ModelDownloader::instance()->cancelDownload(); m_downloadButton->setEnabled(true); m_cancelDownloadButton->setEnabled(false); +#ifdef ENABLE_STABLE_DIFFUSION + m_sdDownloadButton->setEnabled(true); +#endif @@ } void LLMSettingsWidget::onDownloadError(const QString &modelName, const QString &error) { m_downloadButton->setEnabled(true); m_cancelDownloadButton->setEnabled(false); +#ifdef ENABLE_STABLE_DIFFUSION + m_sdDownloadButton->setEnabled(true); +#endif @@ }Also applies to: 454-462, 526-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LLMSettingsWidget.cpp` around lines 817 - 820, onSDDownloadModelClicked() disables m_sdDownloadButton and sets m_downloadStatusLabel but does not re-enable the button on cancel/error; update the download completion/error/cancel handlers (the slots connected to ModelDownloader::instance()->startDownload, e.g. the callbacks handling success, failure and cancellation) to always re-enable m_sdDownloadButton and update m_downloadStatusLabel appropriately (error or cancelled message) so the UI is restored on all paths; apply the same fix to the other similar callers/handlers referenced around the other ranges (the same button/state members and their download-related slots).src/SDWorker_test.cpp (1)
123-130:⚠️ Potential issue | 🟡 MinorUse a temp path and wait for
generationErrorinGenerateWithoutModel.This test is still brittle: it hardcodes a Unix path and asserts synchronously right after
processEvents(). Prefer a Qt temp path +QSignalSpy::wait()/QTRY_COMPAREfor stability.Suggested hardening
TEST_F(SDWorkerTest, GenerateWithoutModel) { SDWorker worker; QSignalSpy errorSpy(&worker, &SDWorker::generationError); - worker.generateTexture("test prompt", "/tmp/test.png"); - QCoreApplication::processEvents(); - ASSERT_EQ(errorSpy.count(), 1); + const QString outPath = QDir::temp().filePath("sdworker_test.png"); + worker.generateTexture("test prompt", outPath); + if (errorSpy.count() == 0) { + ASSERT_TRUE(errorSpy.wait(1000)); + } + ASSERT_EQ(errorSpy.count(), 1); EXPECT_TRUE(errorSpy.first().first().toString().contains("No SD model loaded")); }Based on learnings: "Applies to src/**/*_test.cpp : Tests must work under Xvfb (headless X11) — avoid assumptions about a real display".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SDWorker_test.cpp` around lines 123 - 130, The test SDWorkerTest::GenerateWithoutModel is brittle: it uses a hardcoded Unix path ("/tmp/test.png") and checks the generationError signal synchronously; replace the hardcoded path with a cross-platform temporary path (e.g., use QTemporaryFile or QDir::tempPath() to build a temp filename) and replace the immediate ASSERT_EQ(errorSpy.count(), 1) / EXPECT_TRUE(...) after QCoreApplication::processEvents() with a proper wait-based assertion (use QSignalSpy::wait(...) or QTRY_COMPARE on errorSpy.count()) to wait for the generationError signal emitted by SDWorker::generationError after calling SDWorker::generateTexture.src/SDWorker.cpp (1)
152-155:⚠️ Potential issue | 🟠 MajorStop is still non-preemptive: generation only stops after
generate_image()returns.
requestStop()sets a flag, but no in-flight interruption is wired into the generation call path, so stop/unload/shutdown can remain blocked until full inference ends.#!/bin/bash # Verify whether stable-diffusion API in repo exposes cancel/interrupt primitives, # and where SDWorker currently checks stop state. headers=$(fd -HI 'stable-diffusion*.h' .) echo "$headers" if [ -n "$headers" ]; then rg -n 'cancel|abort|stop|interrupt|progress|generate_image|set_progress' $headers fi rg -n 'requestStop|m_stopRequested|progressCallback|generate_image' src/SDWorker.cppAlso applies to: 229-233, 354-358, 398-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SDWorker.cpp` around lines 152 - 155, requestStop() currently only flips m_stopRequested but isn’t wired into the image generation path, so generation blocks until generate_image() returns; update SDWorker to propagate the stop request into the inference call path by either (A) calling the underlying SDK’s cancel/interrupt primitive from requestStop() if available, or (B) passing a progressCallback/lambda into generate_image() (or into the loop inside SDWorker’s generate/infer methods) that checks m_stopRequested and returns a cancel signal to abort generation; change the call sites that invoke generate_image() (and any loops in SDWorker methods mentioned: the areas around the generate_image invocations and progress handling) to honor the callback return or cancellation API so shutdown/unload becomes preemptive.
🤖 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/SDManager.cpp`:
- Around line 383-387: The filename extension logic currently preserves .jpg but
SDWorker always writes PNG bytes, so update the handling to always normalize to
".png": in the block using fileName (the endsWith/replace logic that currently
checks ".png" and ".jpg") remove the special-case for ".jpg" and always strip
any existing extension (the QRegularExpression(R"(\.\w+$)")) and append ".png";
apply the same change to the other identical block referenced (the one around
lines 427-431) so all saved files consistently get a .png extension even if the
original name was .jpg.
---
Duplicate comments:
In `@src/LLMSettingsWidget.cpp`:
- Around line 70-79: The new SD UI was added as QWidget tabs under the
ENABLE_STABLE_DIFFUSION block (creating sdModelsTab/sdSettingsTab, calling
setupSDModelsTab/setupSDSettingsTab, and adding them to m_tabWidget), but per
guidelines this must be implemented in QML; remove or revert adding
QWidget-based SD tabs and instead wire the existing QML settings surface to
expose the SD Models and SD Settings pages (e.g., remove creation/registration
of sdModelsTab/sdSettingsTab and the calls to
setupSDModelsTab/setupSDSettingsTab and m_tabWidget->addTab) and implement the
SD UI as QML components accessible from the QML settings view or by invoking the
QML settings page from C++ when ENABLE_STABLE_DIFFUSION is defined.
- Around line 817-820: onSDDownloadModelClicked() disables m_sdDownloadButton
and sets m_downloadStatusLabel but does not re-enable the button on
cancel/error; update the download completion/error/cancel handlers (the slots
connected to ModelDownloader::instance()->startDownload, e.g. the callbacks
handling success, failure and cancellation) to always re-enable
m_sdDownloadButton and update m_downloadStatusLabel appropriately (error or
cancelled message) so the UI is restored on all paths; apply the same fix to the
other similar callers/handlers referenced around the other ranges (the same
button/state members and their download-related slots).
In `@src/MaterialEditorQML.cpp`:
- Around line 22-24: The TU uses QRegularExpression (and possibly
QRegularExpressionMatchIterator) without a direct include, so add an explicit
`#include` <QRegularExpression> at the top of MaterialEditorQML.cpp (near the
other Qt includes) to avoid relying on transitive includes; ensure the include
is added alongside `#include` <QUrl>, `#include` <QFile>, `#include` <QPalette> so
references to QRegularExpression compile independently.
- Around line 85-93: MaterialEditorQML is not listening for
SDManager::generationStopped, so sdIsGenerating can miss true stop events; add a
connection for SDManager::generationStopped in the same block where other
signals are connected (SDManager::instance()) and implement or wire it to a
handler (e.g., onSDGenerationStopped) that mirrors onSDGenerationCompleted
behavior (clear generating state, update progress/UI) instead of relying on
stopTextureGeneration() to emit the change; update any duplicate connection
blocks (e.g., the one around lines 2948-2953) to include the same
generationStopped -> onSDGenerationStopped mapping.
In `@src/SDWorker_test.cpp`:
- Around line 123-130: The test SDWorkerTest::GenerateWithoutModel is brittle:
it uses a hardcoded Unix path ("/tmp/test.png") and checks the generationError
signal synchronously; replace the hardcoded path with a cross-platform temporary
path (e.g., use QTemporaryFile or QDir::tempPath() to build a temp filename) and
replace the immediate ASSERT_EQ(errorSpy.count(), 1) / EXPECT_TRUE(...) after
QCoreApplication::processEvents() with a proper wait-based assertion (use
QSignalSpy::wait(...) or QTRY_COMPARE on errorSpy.count()) to wait for the
generationError signal emitted by SDWorker::generationError after calling
SDWorker::generateTexture.
In `@src/SDWorker.cpp`:
- Around line 152-155: requestStop() currently only flips m_stopRequested but
isn’t wired into the image generation path, so generation blocks until
generate_image() returns; update SDWorker to propagate the stop request into the
inference call path by either (A) calling the underlying SDK’s cancel/interrupt
primitive from requestStop() if available, or (B) passing a
progressCallback/lambda into generate_image() (or into the loop inside
SDWorker’s generate/infer methods) that checks m_stopRequested and returns a
cancel signal to abort generation; change the call sites that invoke
generate_image() (and any loops in SDWorker methods mentioned: the areas around
the generate_image invocations and progress handling) to honor the callback
return or cancellation API so shutdown/unload becomes preemptive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dc26d9e-265a-40d4-a89c-9255cc1a9a75
📒 Files selected for processing (9)
CLAUDE.mdsrc/LLMSettingsWidget.cppsrc/LLMSettingsWidget.hsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/SDManager.cppsrc/SDWorker.cppsrc/SDWorker_test.cppsrc/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/LLMSettingsWidget.h
Comment on lines
+383
to
+387
| if (!fileName.endsWith(".png", Qt::CaseInsensitive) && | ||
| !fileName.endsWith(".jpg", Qt::CaseInsensitive)) { | ||
| fileName.replace(QRegularExpression(R"(\.\w+$)"), ""); | ||
| fileName += ".png"; | ||
| } |
There was a problem hiding this comment.
Filename extension handling is inconsistent with actual output format.
Here .jpg names are preserved, but SDWorker always writes PNG bytes. That can leave JPEG-named files containing PNG data, which may fail in consumers that trust extension.
Suggested fix (normalize to PNG consistently)
- if (!fileName.endsWith(".png", Qt::CaseInsensitive) &&
- !fileName.endsWith(".jpg", Qt::CaseInsensitive)) {
- fileName.replace(QRegularExpression(R"(\.\w+$)"), "");
- fileName += ".png";
- }
+ fileName.replace(QRegularExpression(R"(\.\w+$)"), "");
+ fileName += ".png";Also applies to: 427-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SDManager.cpp` around lines 383 - 387, The filename extension logic
currently preserves .jpg but SDWorker always writes PNG bytes, so update the
handling to always normalize to ".png": in the block using fileName (the
endsWith/replace logic that currently checks ".png" and ".jpg") remove the
special-case for ".jpg" and always strip any existing extension (the
QRegularExpression(R"(\.\w+$)")) and append ".png"; apply the same change to the
other identical block referenced (the one around lines 427-431) so all saved
files consistently get a .png extension even if the original name was .jpg.
Wrap entire SDWorker.cpp and SDManager.cpp files with LCOV_EXCL since SD is not enabled in CI (requires GPU + model files). Also mark SD getters, texture generation, img2img editing, and LLM->SD auto-trigger sections in MaterialEditorQML.cpp as excluded from coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/SDWorker.cpp (1)
22-27: Busy-wait timeout may be insufficient for long generations.The loops wait a maximum of 1 second (100 × 10ms) for
m_isGeneratingto clear. Ifgenerate_image()takes longer (especially for high step counts or large images), these loops may timeout while generation is still in progress, potentially leading to attempts to freem_ctxwhile it's still in use.Consider increasing the timeout or using a condition variable to wait indefinitely until generation completes.
Also applies to: 107-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SDWorker.cpp` around lines 22 - 27, The current busy-wait in the stop path uses m_isGenerating.load() with a fixed 100×10ms loop after setting m_stopRequested, which can prematurely proceed while generate_image() (and accesses to m_ctx) are still running; replace the spin-wait with a proper synchronization primitive (e.g., std::condition_variable or QWaitCondition paired with a QMutex) so generate_image() signals completion by clearing m_isGenerating and notifying the condition variable, and the stop path waits on that condition (or, if you must keep a timeout, significantly increase it and loop with notification) — update both occurrences that use m_isGenerating/m_stopRequested (the block at the top and the block around lines 107-112) and ensure generate_image() notifies on completion before any code frees or accesses m_ctx.src/MaterialEditorQML.cpp (1)
1668-1673: Consider centralizing the generated textures path construction.The pattern
QStandardPaths::writableLocation(QStandardPaths::AppDataLocation)+"generated_textures"is duplicated here and also inSDManager::generateOutputPath()(see context snippet 1 at src/SDManager.cpp:344-355). If the base directory logic changes in one place, the others may diverge.Consider exposing a static helper (e.g.,
SDManager::getGeneratedTexturesDir()) or a shared utility to avoid duplication.Also applies to: 2794-2795, 2918-2919
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MaterialEditorQML.cpp` around lines 1668 - 1673, The generated-textures path construction is duplicated; add a single shared helper (e.g., a static SDManager::getGeneratedTexturesDir() or common utility) that returns QDir(QStandardPaths::writableLocation(QStandardPaths::AppDataLocation)).filePath("generated_textures"), then replace the ad-hoc constructions in MaterialEditorQML.cpp (the block using genTexPath/texName), SDManager::generateOutputPath(), and the other occurrences (around lines referenced) to call that helper so all code uses the same centralized path provider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 1668-1673: The generated-textures path construction is duplicated;
add a single shared helper (e.g., a static SDManager::getGeneratedTexturesDir()
or common utility) that returns
QDir(QStandardPaths::writableLocation(QStandardPaths::AppDataLocation)).filePath("generated_textures"),
then replace the ad-hoc constructions in MaterialEditorQML.cpp (the block using
genTexPath/texName), SDManager::generateOutputPath(), and the other occurrences
(around lines referenced) to call that helper so all code uses the same
centralized path provider.
In `@src/SDWorker.cpp`:
- Around line 22-27: The current busy-wait in the stop path uses
m_isGenerating.load() with a fixed 100×10ms loop after setting m_stopRequested,
which can prematurely proceed while generate_image() (and accesses to m_ctx) are
still running; replace the spin-wait with a proper synchronization primitive
(e.g., std::condition_variable or QWaitCondition paired with a QMutex) so
generate_image() signals completion by clearing m_isGenerating and notifying the
condition variable, and the stop path waits on that condition (or, if you must
keep a timeout, significantly increase it and loop with notification) — update
both occurrences that use m_isGenerating/m_stopRequested (the block at the top
and the block around lines 107-112) and ensure generate_image() notifies on
completion before any code frees or accesses m_ctx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e20cabbb-d546-4f1a-ae06-40fce924b006
📒 Files selected for processing (3)
src/MaterialEditorQML.cppsrc/SDManager.cppsrc/SDWorker.cpp
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
-DENABLE_STABLE_DIFFUSION=ON)New Files
src/SDWorker.h/cpp— Worker thread wrapping stable-diffusion.cpp C APIsrc/SDManager.h/cpp— QML_SINGLETON for model/settings/generation managementsrc/SDWorker_test.cpp— Unit tests (11 tests)src/SDManager_test.cpp— Unit tests (14 tests)Test plan
-DENABLE_STABLE_DIFFUSION=ON— compiles and runs-DENABLE_STABLE_DIFFUSION=OFF— compiles, no SD code includedSDWorker*+SDManager*tests pass (33 tests total with LLMSettingsWidget)LLMSettingsWidget*tests updated for renamed tabs and SD tab count🤖 Generated with Claude Code
Summary by CodeRabbit