feat: Real-ESRGAN texture upscaling (ONNX) (#405) - #749
Merged
Conversation
ONNX-backed 2x/4x super-resolution, reusing the #404 ONNX infra (ENABLE_ONNX, ModelDownloader, NCHW conversion). - TextureUpscaler (Ogre-free, like PbrMapSynth): scale-aware overlapping-tile upscale that composites results in OUTPUT space with a feathered seam blend. Detects the scale factor from the model's output/input ratio at runtime (validates the output tensor element count before copying). Reuses PbrMapSynth::toNCHW / nchwToRgb. - AIAssistManager: Map enum extended with UpscaleX2/UpscaleX4 (BSD-3 Real-ESRGAN models, downloaded on first use from the same HF repo); upscaleTexture(srcPath, scale, overwrite) ensures+runs+caches, writes <stem>_upscaled.png, emits upscaleStarted/Completed/Error. Sentry breadcrumb ai.assist.upscale. - CLI: `qtmesh material --texture low.png --upscale {2|4} [-o high.png]` (cmdMaterialUpscale, delegates to the facade). ENABLE_ONNX-guarded. Model: Real-ESRGAN x4plus / x2plus (BSD-3-Clause, xinntao), exported to ONNX and hosted at fernandotonon/QtMeshEditor-models. Verified end-to-end: 256x256 → 1024x1024 (4x) and 128 → 256 (2x), model auto-downloaded from HF; bad factor / missing texture rejected with usage errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- MCP: upscale_texture tool (texture_path, scale 2/4, overwrite) → AIAssistManager. - GUI: "Upscale 2× / 4×" buttons in the Material Editor Texture Properties panel (ONNX-only), via MaterialEditorQML::upscaleCurrentTexture which relays the facade's upscale signals. - Tests: CLIPipeline upscale coverage (missing texture / bad factor / non-numeric / no-model-fails-clean, offline-guarded) + TextureUpscaler error-contract tests. - scripts/export-realesrgan-onnx.py: one-time offline .pth→ONNX exporter (BSD-3 Real-ESRGAN x4plus/x2plus, pinned release assets). NOT shipped. - docs: CLAUDE.md CLI line + architecture entry; refreshed the #404 hosting note (models are now hosted on HF) + the QTMESH_PBR_NO_DOWNLOAD offline guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Real-ESRGAN-based 2×/4× texture super-resolution as a new ChangesReal-ESRGAN Texture Upscaling Feature
Sequence Diagram(s)sequenceDiagram
rect rgba(135, 206, 235, 0.5)
Note over TexturePropertiesPanel,WorkerThread: GUI-initiated upscale flow
end
participant TexturePropertiesPanel
participant MaterialEditorQML
participant AIAssistManager
participant WorkerThread as WorkerThread (TextureUpscaler)
TexturePropertiesPanel->>MaterialEditorQML: upscaleCurrentTexture(2 or 4)
MaterialEditorQML->>MaterialEditorQML: resolve file:// URL, validate on-disk path
MaterialEditorQML->>AIAssistManager: ensureUpscaleModel(scale)
MaterialEditorQML-->>TexturePropertiesPanel: upscaleStarted / upscaleDownloading
MaterialEditorQML->>WorkerThread: detach upscale(srcPath, modelPath, opts, ProgressFn)
WorkerThread-->>MaterialEditorQML: invokeMethod upscaleProgress(done, total)
MaterialEditorQML-->>TexturePropertiesPanel: onUpscaleProgress → pbrStatus update
alt success
WorkerThread-->>MaterialEditorQML: invokeMethod upscaleCompleted(outPath)
MaterialEditorQML-->>TexturePropertiesPanel: onUpscaleCompleted → pbrStatus = "Upscaled → <stem>"
else error or cancelled
WorkerThread-->>MaterialEditorQML: invokeMethod upscaleError(err)
MaterialEditorQML-->>TexturePropertiesPanel: onUpscaleError → pbrStatus = "Upscale: <err>"
end
sequenceDiagram
rect rgba(144, 238, 144, 0.5)
Note over CLIUser,TextureUpscaler: CLI and MCP upscale flows
end
participant CLIUser as CLI User
participant CLIPipeline
participant MCPClient as MCP Client
participant MCPServer
participant AIAssistManager
participant TextureUpscaler
CLIUser->>CLIPipeline: qtmesh material --texture low.png --upscale 4 -o high.png
CLIPipeline->>CLIPipeline: parse --upscale, validate factor ∈ {2,4}
CLIPipeline->>AIAssistManager: upscaleTexture(srcPath, scale, overwrite=true)
AIAssistManager->>TextureUpscaler: upscale(image, modelPath, opts)
TextureUpscaler-->>AIAssistManager: Result{ok, image, scale}
AIAssistManager->>AIAssistManager: save PNG, emit upscaleCompleted
AIAssistManager-->>CLIPipeline: output path
CLIPipeline->>CLIPipeline: rename to -o path if differs
CLIPipeline-->>CLIUser: print source/output dimensions
MCPClient->>MCPServer: tools/call upscale_texture {texture_path, scale, overwrite}
MCPServer->>AIAssistManager: upscaleTexture(texture_path, scale, overwrite)
AIAssistManager-->>MCPServer: outputPath
MCPServer-->>MCPClient: {outputPath, scale}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9136174f0a
ℹ️ 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
+324
to
+326
| const QString outPath = QDir(fi.absolutePath()) | ||
| .filePath(fi.completeBaseName() + QStringLiteral("_upscaled.png")); | ||
| if (!overwrite && QFileInfo::exists(outPath)) { // cache: skip re-upscale |
There was a problem hiding this comment.
When callers use the default overwrite=false path (for example the MCP tool), both 2× and 4× requests map to the same <stem>_upscaled.png cache file, so a later request for the other scale returns the stale image and emits success without running the requested model. This makes the reported scale/output wrong whenever users try both factors for the same source texture; use scale-specific names or validate the cached image dimensions before reusing it.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in de468f2 — the output is now <stem>_upscaled_x{2,4}.png, so a cached 2× result can't be returned for a 4× request (and vice versa).
Comment on lines
+4208
to
+4210
| QFile::remove(outputPath); | ||
| if (QFile::rename(produced, outputPath)) | ||
| finalPath = outputPath; |
There was a problem hiding this comment.
-o cannot be written
If the explicit -o destination is invalid or unwritable, QFile::rename(produced, outputPath) returns false but the command still exits 0 and leaves the image at the temporary <stem>_upscaled.png path. In that scenario automation sees a successful qtmesh material --texture ... --upscale ... -o <path> even though the requested output file was never created, so this branch should report the rename failure and return nonzero.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in de468f2 — the CLI now rename→copy-falls-back for -o, and returns exit 1 if the requested output can't be produced (was silently exit 0).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 252: The documentation in CLAUDE.md line 252 states that the model base
URL has an "empty default until the exported .onnx files are hosted", but the
actual implementation in src/AIAssistManager.cpp (lines 107-119) defines and
uses a built-in kDefaultModelBaseUrl constant as the fallback. Update the
CLAUDE.md documentation to accurately reflect that the runtime has a built-in
default URL (kDefaultModelBaseUrl pointing to the Hugging Face repository)
rather than an empty default, to avoid misleading users about offline/self-host
setup behavior.
In `@scripts/export-realesrgan-onnx.py`:
- Around line 32-35: Add SHA-256 integrity verification for downloaded model
files before deserialization to prevent execution of potentially compromised
weights. Modify the download function to compute the SHA-256 hash of the
downloaded file and compare it against a pinned hash value stored in the script.
Before calling ModelLoader().load_from_file() on line 43, verify that the hash
check passes and raise an exception if verification fails. Define the expected
SHA-256 hashes for each model variant as constants near the top of the script
alongside other configuration values.
In `@src/AIAssistManager.cpp`:
- Around line 338-345: The synchronous model download via ensureModelBlocking()
and texture upscaling via TextureUpscaler::upscale() in the upscaling code path
can freeze the QML/UI thread for up to 120+ seconds. Refactor by keeping the
current blocking implementation as a helper method for CLI/MCP usage, then
create a new async worker thread path that performs the model download and
upscaling operations asynchronously and emits the existing lifecycle signals
upon completion. Route the QML-facing call from
MaterialEditorQML::upscaleCurrentTexture through the new async worker path
instead of calling the blocking implementation directly, ensuring the UI remains
responsive during the download and inference operations.
- Around line 324-328: The cache file path construction at the outPath variable
does not include the upscaling scale factor, causing cached results from 2×
upscaling to be incorrectly reused for 4× requests when overwrite is false.
Modify the outPath construction to incorporate the scale factor into the
filename (for example, by appending the scale value like _2x or _4x to the
completeBaseName before adding _upscaled.png), or alternatively validate that
any cached file has dimensions matching the requested scale before reusing it.
This ensures the cache key is unique per scale level and prevents serving
incorrect resolution results.
In `@src/CLIPipeline.cpp`:
- Around line 4206-4211: The code in CLIPipeline.cpp currently silently ignores
when QFile::rename(produced, outputPath) fails (which can happen during
cross-device moves), but still exits successfully, violating the contract of the
-o flag. When the rename operation fails, you need to add error handling that
ensures the command exits with a non-zero status or propagates an error to
indicate the failure to move the file to the requested output path specified by
the -o option. Check the return value of QFile::rename() and if it returns
false, log an appropriate error message and ensure the CLIPipeline properly
indicates failure rather than continuing as if the operation succeeded.
In `@src/TextureUpscaler.cpp`:
- Line 98: The texture upscaling process is losing alpha channel information
because the input is converted to Format_RGB888 at line 98 and the output at
line 181 is RGB-only, making any transparent or semi-transparent textures become
fully opaque. Before converting srcIn to Format_RGB888, extract and preserve the
alpha channel from the original image. After the upscaling is complete and
r.image is built at line 181, reapply the original alpha channel to the result
image before returning it, ensuring that cutout and opacity textures maintain
their transparency information through the upscaling process.
- Around line 129-131: The scale factor calculation at line 129 only validates
the width dimension using t.ow and probe through integer division, but the code
later assumes consistent scaling on both axes (tw * scale by th * scale). You
must also calculate and validate the scale factor for the height dimension
(t.oh) and ensure both width and height scale factors are equal. If the height
scale differs from the width scale or is invalid, set r.error with an
appropriate message and return early, similar to the existing check for r.scale
< 1.
- Around line 133-135: Add guard checks before the memory allocations for the
acc and weight vectors in the TextureUpscaler code. The outW and outH
calculations can overflow when multiplying by r.scale, and the subsequent
allocations of acc and weight vectors can attempt to allocate multi-gigabyte
buffers. Insert validation logic before the vector allocations to check if the
computed oplane size (outW times outH) exceeds a safe threshold, and throw a
controlled exception or return early if it does, preventing std::bad_alloc from
bypassing the Ort::Exception handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58b4d4de-1135-480b-88dc-a48f09ce9438
📒 Files selected for processing (17)
CLAUDE.mdqml/TexturePropertiesPanel.qmlscripts/export-realesrgan-onnx.pysrc/AIAssistManager.cppsrc/AIAssistManager.hsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_cmdmaterial_coverage_test.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/TextureUpscaler.cppsrc/TextureUpscaler.hsrc/TextureUpscaler_test.cpptests/CMakeLists.txt
… loads (#405) scan-assets-qtmesh failed: the Linux .deb/Docker binary aborts with "libonnxruntime.so.1: cannot open shared object file". Root cause (latent since #404 turned ENABLE_ONNX on for the Linux release): the POST_BUILD copied only the single RESOLVED versioned lib (libonnxruntime.so.1.20.1) next to the binary, but the loader requests the SONAME (libonnxruntime.so.1), which wasn't shipped — so `./bin/*.so*` packaging never included a name the binary could load. - OnnxRuntime.cmake exposes QTMESH_ONNX_LIB_DIR. - The app + UnitTests POST_BUILD now glob-copy every libonnxruntime.so* / .dylib / .dll (versioned file + SONAME symlinks) next to the binary, so the packaged .deb/Docker image resolves libonnxruntime.so.1 at runtime. Verified on macOS: both libonnxruntime.1.20.1.dylib and libonnxruntime.dylib are now copied next to the binary (previously only one). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/CMakeLists.txt (3)
596-599: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffNote:
copy_if_differentdereferences symlinks, creating duplicate files.The command
${CMAKE_COMMAND} -E copy_if_differentcopies the content of symlink targets rather than preserving the symlinks themselves. On Linux, if the source directory containslibonnxruntime.so -> libonnxruntime.so.1 -> libonnxruntime.so.1.15.0, the build will create three separate files with identical content in the output directory, tripling the disk footprint.This approach works correctly at runtime (the dynamic linker finds
libonnxruntime.so.1as a regular file), so it's not a functional defect. If you want to preserve symlinks to save space and match the source layout more closely, consider usingfile(INSTALL ...)withFOLLOW_SYMLINK_CHAINin a CMake script invoked viaadd_custom_command, or platform-specific commands likecp -Pon Unix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CMakeLists.txt` around lines 596 - 599, The add_custom_command in the POST_BUILD step uses copy_if_different which dereferences symlinks, causing duplicate files with identical content to be created in the output directory. Replace the copy_if_different approach with either a file(INSTALL ...) command in a CMake script invoked through add_custom_command with the FOLLOW_SYMLINK_CHAIN option to preserve the symlink structure, or use platform-specific commands like cp -P on Unix platforms that maintain symlinks instead of dereferencing them. This will preserve the symlink chain from the source directory and avoid creating redundant copies of the ONNX Runtime library.
591-600: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider validating that the glob found libraries.
If
QTMESH_ONNX_LIB_DIRis set but contains no matching runtime files, theforeachloop silently does nothing and the application will fail at runtime with missing library errors. Adding a validation check would fail fast during build rather than at runtime.🛡️ Suggested defensive check
file(GLOB _ort_runtime_libs "${QTMESH_ONNX_LIB_DIR}/libonnxruntime.so*" "${QTMESH_ONNX_LIB_DIR}/libonnxruntime*.dylib" "${QTMESH_ONNX_LIB_DIR}/onnxruntime.dll") + if(NOT _ort_runtime_libs) + message(FATAL_ERROR "ONNX Runtime libraries not found in ${QTMESH_ONNX_LIB_DIR}") + endif() foreach(_ortlib ${_ort_runtime_libs})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CMakeLists.txt` around lines 591 - 600, The glob pattern used to find ONNX runtime libraries with file(GLOB _ort_runtime_libs ...) may result in an empty list if QTMESH_ONNX_LIB_DIR doesn't contain matching files, allowing the build to succeed silently while the application fails at runtime. Add a validation check immediately after the file(GLOB ...) command and before the foreach(_ortlib ${_ort_runtime_libs}) loop to verify that _ort_runtime_libs is not empty, and emit a CMake warning or error message if no matching libraries are found.
709-718: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSame validation and symlink concerns as the app target.
This block has the same two considerations as Lines 591-600:
- An empty glob would silently skip copying libraries, leading to runtime failure. Consider adding validation after the
file(GLOB ...)call.copy_if_differentdereferences symlinks, creating duplicate files on Unix platforms (functional but space-inefficient).See the earlier comments on Lines 591-600 for detailed analysis and suggested fixes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CMakeLists.txt` around lines 709 - 718, The file(GLOB) call for _ort_test_libs in the UnitTests target post-build section has the same two issues as the earlier app target: first, add validation after the file(GLOB _ort_test_libs ...) call to ensure the glob returned results and handle the empty case appropriately to prevent silent failures during testing; second, replace the copy_if_different command with a method that preserves symlinks rather than dereferencing them to avoid creating duplicate library files on Unix platforms. Apply the same validation and symlink-aware copying approach that was used in the earlier target block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/CMakeLists.txt`:
- Around line 596-599: The add_custom_command in the POST_BUILD step uses
copy_if_different which dereferences symlinks, causing duplicate files with
identical content to be created in the output directory. Replace the
copy_if_different approach with either a file(INSTALL ...) command in a CMake
script invoked through add_custom_command with the FOLLOW_SYMLINK_CHAIN option
to preserve the symlink structure, or use platform-specific commands like cp -P
on Unix platforms that maintain symlinks instead of dereferencing them. This
will preserve the symlink chain from the source directory and avoid creating
redundant copies of the ONNX Runtime library.
- Around line 591-600: The glob pattern used to find ONNX runtime libraries with
file(GLOB _ort_runtime_libs ...) may result in an empty list if
QTMESH_ONNX_LIB_DIR doesn't contain matching files, allowing the build to
succeed silently while the application fails at runtime. Add a validation check
immediately after the file(GLOB ...) command and before the foreach(_ortlib
${_ort_runtime_libs}) loop to verify that _ort_runtime_libs is not empty, and
emit a CMake warning or error message if no matching libraries are found.
- Around line 709-718: The file(GLOB) call for _ort_test_libs in the UnitTests
target post-build section has the same two issues as the earlier app target:
first, add validation after the file(GLOB _ort_test_libs ...) call to ensure the
glob returned results and handle the empty case appropriately to prevent silent
failures during testing; second, replace the copy_if_different command with a
method that preserves symlinks rather than dereferencing them to avoid creating
duplicate library files on Unix platforms. Apply the same validation and
symlink-aware copying approach that was used in the earlier target block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fcb1753-e088-47b0-8523-f98af881963e
📒 Files selected for processing (2)
cmake/OnnxRuntime.cmakesrc/CMakeLists.txt
…chain
CodeRabbit findings on the Real-ESRGAN work:
Major:
- Scale-aware cache: output is now <stem>_upscaled_x{2,4}.png so a cached 2×
result is never returned for a 4× request (and vice versa).
- GUI no longer freezes: upscaleCurrentTexture ensures the model on the GUI
thread (download needs an event loop), then runs the pure-CPU tiled inference
on a std::thread and marshals the result back via a queued invocation.
Added AIAssistManager::ensureUpscaleModel for the main-thread ensure step.
(CLI/MCP keep the synchronous path.)
- Preserve source alpha: cutout/opacity textures keep their alpha (upscaled
nearest-to-match and reapplied) instead of coming back fully opaque.
- Guard the output-canvas allocation: compute size in 64-bit, cap at 256 Mpx,
and catch std::bad_alloc so a huge input / bad scale fails cleanly instead of
overflowing int or terminating outside the Ort handler.
- CLI honors -o strictly: rename→copy fallback (cross-device), and return exit 1
if the requested output file can't be produced (was silently exit 0).
- Export scripts verify SHA-256 of each .pth before deserializing (.pth is a
code-execution boundary) — both realesrgan + pbrify scripts.
Minor:
- Validate the detected scale is a uniform integer factor on BOTH axes, and that
each tile returns exactly tw*scale × th*scale (else fail, not silent crop).
- CLAUDE.md: correct the stale "empty default" model-base-URL wording (now the
hosted HF repo).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Addressed all review feedback in de468f2 (+ synced master for the 3.8.1 packaging fixes so scan-assets uses a working .deb): Major
Minor
Replied inline on each thread. |
The GUI gave no feedback during an upscale (a 2h run looked hung), and CPU inference was pinned to a single core. - TextureUpscaler: optional ProgressFn(done,total) reported per tile; returning false cancels (error="cancelled"). SetIntraOpNumThreads now uses hardware_concurrency-1 instead of 1 — a 256→1024 4× drops from ~2 min to ~7.5s (~7 cores). (Scoped to upscaling; PbrMapSynth stays single-threaded — its maps are small/fast.) - MaterialEditorQML: upscaleCurrentTexture runs on a worker, emits upscaleDownloading (first-run model fetch), upscaleProgress (per tile), and upscaleCompleted/Error; cancelUpscale() sets a shared atomic the worker's progress callback checks. Model-ensure stays on the GUI thread (download needs an event loop). - QML: status shows "Downloading upscale model…" / "Upscaling… tile X/Y", the scale buttons disable mid-run, and a Cancel button is shown. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MaterialEditorQML.cpp (1)
4242-4247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBreadcrumb the upscale and cancel UI actions.
generatePbrFromDiffuse()recordsui.action, but the new QML-triggered upscale and cancel actions do not. Add breadcrumbs without full file paths. As per coding guidelines, “All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks…”🛠️ Proposed fix
void MaterialEditorQML::upscaleCurrentTexture(int scale) { + SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), + QStringLiteral("Upscale current texture %1x").arg(scale)); `#ifndef` ENABLE_ONNX Q_UNUSED(scale); emit upscaleError(tr("AI upscaling is not enabled. Rebuild with ENABLE_ONNX=ON."));void MaterialEditorQML::cancelUpscale() { + SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), + QStringLiteral("Cancel texture upscale")); if (m_upscaleCancel) m_upscaleCancel->store(true); }Also applies to: 4306-4309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MaterialEditorQML.cpp` around lines 4242 - 4247, The upscaleCurrentTexture() function and the cancel action handler are missing breadcrumb tracking that is required per coding guidelines. Add SentryReporter::addBreadcrumb() calls to both the upscale action (in upscaleCurrentTexture()) and the cancel action (at lines 4306-4309) using the 'ui.action' category with descriptive messages that indicate which action was triggered. Follow the same pattern used in generatePbrFromDiffuse() for consistency.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 252-253: Update the documentation in the Real-ESRGAN texture
upscaling section to reflect the actual scale-specific cache naming convention.
Locate the phrase "caches `<stem>_upscaled.png` next to the source" in the text
describing AIAssistManager::upscaleTexture and change it to clarify that the
cached output filename includes the scale factor: `<stem>_upscaled_x2.png` for
2× upscaling and `<stem>_upscaled_x4.png` for 4× upscaling, ensuring the
documentation accurately reflects the implementation behavior for cache reuse
and overwrite handling.
In `@src/MaterialEditorQML.cpp`:
- Around line 4255-4277: The cancel flag m_upscaleCancel is created too late in
the upscaling workflow. It is currently assigned after the ensureUpscaleModel
method returns, but the Cancel button becomes available to the user as soon as
upscaling starts, meaning a user clicking Cancel during the model download phase
will have their action ignored since m_upscaleCancel doesn't exist yet. Move the
line that creates m_upscaleCancel (the std::make_shared assignment) to the
beginning of the function, before any calls to ensureUpscaleModel, so that the
cancel flag is ready to receive cancellation signals from the moment the UI
becomes interactive.
- Around line 4271-4294: The upscale operation always recomputes and overwrites
the output file at outPath, ignoring any cached result from a previous run with
the same scale factor. Before launching the worker thread with the std::thread
lambda that calls TextureUpscaler::upscale, check if the file already exists at
outPath. If the cached upscaled image exists, skip the expensive inference and
worker launch by either returning early or directly emitting the completion
signal with the cached file path. This preserves the cache behavior and avoids
redundant CPU-intensive computation on repeated requests with the same scale.
In `@src/MaterialEditorQML.h`:
- Around line 577-580: The documentation comment for the upscale function (the
comment block at lines 577-580 in MaterialEditorQML.h) states that the output
file is written as `_upscaled.png`, but the actual implementation uses
`_upscaled_x%1.png` (where %1 is the scale factor). Update the comment to
reflect the correct filename pattern `_upscaled_x%1.png` so that callers know
which file to expect.
In `@src/TextureUpscaler.cpp`:
- Around line 89-90: The upscale function (starting at the function signature
with parameters srcIn, modelPath, opts, and onProgress) is missing the required
Sentry breadcrumb emission. First, ensure SentryReporter.h is included at the
top of the file with other local includes if it is not already present. Then, at
the beginning of the upscale function body, add a breadcrumb emission with
category 'ai.assist.upscale' to track this AI-assisted operation. When
constructing the breadcrumb details, avoid including the full file path from
modelPath to keep logs clean and avoid exposing system paths.
- Around line 258-265: The code does not validate the results of scaled() and
convertToFormat() before dereferencing them with constScanLine() and scanLine(),
which can cause null pointer dereferences under memory pressure. Additionally,
Qt::SmoothTransformation blurs the alpha mask inappropriately when
nearest-neighbor scaling is intended. Add null checks for both the upAlpha
QImage returned from srcAlpha.scaled() and the rgba QImage returned from
rgb.convertToFormat() before accessing their scanlines, and replace
Qt::SmoothTransformation with Qt::FastTransformation when scaling srcAlpha to
preserve the alpha channel with nearest-neighbor filtering instead of smoothing.
---
Outside diff comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 4242-4247: The upscaleCurrentTexture() function and the cancel
action handler are missing breadcrumb tracking that is required per coding
guidelines. Add SentryReporter::addBreadcrumb() calls to both the upscale action
(in upscaleCurrentTexture()) and the cancel action (at lines 4306-4309) using
the 'ui.action' category with descriptive messages that indicate which action
was triggered. Follow the same pattern used in generatePbrFromDiffuse() for
consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bbaf8e2-76e2-4f65-be2a-8c2ac30ae5df
📒 Files selected for processing (11)
CLAUDE.mdqml/TexturePropertiesPanel.qmlscripts/export-pbrify-onnx.pyscripts/export-realesrgan-onnx.pysrc/AIAssistManager.cppsrc/AIAssistManager.hsrc/CLIPipeline.cppsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/TextureUpscaler.cppsrc/TextureUpscaler.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/AIAssistManager.h
- src/AIAssistManager.cpp
- src/CLIPipeline.cpp
Comment on lines
+252
to
+253
| - **PBR map synthesis from albedo** (`src/PbrMapSynth.h/cpp` + `src/AIAssistManager.h/cpp`, issue #404): predicts normal + height maps from a single albedo/diffuse texture via an ONNX UNet (DeepBump-style) and derives roughness from albedo luminance. **First ONNX consumer.** `cmake/OnnxRuntime.cmake` downloads the prebuilt ONNX Runtime 1.20.1 per-platform behind `ENABLE_ONNX` (OFF by default; ON for release + the Linux coverage build) and exposes the imported `qtmesh_onnx` target — macOS uses the **universal2** archive (no per-arch trap; CoreML EP inside, CPU EP fallback). `PbrMapSynth` is the Ogre-free, unit-tested core (NCHW packing, overlapping-tile inference with feathered seam blend, normal/height decode with strength + OpenGL/DirectX `invertG`, roughness heuristic); it discovers the model's input channel count + output names/shapes at runtime rather than hardcoding, and falls back to the Sobel `NormalMapGenerator` to derive a normal when the model emits only height. **Model: PBRify_Remix (CC0-1.0).** Three separate per-map SPAN models (`1x-PBRify_NormalV3` / `RoughnessV2` / `Height`, all 3-channel-in/3-channel-out, ~1.6 MB each as ONNX) from [Kim2091/PBRify_Remix](https://github.com/Kim2091/PBRify_Remix), trained only on CC0 AmbientCG/Poly Haven textures — redistributable with zero obligations (DeepBump is GPL-3.0 and was rejected). **License due-diligence (#404):** the repo's own LICENSE is CC0-1.0 and its README states the models were *"trained exclusively on high quality CC0 content from ambientCG"*. OpenModelDB's NormalV3 page lists the training set as "ambientCG + UltraSharpV2", and UltraSharpV2 is itself `cc-by-nc-sa-4.0` — a discrepancy. We treat the **author's explicit repo CC0 LICENSE + "exclusively CC0" statement as authoritative** (the OpenModelDB note is third-party/likely stale) and ship on that basis; revisit if the author clarifies otherwise. They ship as PyTorch `.pth`; `scripts/export-pbrify-onnx.py` is the one-time, offline, NOT-shipped dev tool that converts them to ONNX (spandrel load → `torch.onnx.export` opset 18, `dynamo=False`, dynamic H/W). `AIAssistManager` (QML_SINGLETON, SDManager pattern) resolves each map's model under `AppData/ai_models/pbr/<file>.onnx`, downloads any missing ones on first use via `ModelDownloader` (base URL from `QSettings ai/pbrModelBaseUrl` or `QTMESH_PBR_MODEL_BASE_URL` env, defaulting to the hosted [`fernandotonon/QtMeshEditor-models`](https://huggingface.co/fernandotonon/QtMeshEditor-models) HF repo via `kDefaultModelBaseUrl`), caches outputs next to the source albedo, and binds normal/roughness into the slice-E canonical slots (`RTShaderHelper::wirePbrSlotsForFFP`). Normal decodes the model's RGB as tangent-space; roughness/height take Rec.601 luminance of the RGB output. Roughness falls back to the offline luminance heuristic when its model is absent, so a roughness-only request always works. **Synchronous** (ONNX is fast — no worker thread), with `pbrSynthStarted/Completed/Error` signals for the GUI. Surfaced via the **"Generate PBR maps from diffuse" button** in the Material Editor's Texture Properties panel (`qml/TexturePropertiesPanel.qml`, shown only when `aiPbrAvailable()`), the MCP `generate_pbr_maps` tool, and the **CLI `qtmesh material --texture <albedo> --generate-pbr [<mesh>] [-o out] [--tile-size N] [--no-normal] [--no-roughness] [--no-height]`** (`CLIPipeline::cmdMaterialGeneratePbr`). Roughness needs no model so a `--no-normal --no-height` request succeeds offline; a normal/height request fails gracefully (exit 1, no output) when the model is missing or the binary was built without `ENABLE_ONNX`. Sentry breadcrumb category `ai.assist.pbr_synth`. **Windows MinGW:** `ENABLE_ONNX` stays OFF (the official ONNX Runtime archive is MSVC-built and won't link under MinGW) — the feature degrades to the "rebuild with -DENABLE_ONNX" message; a follow-up can wire it. The exported `.onnx` files are hosted at [`fernandotonon/QtMeshEditor-models`](https://huggingface.co/fernandotonon/QtMeshEditor-models) (`kDefaultModelBaseUrl`) and download on first use; override with `QTMESH_PBR_MODEL_BASE_URL` / `ai/pbrModelBaseUrl`, or set `QTMESH_PBR_NO_DOWNLOAD` to force the offline path (tests do this). | ||
| - **Real-ESRGAN texture upscaling** (`src/TextureUpscaler.h/cpp` + `AIAssistManager`, issue #405): ONNX-backed 2×/4× super-resolution, reusing the #404 ONNX infra. `TextureUpscaler` is the Ogre-free core (reuses `PbrMapSynth::toNCHW`/`nchwToRgb`): a **scale-aware** overlapping-tile upscale that composites results in OUTPUT space with a feathered seam blend, detecting the scale factor from the model's output/input ratio at runtime (and validating the output tensor element count before copying — guards a mismatched-shape model). `AIAssistManager::upscaleTexture(srcPath, scale, overwrite)` extends the per-model `Map` enum with `UpscaleX2`/`UpscaleX4`, downloads the model on first use (same HF repo), runs, caches `<stem>_upscaled.png` next to the source, and emits `upscaleStarted/Completed/Error`. **Model: Real-ESRGAN x4plus / x2plus (BSD-3-Clause, [xinntao](https://github.com/xinntao/Real-ESRGAN))** — the repo LICENSE has no code/weights carve-out and OpenModelDB classifies the released weights as BSD-3; exported to ONNX via `scripts/export-realesrgan-onnx.py` (one-time, offline, NOT shipped). Surfaced via **CLI `qtmesh material --texture <low> --upscale {2|4} [-o <high>]`** (`CLIPipeline::cmdMaterialUpscale`), the MCP `upscale_texture` tool, and **"Upscale 2× / 4×" buttons** in the Material Editor's Texture Properties panel. Sentry breadcrumb category `ai.assist.upscale`. Real-ESRGAN on CPU is slow (~2 min for a 256² → 1024² 4× on a laptop); CoreML EP on macOS helps. Verified end-to-end: 256→1024 (4×) and 128→256 (2×) with the model auto-downloaded. |
There was a problem hiding this comment.
Keep the cached output name consistent with the implementation.
The docs still say upscaled results are cached as <stem>_upscaled.png, but the behavior here is scale-specific (<stem>_upscaled_x2.png / _x4.png). That stale wording will mislead users about cache reuse and overwrite behavior.
Suggested doc patch
- caches results as `<stem>_upscaled.png` next to the source
+- caches results as `<stem>_upscaled_x2.png` / `<stem>_upscaled_x4.png` next to the source, depending on scale🧰 Tools
🪛 LanguageTool
[grammar] ~252-~252: Use a hyphen to join words.
Context: ...odels were "trained exclusively on high quality CC0 content from ambientCG". Op...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` around lines 252 - 253, Update the documentation in the
Real-ESRGAN texture upscaling section to reflect the actual scale-specific cache
naming convention. Locate the phrase "caches `<stem>_upscaled.png` next to the
source" in the text describing AIAssistManager::upscaleTexture and change it to
clarify that the cached output filename includes the scale factor:
`<stem>_upscaled_x2.png` for 2× upscaling and `<stem>_upscaled_x4.png` for 4×
upscaling, ensuring the documentation accurately reflects the implementation
behavior for cache reuse and overwrite handling.
Comment on lines
+4255
to
+4277
| emit upscaleStarted(); | ||
| // Ensure the model on THIS (GUI) thread first — the download uses a | ||
| // QEventLoop and ModelDownloader's queued signals, which need a running | ||
| // event loop. If the model isn't on disk yet, signal the download phase so | ||
| // the UI can show "Downloading model…" instead of a silent "Upscaling…". | ||
| AIAssistManager* ai = AIAssistManager::instance(); | ||
| const QString preModel = ai->modelPath( | ||
| (scale == 2) ? AIAssistManager::Map::UpscaleX2 : AIAssistManager::Map::UpscaleX4); | ||
| if (!QFileInfo::exists(preModel)) | ||
| emit upscaleDownloading(); | ||
| const QString model = ai->ensureUpscaleModel(scale); | ||
| if (model.isEmpty()) { | ||
| emit upscaleError(tr("Upscale model unavailable (offline or download failed).")); | ||
| return; | ||
| } | ||
|
|
||
| const QFileInfo fi(src); | ||
| const QString outPath = QDir(fi.absolutePath()) | ||
| .filePath(fi.completeBaseName() + QStringLiteral("_upscaled_x%1.png").arg(scale)); | ||
|
|
||
| // Fresh cancel flag for this run (shared_ptr keeps it alive for the worker). | ||
| m_upscaleCancel = std::make_shared<std::atomic_bool>(false); | ||
| auto cancel = m_upscaleCancel; |
There was a problem hiding this comment.
Create the cancel flag before the model ensure step.
QML shows Cancel as soon as upscaling is set, but m_upscaleCancel is only assigned after ensureUpscaleModel(scale) returns. A Cancel click during first-run download is therefore a no-op and the worker still starts afterward.
🛠️ Proposed fix
}
emit upscaleStarted();
+ // Fresh cancel flag for this run before any potentially long model download.
+ m_upscaleCancel = std::make_shared<std::atomic_bool>(false);
+ auto cancel = m_upscaleCancel;
+
// Ensure the model on THIS (GUI) thread first — the download uses a
// QEventLoop and ModelDownloader's queued signals, which need a running
// event loop. If the model isn't on disk yet, signal the download phase so
@@
const QString model = ai->ensureUpscaleModel(scale);
+ if (cancel->load()) {
+ emit upscaleError(tr("cancelled"));
+ return;
+ }
if (model.isEmpty()) {
emit upscaleError(tr("Upscale model unavailable (offline or download failed)."));
return;
@@
- // Fresh cancel flag for this run (shared_ptr keeps it alive for the worker).
- m_upscaleCancel = std::make_shared<std::atomic_bool>(false);
- auto cancel = m_upscaleCancel;
QPointer<MaterialEditorQML> self(this);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MaterialEditorQML.cpp` around lines 4255 - 4277, The cancel flag
m_upscaleCancel is created too late in the upscaling workflow. It is currently
assigned after the ensureUpscaleModel method returns, but the Cancel button
becomes available to the user as soon as upscaling starts, meaning a user
clicking Cancel during the model download phase will have their action ignored
since m_upscaleCancel doesn't exist yet. Move the line that creates
m_upscaleCancel (the std::make_shared assignment) to the beginning of the
function, before any calls to ensureUpscaleModel, so that the cancel flag is
ready to receive cancellation signals from the moment the UI becomes
interactive.
Comment on lines
+4271
to
+4294
| const QFileInfo fi(src); | ||
| const QString outPath = QDir(fi.absolutePath()) | ||
| .filePath(fi.completeBaseName() + QStringLiteral("_upscaled_x%1.png").arg(scale)); | ||
|
|
||
| // Fresh cancel flag for this run (shared_ptr keeps it alive for the worker). | ||
| m_upscaleCancel = std::make_shared<std::atomic_bool>(false); | ||
| auto cancel = m_upscaleCancel; | ||
| QPointer<MaterialEditorQML> self(this); | ||
|
|
||
| // Then run the pure-CPU tiled inference + save on a worker so the UI doesn't | ||
| // freeze, reporting per-tile progress + honoring cancel, and marshal results | ||
| // back via queued invocations. (CLI/MCP keep the synchronous path.) | ||
| std::thread([self, src, model, outPath, cancel]() { | ||
| TextureUpscaler::ProgressFn onProgress = [self, cancel](int done, int total) -> bool { | ||
| if (cancel->load()) return false; // cancel requested | ||
| QMetaObject::invokeMethod(qApp, [self, done, total]() { | ||
| if (self) emit self->upscaleProgress(done, total); | ||
| }, Qt::QueuedConnection); | ||
| return true; | ||
| }; | ||
| const QImage in(src); | ||
| const TextureUpscaler::Result res = | ||
| TextureUpscaler::upscale(in, model, {}, onProgress); | ||
| const bool ok = res.ok && !res.image.isNull() && res.image.save(outPath, "PNG"); |
There was a problem hiding this comment.
Reuse the scale-specific cache before launching inference.
The GUI path always recomputes and overwrites outPath, so repeat clicks do not benefit from the _upscaled_xN.png cache and can needlessly run minutes of CPU inference. Check the output before model download/worker launch, or expose an explicit overwrite path. As per coding guidelines, Real-ESRGAN upscaling “Caches results as <stem>_upscaled.png next to source”; this PR’s scale-aware variant should preserve that cache behavior.
⚡ Proposed fix
if (src.isEmpty() || !QFileInfo::exists(src)) {
emit upscaleError(tr("No on-disk texture to upscale. Apply or save a texture first."));
return;
}
+ const QFileInfo fi(src);
+ const QString outPath = QDir(fi.absolutePath())
+ .filePath(fi.completeBaseName() + QStringLiteral("_upscaled_x%1.png").arg(scale));
+ if (QFileInfo::exists(outPath)) {
+ emit upscaleCompleted(outPath);
+ return;
+ }
emit upscaleStarted();
@@
- const QFileInfo fi(src);
- const QString outPath = QDir(fi.absolutePath())
- .filePath(fi.completeBaseName() + QStringLiteral("_upscaled_x%1.png").arg(scale));
-
// Fresh cancel flag for this run (shared_ptr keeps it alive for the worker).📝 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
| const QFileInfo fi(src); | |
| const QString outPath = QDir(fi.absolutePath()) | |
| .filePath(fi.completeBaseName() + QStringLiteral("_upscaled_x%1.png").arg(scale)); | |
| // Fresh cancel flag for this run (shared_ptr keeps it alive for the worker). | |
| m_upscaleCancel = std::make_shared<std::atomic_bool>(false); | |
| auto cancel = m_upscaleCancel; | |
| QPointer<MaterialEditorQML> self(this); | |
| // Then run the pure-CPU tiled inference + save on a worker so the UI doesn't | |
| // freeze, reporting per-tile progress + honoring cancel, and marshal results | |
| // back via queued invocations. (CLI/MCP keep the synchronous path.) | |
| std::thread([self, src, model, outPath, cancel]() { | |
| TextureUpscaler::ProgressFn onProgress = [self, cancel](int done, int total) -> bool { | |
| if (cancel->load()) return false; // cancel requested | |
| QMetaObject::invokeMethod(qApp, [self, done, total]() { | |
| if (self) emit self->upscaleProgress(done, total); | |
| }, Qt::QueuedConnection); | |
| return true; | |
| }; | |
| const QImage in(src); | |
| const TextureUpscaler::Result res = | |
| TextureUpscaler::upscale(in, model, {}, onProgress); | |
| const bool ok = res.ok && !res.image.isNull() && res.image.save(outPath, "PNG"); | |
| if (src.isEmpty() || !QFileInfo::exists(src)) { | |
| emit upscaleError(tr("No on-disk texture to upscale. Apply or save a texture first.")); | |
| return; | |
| } | |
| const QFileInfo fi(src); | |
| const QString outPath = QDir(fi.absolutePath()) | |
| .filePath(fi.completeBaseName() + QStringLiteral("_upscaled_x%1.png").arg(scale)); | |
| if (QFileInfo::exists(outPath)) { | |
| emit upscaleCompleted(outPath); | |
| return; | |
| } | |
| emit upscaleStarted(); | |
| // Fresh cancel flag for this run (shared_ptr keeps it alive for the worker). | |
| m_upscaleCancel = std::make_shared<std::atomic_bool>(false); | |
| auto cancel = m_upscaleCancel; | |
| QPointer<MaterialEditorQML> self(this); | |
| // Then run the pure-CPU tiled inference + save on a worker so the UI doesn't | |
| // freeze, reporting per-tile progress + honoring cancel, and marshal results | |
| // back via queued invocations. (CLI/MCP keep the synchronous path.) | |
| std::thread([self, src, model, outPath, cancel]() { | |
| TextureUpscaler::ProgressFn onProgress = [self, cancel](int done, int total) -> bool { | |
| if (cancel->load()) return false; // cancel requested | |
| QMetaObject::invokeMethod(qApp, [self, done, total]() { | |
| if (self) emit self->upscaleProgress(done, total); | |
| }, Qt::QueuedConnection); | |
| return true; | |
| }; | |
| const QImage in(src); | |
| const TextureUpscaler::Result res = | |
| TextureUpscaler::upscale(in, model, {}, onProgress); | |
| const bool ok = res.ok && !res.image.isNull() && res.image.save(outPath, "PNG"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MaterialEditorQML.cpp` around lines 4271 - 4294, The upscale operation
always recomputes and overwrites the output file at outPath, ignoring any cached
result from a previous run with the same scale factor. Before launching the
worker thread with the std::thread lambda that calls TextureUpscaler::upscale,
check if the file already exists at outPath. If the cached upscaled image
exists, skip the expensive inference and worker launch by either returning early
or directly emitting the completion signal with the cached file path. This
preserves the cache behavior and avoids redundant CPU-intensive computation on
repeated requests with the same scale.
Source: Coding guidelines
Comment on lines
+577
to
+580
| /// #405: upscale the current texture by `scale` (2 or 4) via Real-ESRGAN, | ||
| /// writing <stem>_upscaled.png next to the source. Runs on a worker thread; | ||
| /// emits upscaleDownloading (during first-run model fetch), upscaleProgress | ||
| /// (per tile), and upscaleCompleted/Error. |
There was a problem hiding this comment.
Document the scale-aware output filename.
The implementation writes _upscaled_x%1.png, but this public QML API comment still says _upscaled.png. Keep the contract aligned so callers know which file to expect.
📝 Proposed fix
- /// writing <stem>_upscaled.png next to the source. Runs on a worker thread;
+ /// writing <stem>_upscaled_x{2,4}.png next to the source. Runs on a worker thread;📝 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
| /// #405: upscale the current texture by `scale` (2 or 4) via Real-ESRGAN, | |
| /// writing <stem>_upscaled.png next to the source. Runs on a worker thread; | |
| /// emits upscaleDownloading (during first-run model fetch), upscaleProgress | |
| /// (per tile), and upscaleCompleted/Error. | |
| /// `#405`: upscale the current texture by `scale` (2 or 4) via Real-ESRGAN, | |
| /// writing <stem>_upscaled_x{2,4}.png next to the source. Runs on a worker thread; | |
| /// emits upscaleDownloading (during first-run model fetch), upscaleProgress | |
| /// (per tile), and upscaleCompleted/Error. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MaterialEditorQML.h` around lines 577 - 580, The documentation comment
for the upscale function (the comment block at lines 577-580 in
MaterialEditorQML.h) states that the output file is written as `_upscaled.png`,
but the actual implementation uses `_upscaled_x%1.png` (where %1 is the scale
factor). Update the comment to reflect the correct filename pattern
`_upscaled_x%1.png` so that callers know which file to expect.
Comment on lines
+89
to
+90
| Result upscale(const QImage& srcIn, const QString& modelPath, const Options& opts, | ||
| const ProgressFn& onProgress) |
There was a problem hiding this comment.
Emit the required ai.assist.upscale breadcrumb in the core path.
The GUI worker calls TextureUpscaler::upscale directly, so this operation can bypass any AIAssistManager::upscaleTexture breadcrumb. Add a breadcrumb at the start of the core operation and avoid logging full file paths. As per coding guidelines, “AI-assisted features … must emit Sentry breadcrumbs with category 'ai.assist.*'.”
🛠️ Proposed fix
Result upscale(const QImage& srcIn, const QString& modelPath, const Options& opts,
const ProgressFn& onProgress)
{
+ SentryReporter::addBreadcrumb(QStringLiteral("ai.assist.upscale"),
+ QStringLiteral("Texture upscale requested"));
Result r;If SentryReporter.h is not already included in this file, add it with the other local includes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TextureUpscaler.cpp` around lines 89 - 90, The upscale function (starting
at the function signature with parameters srcIn, modelPath, opts, and
onProgress) is missing the required Sentry breadcrumb emission. First, ensure
SentryReporter.h is included at the top of the file with other local includes if
it is not already present. Then, at the beginning of the upscale function body,
add a breadcrumb emission with category 'ai.assist.upscale' to track this
AI-assisted operation. When constructing the breadcrumb details, avoid including
the full file path from modelPath to keep logs clean and avoid exposing system
paths.
Source: Coding guidelines
Comment on lines
+258
to
+265
| const QImage upAlpha = srcAlpha.scaled(outW, outH, Qt::IgnoreAspectRatio, | ||
| Qt::SmoothTransformation); | ||
| QImage rgba = rgb.convertToFormat(QImage::Format_RGBA8888); | ||
| for (int y = 0; y < outH; ++y) { | ||
| const uchar* aline = upAlpha.constScanLine(y); | ||
| uchar* dline = rgba.scanLine(y); | ||
| for (int x = 0; x < outW; ++x) | ||
| dline[x * 4 + 3] = aline[x * 4 + 3]; // copy alpha channel |
There was a problem hiding this comment.
Guard alpha scaling before copying scanlines.
scaled() or convertToFormat() can return a null QImage under memory pressure, but the loop immediately dereferences scanlines. Also, the comment says nearest-neighbour alpha, while SmoothTransformation blurs cutout/opacity masks.
🛡️ Proposed fix
- const QImage upAlpha = srcAlpha.scaled(outW, outH, Qt::IgnoreAspectRatio,
- Qt::SmoothTransformation);
+ const QImage upAlpha = srcAlpha.scaled(outW, outH, Qt::IgnoreAspectRatio,
+ Qt::FastTransformation);
+ if (upAlpha.isNull()) {
+ r.error = QStringLiteral("failed to scale alpha channel");
+ return r;
+ }
QImage rgba = rgb.convertToFormat(QImage::Format_RGBA8888);
+ if (rgba.isNull()) {
+ r.error = QStringLiteral("failed to allocate RGBA upscaled image");
+ return r;
+ }
for (int y = 0; y < outH; ++y) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TextureUpscaler.cpp` around lines 258 - 265, The code does not validate
the results of scaled() and convertToFormat() before dereferencing them with
constScanLine() and scanLine(), which can cause null pointer dereferences under
memory pressure. Additionally, Qt::SmoothTransformation blurs the alpha mask
inappropriately when nearest-neighbor scaling is intended. Add null checks for
both the upAlpha QImage returned from srcAlpha.scaled() and the rgba QImage
returned from rgb.convertToFormat() before accessing their scanlines, and
replace Qt::SmoothTransformation with Qt::FastTransformation when scaling
srcAlpha to preserve the alpha channel with nearest-neighbor filtering instead
of smoothing.
|
4 tasks
fernandotonon
added a commit
that referenced
this pull request
Jun 22, 2026
Reflect what actually shipped in #749: worker-threaded GUI path with download/progress/cancel signals, scale-suffixed cache filename, and the hardware_concurrency-1 intra-op thread bump (256→1024 4× ~2min → ~7.5s). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Implements #405 — 2×/4× texture super-resolution via Real-ESRGAN, reusing the ONNX infra landed for #404.
Backend & model (per the issue's fallback guidance)
ENABLE_ONNX, macOS universal2 = CoreML+CPU EP, Linux CI) with zero new dependency.scripts/export-realesrgan-onnx.pyand hosted on the existingfernandotonon/QtMeshEditor-modelsHF repo, downloaded on first use.What's included
TextureUpscaler(Ogre-free, reusesPbrMapSynth::toNCHW/nchwToRgb): scale-aware overlapping-tile upscale, compositing in OUTPUT space with a feathered seam blend; scale detected from the model's output/input ratio at runtime; output-tensor element count validated before copy.AIAssistManager::upscaleTexture—Mapenum extended withUpscaleX2/UpscaleX4; ensure-download + run + cache (<stem>_upscaled.png),upscaleStarted/Completed/Errorsignals.qtmesh material --texture low.png --upscale {2|4} [-o high.png].upscale_texturetool.ai.assist.upscale. Cache skips re-upscale of an existing output.Acceptance criteria
ai.assist.upscaleTests
QTMESH_PBR_NO_DOWNLOAD.TextureUpscalererror-contract tests (null source, missing model, not-built).-DENABLE_ONNX=ON(Linux).Notes
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--upscaleflag), and API. Output files are saved as<filename>_upscaled.png.Improvements