feat: AI PBR map synthesis from albedo (ONNX) (#404)#738
feat: AI PBR map synthesis from albedo (ONNX) (#404)#738fernandotonon wants to merge 11 commits into
Conversation
First step of AI PBR map synthesis (#404): land ONNX Runtime as an optional dependency behind ENABLE_ONNX (OFF by default; release/test builds will turn it on in a later commit). - cmake/OnnxRuntime.cmake downloads the official prebuilt ONNX Runtime 1.20.1 release archive per-platform (verified SHA256) and exposes it as the imported SHARED target `qtmesh_onnx`, mirroring cmake/Libsodium.cmake's download pattern. macOS uses the universal2 archive so there is no per-arch trap (the libsodium-built-x86_64 lesson); CoreML EP ships inside it, CPU EP is always present. Linux x64/aarch64 selected via CMAKE_SYSTEM_PROCESSOR. Windows is mapped but ENABLE_ONNX stays off on the MinGW path (MSVC-built archive won't link) — the feature degrades gracefully. - .gitignore: broaden the cmake-module negation from the single Libsodium.cmake to cmake/*.cmake so tracked CMake modules aren't swallowed by the *.cmake rule. No consumers yet — configures as a no-op until AIAssistManager lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- PbrMapSynth (Ogre-free, unit-testable like NormalMapGenerator): NCHW tensor packing, overlapping-tile inference with feathered seam blend, normal/height tensor decoding (strength + OpenGL/DirectX invertG), and a low-frequency roughness heuristic from albedo luminance. The ONNX inference (synthesize()) is guarded by ENABLE_ONNX; the building blocks compile unconditionally. Input channel count + output names/shapes are discovered from the model at runtime rather than hardcoded, so a permissive UNet whose convention differs from DeepBump still works (normal, height, or both). - AIAssistManager (QML_SINGLETON, SDManager-pattern): wraps PbrMapSynth, resolves the model under AppData/ai_models/pbr, downloads it on first use via ModelDownloader (URL via QSettings ai/pbrModelUrl or QTMESH_PBR_MODEL_URL env; empty default until a permissive host is chosen), caches outputs next to the source albedo, and derives normal-from-height via the existing Sobel NormalMapGenerator when the model emits only height. Synchronous (ONNX is fast) with progress/completed/error signals for the GUI. Sentry breadcrumb ai.assist.pbr_synth. - CMake: compile both into the app + UnitTests; link qtmesh_onnx and copy the ONNX Runtime shared lib next to each binary (POST_BUILD) when ENABLE_ONNX. Verified: builds + links against ONNX Runtime 1.20.1 on macOS arm64. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GL-free, ONNX-free tests for the synthesis building blocks: NCHW packing (RGB + luminance), nchwToRgb round-trip, flat-normal→blue decode, invertG green flip, height scaling, and the roughness heuristic (dark>bright, flat-luminance level). Run on any build under Xvfb in CI. Verified locally via a standalone harness (the gtest main aborts without GL on macOS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CLI surface for PBR map synthesis:
qtmesh material --texture albedo.png --generate-pbr [<mesh>] [-o out] \
[--tile-size N] [--no-normal] [--no-roughness] [--no-height]
Synchronous (no SD-style event loop): runs AIAssistManager::synthesizePbrMaps,
writes normal/roughness/height PNGs next to the albedo, and — when a mesh is
given — imports it, binds normal_map/roughness into the slice-E canonical slots
via RTShaderHelper::wirePbrSlotsForFFP, and re-exports. ENABLE_ONNX-guarded;
exit 1 with a "rebuild with -DENABLE_ONNX" message otherwise.
Also fixes PbrMapSynth::synthesize ordering so a roughness-only request
(--no-normal --no-height) succeeds offline — roughness is a pure heuristic and
must not require the ONNX model. Verified: roughness-only writes the PNG with no
model present; a full request fails cleanly when the model is missing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MCP surface for PBR map synthesis: registers + advertises generate_pbr_maps (ENABLE_ONNX-guarded). Takes albedo_path (required) + normal/roughness/height/ tile_size/overwrite; runs AIAssistManager::synthesizePbrMaps, and when a mesh is selected binds normal_map/roughness into the canonical slice-E slots via RTShaderHelper::wirePbrSlotsForFFP. Returns the output map paths + boundSubmeshes. Without ENABLE_ONNX it returns the standard "rebuild with -DENABLE_ONNX" error. Sentry breadcrumb ai.assist.pbr_synth. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Material Editor surface: a button in the Texture Properties panel (next to "Save Texture As…", shown only on an ONNX build via aiPbrAvailable()) that runs MaterialEditorQML::generatePbrFromDiffuse() on the current texture. It resolves the diffuse's on-disk path via the existing group-agnostic preview resolver, calls AIAssistManager::synthesizePbrMaps, and binds normal_map/roughness into the active material's canonical slots (RTShaderHelper::wirePbrSlotsForFFP + compile). pbrSynthStarted/Completed/Error signals drive a small status label. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
) - CLIPipeline_cmdmaterial_coverage_test.cpp: --generate-pbr cases — missing --texture (2/1), out-of-range --tile-size (2/1), roughness-only success offline (writes _roughness.png, ENABLE_ONNX) and the no-model/full-request clean failure (exit 1, no maps). Each branches on ENABLE_ONNX. - deploy.yml: add -DENABLE_ONNX=ON to the Linux coverage/test build and the Linux + macOS release builds (macOS universal2 archive is safe under -DCMAKE_OSX_ARCHITECTURES). Windows MinGW intentionally left off. - CLAUDE.md: document the CLI subcommands + a full architecture entry under the AI-Assisted Authoring epic (#397), including the GPL-model rationale, the empty production-URL TODO, and the Windows-MinGW deferral. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 37 minutes and 21 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces end-to-end AI PBR map synthesis from albedo textures using ONNX Runtime. It adds a new ChangesONNX PBR Map Synthesis Feature
Sequence Diagram(s)sequenceDiagram
actor User
participant TexturePropertiesPanel
participant MaterialEditorQML
participant AIAssistManager
participant PbrMapSynth
participant OnnxRuntime
User->>TexturePropertiesPanel: click "Generate PBR maps from diffuse"
TexturePropertiesPanel->>MaterialEditorQML: generatePbrFromDiffuse()
MaterialEditorQML->>MaterialEditorQML: resolve diffuse file path
MaterialEditorQML->>AIAssistManager: synthesizePbrMaps(albedoPath, opts)
AIAssistManager->>AIAssistManager: check disk cache
alt cache miss
AIAssistManager->>PbrMapSynth: synthesize(albedo, modelPath, opts)
PbrMapSynth->>OnnxRuntime: session.Run(tiled input)
OnnxRuntime-->>PbrMapSynth: normal/height planes
PbrMapSynth-->>AIAssistManager: Result (normal/roughness/height QImage)
AIAssistManager->>AIAssistManager: write output PNGs
end
AIAssistManager-->>MaterialEditorQML: PbrMapSynthResult
MaterialEditorQML->>MaterialEditorQML: bind textures into Ogre material, wire PBR slots
MaterialEditorQML->>TexturePropertiesPanel: emit pbrSynthCompleted(result)
TexturePropertiesPanel->>User: update status label
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/PbrMapSynth_test.cpp (1)
42-52: ⚡ Quick winAdd regression tests for invalid channel-count handling and zero-valued outputs.
Please add cases for unsupported
toNCHW(..., channels)values (e.g., 2/4) and for “valid but all-zero” output presence semantics, so these edge paths stay covered.Also applies to: 137-146
🤖 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/PbrMapSynth_test.cpp` around lines 42 - 52, The test ToNchwGrayscaleLuma currently only covers valid single-channel conversion. Add regression test cases to cover edge cases: test the toNCHW function with unsupported channel counts (such as 2 and 4) to verify proper error handling or expected behavior for invalid inputs, and add test cases that verify the function correctly handles inputs that produce all-zero outputs to ensure the edge case for zero-valued output presence semantics is properly covered. These additional assertions should be included in the existing test function to ensure these error paths remain covered.src/CLIPipeline.cpp (1)
3791-3792: ⚡ Quick winAdd file import/export breadcrumbs for the mesh branch.
The PBR mesh path imports and exports assets but only records the AI breadcrumb, so file I/O is missing from telemetry.
Proposed fix
if (!initOgreHeadless()) return 1; + SentryReporter::addBreadcrumb(QStringLiteral("file.import"), + QStringLiteral("Importing file %1").arg(meshFi.absoluteFilePath())); MeshImporterExporter::importer({meshFi.absoluteFilePath()}); @@ Ogre::SceneNode* node = entity->getParentSceneNode(); + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), + QStringLiteral("Exporting file %1").arg(outFi.absoluteFilePath())); if (MeshImporterExporter::exporter(node, outFi.absoluteFilePath(), formatForExtension(outputPath)) != 0) {As per coding guidelines, all user-facing actions and significant operations must be tracked with
SentryReporter::addBreadcrumb(category, message), usingfile.import/file.exportfor I/O operations.Also applies to: 3834-3836
🤖 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/CLIPipeline.cpp` around lines 3791 - 3792, The MeshImporterExporter::importer call at line 3792 is missing a telemetry breadcrumb to track the file import operation. Add a SentryReporter::addBreadcrumb call with category "file.import" and a descriptive message (e.g., including the file path) immediately before or after the MeshImporterExporter::importer invocation to ensure this user-facing file I/O operation is properly recorded in telemetry. Apply the same fix to the corresponding export operations mentioned at lines 3834-3836.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 72-73: The documentation for the qtmesh material command examples
needs clarification on mesh behavior and output files. Update the first example
to explicitly include a mesh argument and clarify that output is bound to the
mesh, or remove the "bound to mesh" description if no mesh is provided. Update
the second example to document that in addition to writing the PNGs, a .material
sidecar file is also generated. Ensure both examples clearly document their
respective output behaviors so users understand the difference between the
mesh-bound and mesh-less workflows.
In `@cmake/OnnxRuntime.cmake`:
- Around line 37-40: The elseif(WIN32) block at line 37 doesn't exclude MinGW
systems even though the file documents MinGW as unsupported. Since MinGW sets
WIN32=TRUE, it will enter this branch and attempt to download MSVC-built
artifacts, causing link-time failures. Add a MinGW exclusion guard to the
elseif(WIN32) condition by checking for the absence of MinGW (using CMake's
built-in MinGW detection variable) so MinGW builds properly skip this block and
degrade gracefully as intended.
In `@src/CLIPipeline_cmdmaterial_coverage_test.cpp`:
- Around line 372-381: The test `GeneratePbrNoModelFailsCleanly` expects the
command to fail due to a missing PBR model, but the AIAssistManager can resolve
the cached pbr_unet.onnx model from the user's AppData directory, causing the
assertion on the expected failure to be invalidated in environments where the
model is already cached. Isolate this test from the global model cache by either
setting up a temporary empty model location that the AIAssistManager will use
instead of AppData, or by mocking/overriding the model readiness state before
executing the cmdMaterial call to ensure the model resolution fails as expected
regardless of what is cached in the system.
In `@src/CLIPipeline.cpp`:
- Around line 3788-3817: The generated map files are being referenced by
basename in the bindSlot lambda but the actual files remain next to the albedo
path rather than beside the exported mesh. Before calling bindSlot with the
generated map paths (the parameters passed to bindSlot in the lines following
the resource group manager setup), copy those generated texture files from their
current location to the output directory specified by outputPath to ensure the
exported material package includes all necessary texture files alongside the
mesh.
- Around line 3332-3334: The pbrTileSize assignment uses QString().toInt() which
silently converts invalid non-numeric input to 0, bypassing subsequent
validation checks since 0 is treated as a valid value. Add validation after the
conversion to check if the string conversion was successful using the boolean
parameter form of toInt() method, and reject non-numeric arguments with an error
message rather than allowing them to silently default to 0.
In `@src/MCPServer.cpp`:
- Around line 1701-1706: The tile_size parameter handling in the
PbrMapSynth::Options configuration does not validate bounds, unlike the CLI. Add
bounds validation for the tile_size argument to ensure it is either 0 or falls
within the range of 32 to 4096 before assigning it to opts.tileSize. This
prevents invalid values from being passed to the synthesis function that could
cause excessive allocation or work. Check the CLI implementation for the exact
validation logic to mirror, then apply the same bounds check in the MCP handler
where opts.tileSize is currently set without validation.
In `@src/PbrMapSynth.cpp`:
- Around line 305-309: The accHeight vector is only allocated when
opts.generateHeight is true, but the normal derivation logic at lines 375-377
may require height data to be present even when generateHeight is disabled.
Additionally, the haveHeight and haveNormal flags are inferred from non-zero
data which incorrectly treats valid all-zero maps as missing. Fix this by always
allocating accHeight regardless of opts.generateHeight value, and update the
haveHeight and haveNormal inference logic (around lines 363-378) to track
whether data was actually generated or processed rather than relying solely on
whether the data contains non-zero values.
- Around line 60-74: The `toNCHW` function assumes that any channel count other
than 1 must be 3, and unconditionally writes three planes to the output vector
in the else block. However, `runModelOnce` can pass `channels == 2`, which
causes out-of-bounds writes to the output vector. Fix this by validating that
channels actually equals 3 before writing all three planes in the else branch,
or by adding a separate case to properly handle `channels == 2` with only two
planes written to the vector.
- Around line 236-244: In the code block where out.normal.assign() and
out.height.assign() are called, add validation before copying data to ensure the
output tensor's spatial dimensions match the input dimensions and sufficient
data exists. Specifically, after getting the shape with info.GetShape(), verify
that sh[2] equals the input height (h) and sh[3] equals the input width (w),
then use GetElementCount() to confirm the tensor has enough elements before
proceeding with the assign() calls for both the normal and height cases. This
prevents buffer over-read or under-read errors when the model outputs different
H/W dimensions.
---
Nitpick comments:
In `@src/CLIPipeline.cpp`:
- Around line 3791-3792: The MeshImporterExporter::importer call at line 3792 is
missing a telemetry breadcrumb to track the file import operation. Add a
SentryReporter::addBreadcrumb call with category "file.import" and a descriptive
message (e.g., including the file path) immediately before or after the
MeshImporterExporter::importer invocation to ensure this user-facing file I/O
operation is properly recorded in telemetry. Apply the same fix to the
corresponding export operations mentioned at lines 3834-3836.
In `@src/PbrMapSynth_test.cpp`:
- Around line 42-52: The test ToNchwGrayscaleLuma currently only covers valid
single-channel conversion. Add regression test cases to cover edge cases: test
the toNCHW function with unsupported channel counts (such as 2 and 4) to verify
proper error handling or expected behavior for invalid inputs, and add test
cases that verify the function correctly handles inputs that produce all-zero
outputs to ensure the edge case for zero-valued output presence semantics is
properly covered. These additional assertions should be included in the existing
test function to ensure these error paths remain covered.
🪄 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: b94c97ae-5bbb-4d37-be6e-a850e3183700
📒 Files selected for processing (19)
.github/workflows/deploy.yml.gitignoreCLAUDE.mdCMakeLists.txtcmake/OnnxRuntime.cmakeqml/TexturePropertiesPanel.qmlsrc/AIAssistManager.cppsrc/AIAssistManager.hsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_cmdmaterial_coverage_test.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/PbrMapSynth.cppsrc/PbrMapSynth.hsrc/PbrMapSynth_test.cpp
| qtmesh material --texture albedo.png --generate-pbr -o out.fbx # AI PBR map synthesis (normal/roughness/height) from a diffuse → maps next to it + bound to mesh (needs ONNX build + first-run model download; roughness works offline) | ||
| qtmesh material --texture albedo.png --generate-pbr --no-height --tile-size 512 # selective maps + larger model tiles; omit the mesh to just write the PNGs |
There was a problem hiding this comment.
Clarify the optional mesh behavior in these examples.
The first example says the output is “bound to mesh” even though no <mesh> argument is passed, and the no-mesh path also writes a .material sidecar in addition to the PNGs. Please make one example include a mesh and update the other to reflect the sidecar.
Proposed doc tweak
-qtmesh material --texture albedo.png --generate-pbr -o out.fbx # AI PBR map synthesis (normal/roughness/height) from a diffuse → maps next to it + bound to mesh (needs ONNX build + first-run model download; roughness works offline)
-qtmesh material --texture albedo.png --generate-pbr --no-height --tile-size 512 # selective maps + larger model tiles; omit the mesh to just write the PNGs
+qtmesh material --texture albedo.png --generate-pbr mesh.fbx -o out.fbx # AI PBR map synthesis (normal/roughness/height) from a diffuse → maps next to it + bound to mesh (needs ONNX build + first-run model download; roughness works offline)
+qtmesh material --texture albedo.png --generate-pbr --no-height --tile-size 512 # selective maps + larger model tiles; omit the mesh to just write the PNGs and a .material sidecar📝 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.
| qtmesh material --texture albedo.png --generate-pbr -o out.fbx # AI PBR map synthesis (normal/roughness/height) from a diffuse → maps next to it + bound to mesh (needs ONNX build + first-run model download; roughness works offline) | |
| qtmesh material --texture albedo.png --generate-pbr --no-height --tile-size 512 # selective maps + larger model tiles; omit the mesh to just write the PNGs | |
| qtmesh material --texture albedo.png --generate-pbr mesh.fbx -o out.fbx # AI PBR map synthesis (normal/roughness/height) from a diffuse → maps next to it + bound to mesh (needs ONNX build + first-run model download; roughness works offline) | |
| qtmesh material --texture albedo.png --generate-pbr --no-height --tile-size 512 # selective maps + larger model tiles; omit the mesh to just write the PNGs and a .material sidecar |
🤖 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 72 - 73, The documentation for the qtmesh material
command examples needs clarification on mesh behavior and output files. Update
the first example to explicitly include a mesh argument and clarify that output
is bound to the mesh, or remove the "bound to mesh" description if no mesh is
provided. Update the second example to document that in addition to writing the
PNGs, a .material sidecar file is also generated. Ensure both examples clearly
document their respective output behaviors so users understand the difference
between the mesh-bound and mesh-less workflows.
| elseif(WIN32) | ||
| set(_ort_archive "onnxruntime-win-x64-${QTMESH_ONNX_VERSION}.zip") | ||
| set(_ort_sha256 "78d447051e48bd2e1e778bba378bec4ece11191c9e538cf7b2c4a4565e8f5581") | ||
| set(_ort_libname "onnxruntime.dll") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "MINGW|WIN32|onnxruntime-win-x64|ENABLE_ONNX|add_definitions\\(-DENABLE_ONNX\\)" \
CMakeLists.txt cmake/OnnxRuntime.cmake src/CMakeLists.txt .github/workflows/deploy.ymlRepository: fernandotonon/QtMeshEditor
Length of output: 1856
🏁 Script executed:
cat -n cmake/OnnxRuntime.cmakeRepository: fernandotonon/QtMeshEditor
Length of output: 4677
🏁 Script executed:
head -50 .github/workflows/deploy.yml | grep -A 20 "matrix:"Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
rg -B 10 "elseif\(WIN32\)" cmake/OnnxRuntime.cmake | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 652
Add MinGW guard before WIN32 ONNX archive routing to prevent build-time failure.
The file documents that MinGW is unsupported (lines 11–13), but the elseif(WIN32) branch at line 37 has no guard to exclude MinGW systems. Since MinGW sets WIN32=TRUE, builds with -DENABLE_ONNX=ON on MinGW will download and attempt to link MSVC-built artifacts, failing at link time rather than degrading gracefully as intended.
🔧 Suggested fix
elseif(UNIX)
if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64|arm64")
set(_ort_archive "onnxruntime-linux-aarch64-${QTMESH_ONNX_VERSION}.tgz")
set(_ort_sha256 "ae4fedbdc8c18d688c01306b4b50c63de3445cdf2dbd720e01a2fa3810b8106a")
else()
set(_ort_archive "onnxruntime-linux-x64-${QTMESH_ONNX_VERSION}.tgz")
set(_ort_sha256 "67db4dc1561f1e3fd42e619575c82c601ef89849afc7ea85a003abbac1a1a105")
endif()
set(_ort_libname "libonnxruntime.so.${QTMESH_ONNX_VERSION}")
+elseif(MINGW)
+ message(FATAL_ERROR "ENABLE_ONNX: MinGW is not supported (official ONNX Runtime Windows binaries are MSVC-built). "
+ "Disable with -DENABLE_ONNX=OFF for MinGW builds.")
elseif(WIN32)
set(_ort_archive "onnxruntime-win-x64-${QTMESH_ONNX_VERSION}.zip")
set(_ort_sha256 "78d447051e48bd2e1e778bba378bec4ece11191c9e538cf7b2c4a4565e8f5581")
set(_ort_libname "onnxruntime.dll")🤖 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 `@cmake/OnnxRuntime.cmake` around lines 37 - 40, The elseif(WIN32) block at
line 37 doesn't exclude MinGW systems even though the file documents MinGW as
unsupported. Since MinGW sets WIN32=TRUE, it will enter this branch and attempt
to download MSVC-built artifacts, causing link-time failures. Add a MinGW
exclusion guard to the elseif(WIN32) condition by checking for the absence of
MinGW (using CMake's built-in MinGW detection variable) so MinGW builds properly
skip this block and degrade gracefully as intended.
| TEST_F(CLIPipelineCmdMaterialCoverageTest, GeneratePbrNoModelFailsCleanly) | ||
| { | ||
| QTemporaryDir dir; | ||
| ASSERT_TRUE(dir.isValid()); | ||
| const QString albedo = writeAlbedo(dir, "tex.png", qRgb(200, 180, 160)); | ||
| ASSERT_FALSE(albedo.isEmpty()); | ||
|
|
||
| ArgvBuilder args({"qtmesh", "material", "--texture", albedo, "--generate-pbr"}); | ||
| EXPECT_EQ(CLIPipeline::cmdMaterial(args.argc(), args.argv()), 1); | ||
| EXPECT_FALSE(QFile::exists(dir.filePath("tex_normal.png"))); |
There was a problem hiding this comment.
Isolate the no-model test from the global model cache.
This test assumes the ONNX model is unavailable, but the manager resolves it from AppData; a developer/CI image with pbr_unet.onnx already cached can make Line 380 succeed and invalidate the failure assertion. Force an empty temp model location or otherwise override model readiness for this case.
As per coding guidelines, AIAssistManager must resolve the PBR model under AppData and download on first use via ModelDownloader.
🤖 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/CLIPipeline_cmdmaterial_coverage_test.cpp` around lines 372 - 381, The
test `GeneratePbrNoModelFailsCleanly` expects the command to fail due to a
missing PBR model, but the AIAssistManager can resolve the cached pbr_unet.onnx
model from the user's AppData directory, causing the assertion on the expected
failure to be invalidated in environments where the model is already cached.
Isolate this test from the global model cache by either setting up a temporary
empty model location that the AIAssistManager will use instead of AppData, or by
mocking/overriding the model readiness state before executing the cmdMaterial
call to ensure the model resolution fails as expected regardless of what is
cached in the system.
Source: Coding guidelines
| if (arg == "--tile-size" && i + 1 < argc) { | ||
| pbrTileSize = QString(argv[++i]).toInt(); | ||
| continue; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the exact lines in question
wc -l src/CLIPipeline.cppRepository: fernandotonon/QtMeshEditor
Length of output: 95
🏁 Script executed:
# Get the context around lines 3332-3334
sed -n '3320,3350p' src/CLIPipeline.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1244
🏁 Script executed:
# Search for other --tile-size usage to understand the context
rg --context 5 "tile-size" src/CLIPipeline.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1939
🏁 Script executed:
# Look for how pbrTileSize is used downstream
rg "pbrTileSize" src/CLIPipeline.cppRepository: fernandotonon/QtMeshEditor
Length of output: 216
🏁 Script executed:
# Check other argument parsing patterns in the file to see if they validate
rg -A 3 "toInt()" src/CLIPipeline.cpp | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 1664
Add validation to reject non-numeric --tile-size arguments.
The code at line 3333 silently converts invalid input like --tile-size abc to 0, bypassing the range check at lines that follow (which skips validation for 0 since it's a valid value meaning whole-image inference). This results in unexpected behavior instead of an error message.
Proposed fix
- if (arg == "--tile-size" && i + 1 < argc) {
- pbrTileSize = QString(argv[++i]).toInt();
+ if (arg == "--tile-size") {
+ if (i + 1 >= argc) {
+ err() << "Error: --tile-size requires a value." << Qt::endl;
+ return 2;
+ }
+ bool ok = false;
+ const int parsedTileSize = QString(argv[++i]).toInt(&ok);
+ if (!ok) {
+ err() << "Error: --tile-size must be an integer." << Qt::endl;
+ return 2;
+ }
+ pbrTileSize = parsedTileSize;
continue;
}🤖 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/CLIPipeline.cpp` around lines 3332 - 3334, The pbrTileSize assignment
uses QString().toInt() which silently converts invalid non-numeric input to 0,
bypassing subsequent validation checks since 0 is treated as a valid value. Add
validation after the conversion to check if the string conversion was successful
using the boolean parameter form of toInt() method, and reject non-numeric
arguments with an error message rather than allowing them to silently default to
0.
| if (outputPath.isEmpty()) outputPath = meshPath; | ||
| const QFileInfo outFi(outputPath); | ||
|
|
||
| if (!initOgreHeadless()) return 1; | ||
| MeshImporterExporter::importer({meshFi.absoluteFilePath()}); | ||
| auto& entities = Manager::getSingleton()->getEntities(); | ||
| Ogre::Entity* entity = nullptr; | ||
| for (auto* obj : entities) { | ||
| if (obj && obj->getMovableType() == "Entity") { | ||
| entity = static_cast<Ogre::Entity*>(obj); break; | ||
| } | ||
| } | ||
| if (!entity) { | ||
| err() << "Error: no mesh entity loaded from: " << meshPath << Qt::endl; | ||
| return 1; | ||
| } | ||
|
|
||
| // Register the albedo's directory so the generated PNGs resolve by name. | ||
| Ogre::ResourceGroupManager::getSingleton().addResourceLocation( | ||
| QFileInfo(albedoPath).absolutePath().toStdString(), "FileSystem", | ||
| Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME); | ||
|
|
||
| auto bindSlot = [&](Ogre::Pass* pass, const char* slot, const QString& path) { | ||
| if (path.isEmpty()) return; | ||
| const std::string tex = QFileInfo(path).fileName().toStdString(); | ||
| Ogre::TextureUnitState* tus = nullptr; | ||
| for (unsigned short i = 0; i < pass->getNumTextureUnitStates(); ++i) | ||
| if (pass->getTextureUnitState(i)->getName() == slot) { tus = pass->getTextureUnitState(i); break; } | ||
| if (!tus) { tus = pass->createTextureUnitState(); tus->setName(slot); } | ||
| tus->setTextureName(tex); |
There was a problem hiding this comment.
Copy generated maps beside the exported mesh before binding by basename.
Line 3812 strips generated map paths to filenames, but the files remain next to the albedo. When -o writes the mesh elsewhere, the exported material references texture files that are not in the output package.
Proposed fix
if (outputPath.isEmpty()) outputPath = meshPath;
const QFileInfo outFi(outputPath);
+
+ auto localizeGeneratedMap = [&](const QString& path) -> QString {
+ if (path.isEmpty()) return QString();
+ const QString dst = QDir(outFi.absolutePath())
+ .filePath(QFileInfo(path).fileName());
+ if (QFileInfo(path).absoluteFilePath()
+ == QFileInfo(dst).absoluteFilePath()) {
+ return path;
+ }
+ QFile::remove(dst);
+ if (!QFile::copy(path, dst)) {
+ err() << "Error: failed to copy generated PBR map to export directory: "
+ << dst << Qt::endl;
+ return QString();
+ }
+ return dst;
+ };
+ const QString normalPath = localizeGeneratedMap(res.normalPath);
+ const QString roughnessPath = localizeGeneratedMap(res.roughnessPath);
+ const QString heightPath = localizeGeneratedMap(res.heightPath);
+ if ((!res.normalPath.isEmpty() && normalPath.isEmpty())
+ || (!res.roughnessPath.isEmpty() && roughnessPath.isEmpty())
+ || (!res.heightPath.isEmpty() && heightPath.isEmpty())) {
+ return 1;
+ }
if (!initOgreHeadless()) return 1;
MeshImporterExporter::importer({meshFi.absoluteFilePath()});
@@
- // Register the albedo's directory so the generated PNGs resolve by name.
+ // Register the export directory so the generated PNGs resolve by name.
Ogre::ResourceGroupManager::getSingleton().addResourceLocation(
- QFileInfo(albedoPath).absolutePath().toStdString(), "FileSystem",
+ outFi.absolutePath().toStdString(), "FileSystem",
Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME);
@@
- bindSlot(pass, "normal_map", res.normalPath);
- bindSlot(pass, "roughness", res.roughnessPath);
+ bindSlot(pass, "normal_map", normalPath);
+ bindSlot(pass, "roughness", roughnessPath);Also applies to: 3827-3828
🤖 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/CLIPipeline.cpp` around lines 3788 - 3817, The generated map files are
being referenced by basename in the bindSlot lambda but the actual files remain
next to the albedo path rather than beside the exported mesh. Before calling
bindSlot with the generated map paths (the parameters passed to bindSlot in the
lines following the resource group manager setup), copy those generated texture
files from their current location to the output directory specified by
outputPath to ensure the exported material package includes all necessary
texture files alongside the mesh.
| PbrMapSynth::Options opts; | ||
| if (args.contains("normal")) opts.generateNormal = args["normal"].toBool(); | ||
| if (args.contains("roughness")) opts.generateRoughness = args["roughness"].toBool(); | ||
| if (args.contains("height")) opts.generateHeight = args["height"].toBool(); | ||
| if (args.contains("tile_size")) opts.tileSize = args["tile_size"].toInt(256); | ||
| if (args.contains("overwrite")) opts.overwriteCache = args["overwrite"].toBool(); |
There was a problem hiding this comment.
Mirror the CLI tile_size bounds in the MCP handler.
The CLI rejects values except 0 or 32..4096, but MCP forwards tile_size directly to synthesis. A bad MCP/HTTP request can drive invalid tiling and excessive allocation/work.
Proposed fix
PbrMapSynth::Options opts;
if (args.contains("normal")) opts.generateNormal = args["normal"].toBool();
if (args.contains("roughness")) opts.generateRoughness = args["roughness"].toBool();
if (args.contains("height")) opts.generateHeight = args["height"].toBool();
- if (args.contains("tile_size")) opts.tileSize = args["tile_size"].toInt(256);
+ if (args.contains("tile_size")) {
+ const QJsonValue tileValue = args.value("tile_size");
+ if (!tileValue.isDouble())
+ return makeErrorResult("'tile_size' must be an integer.");
+ const int tileSize = tileValue.toInt();
+ if (tileSize != 0 && (tileSize < 32 || tileSize > 4096)) {
+ return makeErrorResult(
+ "'tile_size' must be 0 (whole image) or between 32 and 4096.");
+ }
+ opts.tileSize = tileSize;
+ }
if (args.contains("overwrite")) opts.overwriteCache = args["overwrite"].toBool();🤖 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/MCPServer.cpp` around lines 1701 - 1706, The tile_size parameter handling
in the PbrMapSynth::Options configuration does not validate bounds, unlike the
CLI. Add bounds validation for the tile_size argument to ensure it is either 0
or falls within the range of 32 to 4096 before assigning it to opts.tileSize.
This prevents invalid values from being passed to the synthesis function that
could cause excessive allocation or work. Check the CLI implementation for the
exact validation logic to mirror, then apply the same bounds check in the MCP
handler where opts.tileSize is currently set without validation.
| auto info = ov.GetTensorTypeAndShapeInfo(); | ||
| auto sh = info.GetShape(); | ||
| const int oc = (sh.size() == 4) ? static_cast<int>(sh[1]) : 1; | ||
| const float* d = ov.GetTensorData<float>(); | ||
| if (oc >= 3 && out.normal.empty()) { | ||
| out.normal.assign(d, d + plane * 3); | ||
| } else if (oc == 1 && out.height.empty()) { | ||
| out.height.assign(d, d + plane); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current output-copy path uses input plane without output-size guard.
rg -n -C4 'GetTensorTypeAndShapeInfo|GetShape|GetTensorData<float>|assign\(d, d \+ plane' src/PbrMapSynth.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1148
🏁 Script executed:
#!/bin/bash
# Get the full function containing lines 236-244
rg -n -B30 'GetTensorTypeAndShapeInfo' src/PbrMapSynth.cpp | head -80Repository: fernandotonon/QtMeshEditor
Length of output: 2809
🏁 Script executed:
#!/bin/bash
# Check for other tensor element count operations in PbrMapSynth.cpp
rg -n 'GetElementCount|GetTensorData|assign|size()' src/PbrMapSynth.cpp | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 2574
🏁 Script executed:
#!/bin/bash
# Search for ONNX API usage patterns and GetElementCount calls
rg -n 'GetElementCount|GetTypeInfo|TensorTypeAndShapeInfo' src/PbrMapSynth.cppRepository: fernandotonon/QtMeshEditor
Length of output: 196
🏁 Script executed:
#!/bin/bash
# Check the full context of output tensor processing
sed -n '235,245p' src/PbrMapSynth.cppRepository: fernandotonon/QtMeshEditor
Length of output: 518
🏁 Script executed:
#!/bin/bash
# Look for other ONNX files or includes to understand API
fd -t f '\.(h|hpp)$' | xargs rg -l 'ort::|Ort::' | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Search for ONNX includes and see what methods are available
rg -n '`#include`.*onnx|ort\.h' src/PbrMapSynth.cppRepository: fernandotonon/QtMeshEditor
Length of output: 106
🌐 Web query:
ONNX Runtime C++ API GetTensorTypeAndShapeInfo GetElementCount method
💡 Result:
In the ONNX Runtime C++ API, the GetElementCount method is a member of the Ort::TensorTypeAndShapeInfo class [1][2]. It returns the total number of elements in a tensor [1][3]. Key technical details: - Function Signature: size_t GetElementCount const [1][2]. - Functionality: It acts as a wrapper for the underlying C API function OrtApi::GetTensorShapeElementCount [1][2]. - Return Value: It returns a size_t representing the product of the tensor's dimensions [1][4]. If the tensor has no shape (i.e., HasShape is false), it typically returns 0 [5][6]. - Usage Context: It is commonly used to determine the total number of elements in a tensor, which is helpful for memory allocation or iteration when accessing underlying tensor data [7][8][4]. Example usage: auto type_info = tensor.GetTensorTypeAndShapeInfo; size_t element_count = type_info.GetElementCount; std::cout << "Total elements: " << element_count << std::endl; [8]
Citations:
- 1: https://onnxruntime.ai/docs/api/c/struct_ort_1_1detail_1_1_tensor_type_and_shape_info_impl.html
- 2: https://onnxruntime.ai/docs/api/c/struct_ort_1_1_tensor_type_and_shape_info.html
- 3: https://tomwildenhain-microsoft.github.io/onnxruntime/docs/api/c/struct_ort_1_1_tensor_type_and_shape_info.html
- 4: https://github.com/microsoft/onnxruntime/blob/main/js/react_native/cpp/TensorUtils.cpp
- 5: https://github.com/microsoft/onnxruntime/blob/master/onnxruntime/core/framework/tensor_type_and_shape.cc
- 6: microsoft/onnxruntime@149d208
- 7: https://github.com/jaoming-affable/onnxruntime/blob/4090d0d0de040794f0ca181f780600f3b2b9834b/csharp/test/Microsoft.ML.OnnxRuntime.EndToEndTests.Capi/CXX_Api_Sample.cpp
- 8: https://microsoft-onnxruntime-40.mintlify.app/inference/c-cpp-api
🌐 Web query:
ONNX Runtime tensor shape validation H W dimensions NCHW format
💡 Result:
ONNX Runtime enforces tensor shape and rank validation as a core part of its execution process to ensure model integrity and compatibility [1][2]. Key aspects of ONNX Runtime tensor shape validation for NCHW and other formats include: 1. Mandatory Format Compliance: The ONNX specification natively uses the NCHW (Batch, Channel, Height, Width) data layout for most spatial operators, such as convolution and pooling [3][4]. If your model uses NHWC or another layout, ONNX Runtime generally expects these to be converted to NCHW, typically via Transpose operations, as it does not natively support NHWC in standard spatial operators [3][5][4]. 2. Rank and Shape Validation: - ONNX Runtime validates both the rank (number of dimensions) and the specific dimension sizes of input tensors against the model's metadata [6][7]. - If the input provided at runtime does not match the expected rank or dimensions defined in the ONNX model, an INVALID_ARGUMENT error is typically raised, terminating execution [6][8][7]. - While some versions or configurations may allow for flexible or symbolic dimensions (e.g., dynamic batch sizes), the rank must usually remain consistent with the model's definition [9][10][11]. - Scalar inputs (rank 0) may sometimes bypass strict rank validation in specific versions/scenarios, though this is considered inconsistent behavior [8]. 3. Inference-Time Checks: Validation occurs during the session's execution phase [12][2]. When running a model, ONNX Runtime verifies that the input tensor matches the shape constraints specified in the graph. Similarly, output shape validation is performed to ensure that the computed results conform to expected dimensions [12][2]. 4. Troubleshooting: If you encounter shape validation errors, ensure that: - Your input tensor dimensions are in the correct order (NCHW for spatial inputs) [3][5]. - The rank (number of axes) of your input exactly matches the model's requirements (e.g., a 2D matrix [N, C] vs. a 1D vector [N]) [6][7]. - You are using tools like Netron to inspect the actual input/output shapes and names defined in your ONNX model file to confirm they match your expected data [7]. In scenarios where custom execution providers (EPs) might transform tensors in ways that deviate from the standard model definition, configuration options such as session.disable_input_validation or session.disable_output_validation may be used, though these are discouraged as they bypass safety checks and place the responsibility of tensor sanity on the user [2].
Citations:
- 1: Enforce shape validation. microsoft/onnxruntime#1716
- 2: Strict Input Output Tensor Validation microsoft/onnxruntime#27986
- 3: https://oliviajain.github.io/onnxruntime/docs/performance/tune-performance.html
- 4: Conv and KERAS storage_order does not match - is this right onnx/onnx#4099
- 5: Resize bilinear + NHWC layout requires scales to be specified microsoft/onnxruntime#2687
- 6: https://onnxruntime.ai/docs/api/python/auto_examples/plot_common_errors.html
- 7: unclear error message: INVALID_ARGUMENT : Unexpected input data type. Actual: (N11onnxruntime17PrimitiveDataTypeIlEE) , expected: (N11onnxruntime17PrimitiveDataTypeIiEE) microsoft/onnxruntime#4787
- 8: InferenceSession.run does not validate rank of scalar inputs microsoft/onnxruntime#19434
- 9: Shape inference fails microsoft/onnxruntime#11614
- 10: onnx and onnxruntime disagree on input with no known rank microsoft/onnxruntime#11891
- 11: https://onnx.ai/onnx/repo-docs/IR.html
- 12: Output Tensor Shape Validation b/w ONNX inference and ORT microsoft/onnxruntime#7252
Validate ONNX output tensor shape and element count before copying data.
Lines 241 and 243 copy from the output tensor using plane (derived from input tile dimensions) without verifying the output tensor's spatial dimensions match. If the model outputs a different H/W, this causes buffer over-read or under-read, leading to memory corruption or crashes.
Validate that output shape matches input (sh[2] == h && sh[3] == w) and use GetElementCount() to ensure sufficient data exists before calling assign().
🤖 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/PbrMapSynth.cpp` around lines 236 - 244, In the code block where
out.normal.assign() and out.height.assign() are called, add validation before
copying data to ensure the output tensor's spatial dimensions match the input
dimensions and sufficient data exists. Specifically, after getting the shape
with info.GetShape(), verify that sh[2] equals the input height (h) and sh[3]
equals the input width (w), then use GetElementCount() to confirm the tensor has
enough elements before proceeding with the assign() calls for both the normal
and height cases. This prevents buffer over-read or under-read errors when the
model outputs different H/W dimensions.
…ts (#404) unit-tests-linux failed to link MaterialEditorQML_test: that target (in tests/CMakeLists.txt, distinct from the main UnitTests) keeps its OWN explicit source list, which compiled MaterialEditorQML.cpp / MCPServer.cpp / CLIPipeline.cpp — all of which now reference AIAssistManager/PbrMapSynth under ENABLE_ONNX (on in the CI coverage build) — without the two new .cpp files, giving "undefined reference to AIAssistManager::instance()" etc. Add PbrMapSynth.cpp + AIAssistManager.cpp to the qtmesh_test_common source list and link qtmesh_onnx into TEST_SUPPORT_LIBRARIES when ENABLE_ONNX, mirroring the ENABLE_LOCAL_LLM / ENABLE_SENTRY pattern. Verified locally (coverage config, ENABLE_ONNX=ON): both compile into qtmesh_test_common and the AIAssistManager undefined references are gone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adopt PBRify_Remix (CC0-1.0, Kim2091) as the model — three separate per-map SPAN models (Normal/Roughness/Height), the realistic permissive choice after DeepBump (GPL) was rejected. Verified end-to-end: the .pth models export to ONNX and produce correct maps via ONNX Runtime 1.20.1 (normal mean RGB ~(127,131,247) tangent-space blue; roughness/height proper grayscale). - scripts/export-pbrify-onnx.py: one-time offline dev tool (NOT shipped) that downloads the CC0 .pth models and exports each to ONNX (spandrel + torch.onnx.export, opset 18, dynamo=False, dynamic H/W). - PbrMapSynth: split the tiling/inference into runTiledModel() returning the full-res planar RGB result for ONE 3ch SPAN model; add decodeGrayscaleFromRgb (luminance of the RGB output, for roughness/height which emit RGB). synthesize() kept as a thin wrapper for the existing tests. - AIAssistManager: per-map model files/URLs (Map enum), download any missing via ModelDownloader from a configurable base URL (ai/pbrModelBaseUrl / QTMESH_PBR_MODEL_BASE_URL; empty until hosted). Each requested map runs its own model; roughness falls back to the offline luminance heuristic when its model is absent so roughness-only always works. - Tests: decodeGrayscaleFromRgb luminance case. - Docs: record the CC0 model choice, the .pth→ONNX export step, and that hosting the exported files is the only remaining gap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/export-pbrify-onnx.py`:
- Around line 32-38: The BASE_URL variable uses raw/main which is mutable and
not pinned to a specific commit, and the download function writes unverified
content directly to disk without integrity checks. Replace the mutable raw/main
reference in BASE_URL with a specific commit hash, then add checksum
verification to the download function that validates the downloaded file against
a known hash before writing it to disk. Apply these same changes to the similar
download code referenced at lines 82-87 to ensure all model artifacts are pinned
and verified.
🪄 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: 1bb2a2d6-4fe1-4ec8-b004-4e1e8a0390b5
📒 Files selected for processing (9)
.github/workflows/deploy.ymlCLAUDE.mdscripts/export-pbrify-onnx.pysrc/AIAssistManager.cppsrc/AIAssistManager.hsrc/PbrMapSynth.cppsrc/PbrMapSynth.hsrc/PbrMapSynth_test.cpptests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/PbrMapSynth.h
- src/AIAssistManager.h
- src/PbrMapSynth_test.cpp
- .github/workflows/deploy.yml
| BASE_URL = "https://github.com/Kim2091/PBRify_Remix/raw/main/Models/{name}.pth" | ||
|
|
||
|
|
||
| def download(name: str, dest: str) -> None: | ||
| url = BASE_URL.format(name=name) | ||
| print(f" downloading {url}") | ||
| urllib.request.urlretrieve(url, dest) |
There was a problem hiding this comment.
Pin and verify model artifacts before export.
Line 32 pulls from raw/main (mutable), and Line 38 writes unverified content to disk. This makes exports non-reproducible and opens a supply-chain tampering path for generated ONNX assets.
Suggested hardening patch
@@
import argparse
+import hashlib
import os
import sys
import urllib.request
@@
-BASE_URL = "https://github.com/Kim2091/PBRify_Remix/raw/main/Models/{name}.pth"
+# Pin to an immutable commit/tag rather than `main`.
+BASE_URL = "https://github.com/Kim2091/PBRify_Remix/raw/<PINNED_REF>/Models/{name}.pth"
+
+# Fill with known-good hashes for reproducible exports.
+MODEL_SHA256 = {
+ "1x-PBRify_NormalV3": "<sha256>",
+ "1x-PBRify_RoughnessV2": "<sha256>",
+ "1x-PBRify_Height": "<sha256>",
+}
@@
def download(name: str, dest: str) -> None:
@@
urllib.request.urlretrieve(url, dest)
+ expected = MODEL_SHA256[name]
+ h = hashlib.sha256()
+ with open(dest, "rb") as f:
+ for chunk in iter(lambda: f.read(1024 * 1024), b""):
+ h.update(chunk)
+ actual = h.hexdigest()
+ if actual != expected:
+ raise RuntimeError(f"SHA256 mismatch for {name}: {actual} != {expected}")Also applies to: 82-87
🧰 Tools
🪛 Ruff (0.15.17)
[error] 38-38: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 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 `@scripts/export-pbrify-onnx.py` around lines 32 - 38, The BASE_URL variable
uses raw/main which is mutable and not pinned to a specific commit, and the
download function writes unverified content directly to disk without integrity
checks. Replace the mutable raw/main reference in BASE_URL with a specific
commit hash, then add checksum verification to the download function that
validates the downloaded file against a known hash before writing it to disk.
Apply these same changes to the similar download code referenced at lines 82-87
to ensure all model artifacts are pinned and verified.
Source: Linters/SAST tools
…-404 # Conflicts: # .github/workflows/deploy.yml
|
1 participant
Implements #404 — predict normal + height maps from a single albedo/diffuse texture via an ONNX UNet, plus a roughness heuristic from albedo luminance. Lands ONNX Runtime as the project's first ONNX consumer.
Decisions (confirmed with maintainer)
ModelDownloaderfrom a configurable URL (QSettings ai/pbrModelUrl/QTMESH_PBR_MODEL_URL), empty by default until a permissive UNet host is chosen. The whole feature degrades gracefully offline.What's included
cmake/OnnxRuntime.cmake(ENABLE_ONNX, OFF by default) — downloads prebuilt ONNX Runtime 1.20.1 per-platform (verified SHA256), imported targetqtmesh_onnx. macOS universal2 (no per-arch trap; CoreML EP + CPU fallback). Runtime lib copied next to the binary/tests.PbrMapSynth(Ogre-free core, unit-tested) — NCHW packing, overlapping-tile inference with feathered seam blend, normal/height decode (strength + OpenGL/DirectXinvertG), roughness heuristic. Discovers model I/O shapes at runtime; derives normal-from-height via the existing SobelNormalMapGeneratorwhen the model emits only height.AIAssistManager(QML_SINGLETON) — model resolve/download/cache, synchronoussynthesizePbrMaps, slice-E slot binding, progress signals.generate_pbr_maps, CLIqtmesh material --texture <albedo> --generate-pbr [<mesh>] [-o] [--tile-size N] [--no-normal|--no-roughness|--no-height]. All bind normal/roughness into the canonical slice-E slots viaRTShaderHelper::wirePbrSlotsForFFP.ai.assist.pbr_synth.#404 acceptance criteria
AIAssistManager)ai.assist.pbr_synthTests
PbrMapSynth_test.cpp— GL/ONNX-free: NCHW round-trip, flat-normal→blue, invertG flip, height scaling, roughness heuristic. Verified locally via a standalone harness (gtest main needs GL on macOS).CLIPipeline_cmdmaterial_coverage_test.cpp—--generate-pbrerror/success contracts (each branches onENABLE_ONNX).-DENABLE_ONNX=ONon Linux.Known follow-ups (documented in CLAUDE.md)
ENABLE_ONNXstays OFF (MSVC-built archive won't link under MinGW); feature degrades to the "rebuild with -DENABLE_ONNX" message..dylib/.sois included in the packaged release bundles.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--generate-pbr, a new texture panel button, and an MCP tool.Documentation
Tests