Image→3D quality pass (#764): Taubin smoothing + iso-surface reprojection + baked diffuse texture (+ Real-ESRGAN chain) - #790
Merged
Conversation
…ker (xatlas texture bake) cores Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- MeshGenPredictor: Options {smoothMesh, smoothIterations, refineSurface,
bakeTexture, textureSize} (all ON by default) + Result {warning, uvs, texture}.
After MC: Taubin smooth -> iso-surface reprojection (4 decoder probes/vertex,
forward-difference gradient, one clamped Newton step) -> xatlas texture bake
(per-texel decoder colour) with vertex-colour fallback + cancellation support
in every decoder pass.
- MeshGenBuilder: textured path — VES_TEXTURE_COORDINATES + per-mesh lit
material with named diffuse_map TUS; saves the baked PNG into
AppData/generated_textures/ (or the export dir) and registers it as a
resource location so exporters resolve it.
- CLI: --no-smooth/--no-refine/--no-bake-texture/--texture-size; texture lands
next to the exported mesh. MCP: smooth/refine/bake_texture/texture_size args
+ schema. GUI: inherits ON defaults; status surfaces texture size / warnings.
- CMake: register MeshRefine + MeshGenBaker in app + test targets.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(TripoSG next) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… baked diffuse) Chains the #405 upscaler on the in-memory baked texture before it is saved, with graceful fallback warnings when the model is unavailable or the run fails. CLI-only surface for now; docs updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…output back)
The x2plus graph pixel-unshuffles its input by 2, so any odd-width/height tile
from the overlapping tiler failed inside the graph (Reshape {1,3,-1,2,W}).
Edge tiles are frequently odd — e.g. a 418px atlas produced a 179px tail tile
and the whole upscale aborted. Pad such tiles edge-replicated to even dims and
crop the model output back; even tiles are bit-identical. Found while chaining
--upscale-texture onto generate3d.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…_image Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds image-to-3D quality stages for mesh refinement and diffuse texture baking, threads the new outputs through predictor, builder, CLI, controller, MCP, and UI paths, adds texture upscaling support, and updates the related documentation. ChangesImage-to-3D quality pass
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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: 2040a30f1e
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/ImageTo3D/MeshGenPredictor.cpp (1)
448-449: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCross-file fragile string match for cancellation.
baked.error == QLatin1String("cancelled")depends onMeshGenBaker::bakenever changing that exact wording; see the companion comment onMeshGenBaker.cppsuggesting an explicitResult::cancelledboolean to make this contract robust to wording changes.🤖 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/ImageTo3D/MeshGenPredictor.cpp` around lines 448 - 449, The cancellation check in MeshGenPredictor::predict relies on matching the exact error string "cancelled", which is brittle across files. Update MeshGenBaker::bake and the related Result type to expose an explicit cancellation flag or status (for example a Result::cancelled field), then have MeshGenPredictor::predict use that structured signal instead of comparing baked.error text.
🧹 Nitpick comments (1)
src/CLIPipeline.cpp (1)
8859-8895: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate upscale wiring vs. MCPServer.cpp — extract a shared helper.
This "optional Real-ESRGAN 2x on the baked texture" block is duplicated almost verbatim in
MCPServer.cpp::toolGenerateMeshFromImage(lines 2199-2209). The two copies have already started to diverge (MCP drops the warning messages CLI emits), which is exactly the kind of drift shared logic prevents. This file already documents the precedent for this pattern:llmDescribeMaterialToEntityis called out as "#406shared core (CLI + MCP)" (seesrc/CLIPipeline.hlines 137-147). Consider factoring the upscale-and-fallback logic into a similar shared helper (e.g. inMeshGenPredictoror a small free function) that both surfaces call.🤖 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 8859 - 8895, The Real-ESRGAN texture upscale fallback logic in CLIPipeline::generateMeshFromImage is duplicated in MCPServer::toolGenerateMeshFromImage and has already diverged, so extract it into a shared helper used by both paths. Move the “ensureUpscaleModel(2) / TextureUpscaler::upscale / warning-or-fallback” behavior into a common function in MeshGenPredictor or a small shared utility, then have both CLIPipeline and MCPServer call that helper to keep warning handling and future changes consistent.
🤖 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 `@docs/IMAGE_TO_3D_QUALITY.md`:
- Around line 75-84: Add the missing blank lines around the “Rejected / parked”
markdown table in IMAGE_TO_3D_QUALITY.md so it passes markdownlint MD058. Update
the section containing the table under “3. Rejected / parked (license or
feasibility)” to ensure there is a blank line immediately before the table and
another immediately after it, keeping the surrounding prose separated from the
table.
In `@src/CLIPipeline.cpp`:
- Around line 8875-8889: The `--upscale-texture` path in `CLIPipeline::...` is
silently skipped when `res.texture` is null, so add an explicit warning when
`upscaleTex` is requested but no baked texture exists. Update the `if
(upscaleTex && !res.uvs.empty() && !res.texture.isNull())` flow to preserve the
current upscale behavior, but emit a clear message in the `else`/pre-check path
explaining that `--upscale-texture` had no effect because texture baking was
disabled or produced no texture. Use the existing `err()` logging style and keep
the message near the current texture-upscale block.
In `@src/ImageTo3D/MeshGenBaker.cpp`:
- Around line 20-34: Validate each element in indices against the vertex count
before calling xatlas in bake; the current checks only enforce non-empty and
triangle-aligned index counts, but do not prevent out-of-range accesses. Add an
explicit bounds guard in MeshGenBaker::bake, similar to
MeshGenBuilder::buildMesh, that rejects any index >= nv and returns a clear
error before the mesh is handed off.
- Around line 147-165: The failure exits in MeshGenBaker::bake are returning
after r.positions, r.indices, r.uvs, r.vertexCount, and r.triangleCount have
already been populated, which violates the documented “no partial data”
contract. Before returning from the empty-queryTexel branch and the
sampler-abort/cancelled branch, clear or reset the result object so it contains
no mesh data, and apply the same cleanup consistently in any other early-failure
path in bake.
- Around line 162-165: The cancellation path in MeshGenBaker is still encoded as
the string value in r.error, and MeshGenPredictor plus the related test are
matching that literal to detect aborts. Add an explicit boolean cancellation
flag to the bake result type used by MeshGenBaker::sampler and propagate it
through MeshGenPredictor::bake handling so cancelled work is checked via the
flag instead of comparing against "cancelled". Update MeshGenBaker_test to
assert the boolean cancellation state rather than the exact error string.
In `@src/ImageTo3D/MeshGenBuilder.cpp`:
- Around line 238-272: The texture-bake save path in MeshGenBuilder::buildMesh
can fail silently, causing the mesh to lose all colour/texture fallback while
the UI still implies a texture was generated. Handle the
result.texture.save(candidate, "PNG") failure explicitly by logging a breadcrumb
via SentryReporter::addBreadcrumb and surfacing the error to the caller/status
flow; if appropriate, also populate a per-vertex fallback from result.texture
and UVs so buildMesh does not drop colour information when texPath remains
empty.
In `@src/MCPServer.cpp`:
- Around line 2192-2221: Non-fatal warnings from mesh generation and texture
upscaling are being dropped before the MCP result is returned. In the
`MCPServer` flow that calls `MeshGenPredictor::predict` and later
`TextureUpscaler::upscale`, copy any `MeshGenPredictor::Result::warning` into
the response payload, and propagate upscale/model-availability warnings instead
of silently ignoring them. Make the `result` construction include these warnings
so the caller can see bake fallbacks and upscale no-ops.
---
Duplicate comments:
In `@src/ImageTo3D/MeshGenPredictor.cpp`:
- Around line 448-449: The cancellation check in MeshGenPredictor::predict
relies on matching the exact error string "cancelled", which is brittle across
files. Update MeshGenBaker::bake and the related Result type to expose an
explicit cancellation flag or status (for example a Result::cancelled field),
then have MeshGenPredictor::predict use that structured signal instead of
comparing baked.error text.
---
Nitpick comments:
In `@src/CLIPipeline.cpp`:
- Around line 8859-8895: The Real-ESRGAN texture upscale fallback logic in
CLIPipeline::generateMeshFromImage is duplicated in
MCPServer::toolGenerateMeshFromImage and has already diverged, so extract it
into a shared helper used by both paths. Move the “ensureUpscaleModel(2) /
TextureUpscaler::upscale / warning-or-fallback” behavior into a common function
in MeshGenPredictor or a small shared utility, then have both CLIPipeline and
MCPServer call that helper to keep warning handling and future changes
consistent.
🪄 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: 76cbe959-9fa1-44c7-839d-c1f5958de21d
📒 Files selected for processing (20)
.gitignoreCLAUDE.mddocs/IMAGE_TO_3D_QUALITY.mdsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/ImageTo3D/MeshGenBaker.cppsrc/ImageTo3D/MeshGenBaker.hsrc/ImageTo3D/MeshGenBaker_test.cppsrc/ImageTo3D/MeshGenBuilder.cppsrc/ImageTo3D/MeshGenBuilder.hsrc/ImageTo3D/MeshGenController.cppsrc/ImageTo3D/MeshGenPredictor.cppsrc/ImageTo3D/MeshGenPredictor.hsrc/ImageTo3D/MeshRefine.cppsrc/ImageTo3D/MeshRefine.hsrc/ImageTo3D/MeshRefine_test.cppsrc/MCPServer.cppsrc/TextureUpscaler.cpptests/CMakeLists.txt
- MeshGenBuilder: re-index the texture resource location on every save (a 2nd+ generation into an already-registered dir was invisible to Ogre's stale file index) + loud LogManager warning when the PNG save fails (nothing to fall back to — the bake replaced the vertex colours) - MeshGenBaker: validate index range up front; clear partial mesh data on the empty-coverage and sampler-abort paths (honours the documented contract); typed Result::cancelled flag replaces stringly-typed matching in the predictor; tests extended for all three - CLI: warn instead of silent no-op when --upscale-texture has no baked texture to work on - MCP: surface Result::warning (bake fallback) to the caller - docs: MD058 blank line before the rejected-alternatives table Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ImageTo3D/MeshGenPredictor.cpp (1)
394-462: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Sentry breadcrumbs for the new quality-pass stages.
The smoothing/reprojection, texture bake, bake fallback, and vertex-color fallback are significant user-facing generation operations, but this path does not add breadcrumbs. Add
SentryReporter::addBreadcrumb(...)at the pass boundaries, using an established category such asai.tool_call.As per coding guidelines,
src/**/*.{h,cpp,qml,js}must “AddSentryReporter::addBreadcrumb(category, message)for all user-facing actions and significant operations, using the established categories such asui.action,ai.tool_call,file.import, andfile.export.”🤖 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/ImageTo3D/MeshGenPredictor.cpp` around lines 394 - 462, The new quality-pass stages in MeshGenPredictor::generateMesh lack Sentry breadcrumbs for significant user-facing operations. Add SentryReporter::addBreadcrumb calls at the boundaries of the smoothing/reprojection pass, the texture bake attempt, the bake fallback path, and the per-vertex color fallback path, using an established category like ai.tool_call and clear stage-specific messages. Keep the calls near the existing if blocks around MeshRefine::taubinSmooth, MeshRefine::isoProjectStep, MeshGenBaker::bake, and the final vertex-color sampling branch.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.
Nitpick comments:
In `@src/ImageTo3D/MeshGenPredictor.cpp`:
- Around line 394-462: The new quality-pass stages in
MeshGenPredictor::generateMesh lack Sentry breadcrumbs for significant
user-facing operations. Add SentryReporter::addBreadcrumb calls at the
boundaries of the smoothing/reprojection pass, the texture bake attempt, the
bake fallback path, and the per-vertex color fallback path, using an established
category like ai.tool_call and clear stage-specific messages. Keep the calls
near the existing if blocks around MeshRefine::taubinSmooth,
MeshRefine::isoProjectStep, MeshGenBaker::bake, and the final vertex-color
sampling branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c1e8548-d11f-4d2e-be4f-61bb1ce6b204
📒 Files selected for processing (8)
docs/IMAGE_TO_3D_QUALITY.mdsrc/CLIPipeline.cppsrc/ImageTo3D/MeshGenBaker.cppsrc/ImageTo3D/MeshGenBaker.hsrc/ImageTo3D/MeshGenBaker_test.cppsrc/ImageTo3D/MeshGenBuilder.cppsrc/ImageTo3D/MeshGenPredictor.cppsrc/MCPServer.cpp
✅ Files skipped from review due to trivial changes (1)
- docs/IMAGE_TO_3D_QUALITY.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/ImageTo3D/MeshGenBaker_test.cpp
- src/MCPServer.cpp
- src/ImageTo3D/MeshGenBaker.h
- src/ImageTo3D/MeshGenBuilder.cpp
- src/ImageTo3D/MeshGenBaker.cpp
- src/CLIPipeline.cpp
…table stages everywhere The baked diffuse alone reads flat; running the #404 PBR synthesis on it (as the Material Editor button does) is what makes the result look like a polished final product. So: - MeshGenBuilder::BuildOptions {textureDir, generatePbrMaps}: after saving the baked diffuse, synthesize normal + roughness next to it (height skipped — no consumer), re-index the resource location so the new PNGs resolve, and bind them into the generated material with the exact Material-Editor recipe (normal_map/roughness TUS + wirePbrSlotsForFFP + applyNormalMap + recompile). Fails soft to diffuse-only when the models are unavailable. - ON by default on every surface, and every pipeline stage is now user-selectable like 'Remove background': * GUI: Inspector checkboxes (Smooth / Refine / Bake texture / PBR maps / Upscale 2x) via a shared InspectorCheck factory; options flow through MeshGenController::generateSelected(..., QVariantMap). The upscale now runs on the WORKER thread (model pre-ensured on the main thread). * CLI: --no-pbr (plus the existing --no-* flags). * MCP: generate_pbr arg + schema. Verified end-to-end: generate3d writes qtmesh_gen3d_N_diffuse/_normal/ _roughness.png next to the export and the glb material references all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ImageTo3D/MeshGenController.cpp (1)
371-381: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn the final status through
completed.The richer
statusMessagetext here is immediately overwritten byPropertiesPanel.qml’sonCompletedhandler, which formats only verts/tris. Include astatusTextor texture fields inoutand let QML render that final message.🤖 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/ImageTo3D/MeshGenController.cpp` around lines 371 - 381, The final mesh generation status is being emitted only via statusMessage, but PropertiesPanel.qml’s onCompleted handler overwrites it, so the richer text from MeshGenController::completion handling is lost. Update MeshGenController.cpp to include a final status payload in out (for example a statusText field, and texture-related fields when present) alongside the existing result data, then have the QML completion path use that value from completed instead of reconstructing a simpler verts/tris message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 1556-1567: The custom CheckBox indicator in PropertiesPanel.qml is
missing a visible keyboard focus state, so tab navigation provides no focus
ring. Update the CheckBox/indicator styling around the indicator Rectangle to
react to focus (or activeFocus) and draw a distinct focus outline or border when
focused, while keeping the current checked/unchecked appearance otherwise. Use
the existing PropertiesPanelController colors and the indicator
Rectangle/CheckBox component as the place to add the focus-visible behavior.
In `@src/ImageTo3D/MeshGenBuilder.cpp`:
- Around line 272-276: The PBR map synthesis path in
MeshGenBuilder::generatePbrMaps is a user-visible AI/material generation
operation but currently only logs failures, so add SentryReporter::addBreadcrumb
with the ai.tool_call category immediately before and/or after the
AIAssistManager::synthesizePbrMaps call. Keep the breadcrumb scoped to the
existing opts.generatePbrMaps block and use a descriptive message that
identifies the PbrMapSynth/sidecar generation step so the operation is traceable
even when synthesis succeeds.
In `@src/ImageTo3D/MeshGenController.cpp`:
- Around line 214-218: Normalize the dependent options in MeshGenController
before any model work starts: if bake_texture is false, force m_upscaleTexture
off (or ignore upscale_texture) so the upscale model is not downloaded/checked
when no UV texture can be produced. Also validate textureSize at this boundary
by clamping it to a safe supported range before it is used by the bake/upscale
worker paths (including the logic around the 242-249 block), rather than
accepting an unbounded public value.
- Around line 350-360: The default PBR path is blocking the GUI because
MeshGenBuilder::buildSceneNode is still doing synchronous PBR synthesis when
buildOpts.generatePbrMaps is enabled via m_generatePbr. Move that work off the
main thread (or make it explicitly asynchronous) so the default mesh load path
does not stall statusMessage updates, progress handling, or Cancel/repaint
responsiveness; keep the main-thread call limited to scene-node integration
after the background PBR work completes.
---
Outside diff comments:
In `@src/ImageTo3D/MeshGenController.cpp`:
- Around line 371-381: The final mesh generation status is being emitted only
via statusMessage, but PropertiesPanel.qml’s onCompleted handler overwrites it,
so the richer text from MeshGenController::completion handling is lost. Update
MeshGenController.cpp to include a final status payload in out (for example a
statusText field, and texture-related fields when present) alongside the
existing result data, then have the QML completion path use that value from
completed instead of reconstructing a simpler verts/tris message.
🪄 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: 29a3a58a-52b3-44fa-8ce9-aa17b5c502d9
📒 Files selected for processing (9)
CLAUDE.mdqml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/ImageTo3D/MeshGenBuilder.cppsrc/ImageTo3D/MeshGenBuilder.hsrc/ImageTo3D/MeshGenController.cppsrc/ImageTo3D/MeshGenController.hsrc/MCPServer.cpp
✅ Files skipped from review due to trivial changes (2)
- src/CLIPipeline.h
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/CLIPipeline.cpp
- src/MCPServer.cpp
Comment on lines
+1556
to
+1567
| indicator: Rectangle { | ||
| x: icRoot.leftPadding | ||
| y: icRoot.height / 2 - height / 2 | ||
| implicitWidth: 16 | ||
| implicitHeight: 16 | ||
| radius: 2 | ||
| color: icRoot.checked | ||
| ? PropertiesPanelController.highlightColor | ||
| : PropertiesPanelController.inputColor | ||
| border.color: PropertiesPanelController.borderColor | ||
| border.width: 1 | ||
| opacity: icRoot.enabled ? 1.0 : 0.45 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore a visible keyboard focus state.
The custom indicator always uses the same border, so tabbing through these new checkboxes gives no visible focus ring after replacing the native CheckBox visuals.
Proposed fix
- border.color: PropertiesPanelController.borderColor
- border.width: 1
+ border.color: icRoot.visualFocus
+ ? PropertiesPanelController.highlightColor
+ : PropertiesPanelController.borderColor
+ border.width: icRoot.visualFocus ? 2 : 1📝 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
| indicator: Rectangle { | |
| x: icRoot.leftPadding | |
| y: icRoot.height / 2 - height / 2 | |
| implicitWidth: 16 | |
| implicitHeight: 16 | |
| radius: 2 | |
| color: icRoot.checked | |
| ? PropertiesPanelController.highlightColor | |
| : PropertiesPanelController.inputColor | |
| border.color: PropertiesPanelController.borderColor | |
| border.width: 1 | |
| opacity: icRoot.enabled ? 1.0 : 0.45 | |
| indicator: Rectangle { | |
| x: icRoot.leftPadding | |
| y: icRoot.height / 2 - height / 2 | |
| implicitWidth: 16 | |
| implicitHeight: 16 | |
| radius: 2 | |
| color: icRoot.checked | |
| ? PropertiesPanelController.highlightColor | |
| : PropertiesPanelController.inputColor | |
| border.color: icRoot.visualFocus | |
| ? PropertiesPanelController.highlightColor | |
| : PropertiesPanelController.borderColor | |
| border.width: icRoot.visualFocus ? 2 : 1 | |
| opacity: icRoot.enabled ? 1.0 : 0.45 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@qml/PropertiesPanel.qml` around lines 1556 - 1567, The custom CheckBox
indicator in PropertiesPanel.qml is missing a visible keyboard focus state, so
tab navigation provides no focus ring. Update the CheckBox/indicator styling
around the indicator Rectangle to react to focus (or activeFocus) and draw a
distinct focus outline or border when focused, while keeping the current
checked/unchecked appearance otherwise. Use the existing
PropertiesPanelController colors and the indicator Rectangle/CheckBox component
as the place to add the focus-visible behavior.
Comment on lines
+272
to
+276
| if (opts.generatePbrMaps) { | ||
| PbrMapSynth::Options po; | ||
| po.generateHeight = false; | ||
| const PbrMapSynthResult pr = | ||
| AIAssistManager::instance()->synthesizePbrMaps(texPath, po); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a breadcrumb for PBR map synthesis.
This starts a user-visible AI/material generation step, but only the failure path logs to Ogre. Add a SentryReporter::addBreadcrumb("ai.tool_call", ...) around the synthesis call so the generated sidecar stage is traceable.
As per coding guidelines, **/*.{cpp,h,qml} files must “Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message) using the established categories such as ui.action, ai.tool_call, file.import, and file.export.”
Proposed fix
+#include "SentryReporter.h"
+
if (opts.generatePbrMaps) {
PbrMapSynth::Options po;
po.generateHeight = false;
+ SentryReporter::addBreadcrumb(
+ QStringLiteral("ai.tool_call"),
+ QStringLiteral("MeshGenBuilder synthesize PBR maps for %1")
+ .arg(QFileInfo(texPath).fileName()));
const PbrMapSynthResult pr =
AIAssistManager::instance()->synthesizePbrMaps(texPath, po);📝 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
| if (opts.generatePbrMaps) { | |
| PbrMapSynth::Options po; | |
| po.generateHeight = false; | |
| const PbrMapSynthResult pr = | |
| AIAssistManager::instance()->synthesizePbrMaps(texPath, po); | |
| if (opts.generatePbrMaps) { | |
| PbrMapSynth::Options po; | |
| po.generateHeight = false; | |
| SentryReporter::addBreadcrumb( | |
| QStringLiteral("ai.tool_call"), | |
| QStringLiteral("MeshGenBuilder synthesize PBR maps for %1") | |
| .arg(QFileInfo(texPath).fileName())); | |
| const PbrMapSynthResult pr = | |
| AIAssistManager::instance()->synthesizePbrMaps(texPath, po); |
🤖 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/ImageTo3D/MeshGenBuilder.cpp` around lines 272 - 276, The PBR map
synthesis path in MeshGenBuilder::generatePbrMaps is a user-visible AI/material
generation operation but currently only logs failures, so add
SentryReporter::addBreadcrumb with the ai.tool_call category immediately before
and/or after the AIAssistManager::synthesizePbrMaps call. Keep the breadcrumb
scoped to the existing opts.generatePbrMaps block and use a descriptive message
that identifies the PbrMapSynth/sidecar generation step so the operation is
traceable even when synthesis succeeds.
Source: Coding guidelines
Comment on lines
+214
to
+218
| const bool wantBake = optBool("bake_texture", true); | ||
| m_upscaleTexture = optBool("upscale_texture", false); | ||
| m_generatePbr = optBool("generate_pbr", true) && wantBake; | ||
| const int textureSize = options.contains(QLatin1String("texture_size")) | ||
| ? options.value(QLatin1String("texture_size")).toInt() : 1024; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Normalize dependent options before model work.
upscale_texture=true with bake_texture=false still downloads/checks the upscale model, then the worker skips upscaling because no UV texture exists. Also, texture_size is accepted unbounded at this public options boundary, which can drive expensive bake/upscale allocations.
Proposed fix
const bool wantSmooth = optBool("smooth", true);
const bool wantRefine = optBool("refine", true);
const bool wantBake = optBool("bake_texture", true);
- m_upscaleTexture = optBool("upscale_texture", false);
+ const bool wantUpscale = optBool("upscale_texture", false);
+ m_upscaleTexture = wantUpscale && wantBake;
m_generatePbr = optBool("generate_pbr", true) && wantBake;
const int textureSize = options.contains(QLatin1String("texture_size"))
? options.value(QLatin1String("texture_size")).toInt() : 1024;
+ if (textureSize < 64 || textureSize > 4096) {
+ emit error(tr("Texture size must be between 64 and 4096 pixels."));
+ return;
+ }Also applies to: 242-249
🤖 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/ImageTo3D/MeshGenController.cpp` around lines 214 - 218, Normalize the
dependent options in MeshGenController before any model work starts: if
bake_texture is false, force m_upscaleTexture off (or ignore upscale_texture) so
the upscale model is not downloaded/checked when no UV texture can be produced.
Also validate textureSize at this boundary by clamping it to a safe supported
range before it is used by the bake/upscale worker paths (including the logic
around the 242-249 block), rather than accepting an unbounded public value.
Comment on lines
+350
to
+360
| emit statusMessage(m_generatePbr && !r.uvs.empty() | ||
| ? tr("Building mesh + PBR maps…") | ||
| : tr("Building mesh…")); | ||
|
|
||
| // PBR synthesis (when enabled) runs inside buildSceneNode on this (main) | ||
| // thread — same as the Material Editor's button; the PBRify models are | ||
| // small and download on first use via the main-thread event loop. | ||
| MeshGenBuilder::BuildOptions buildOpts; | ||
| buildOpts.generatePbrMaps = m_generatePbr; | ||
| Ogre::SceneNode* node = | ||
| MeshGenBuilder::buildSceneNode(r, QStringLiteral("qtmesh_gen3d")); | ||
| MeshGenBuilder::buildSceneNode(r, QStringLiteral("qtmesh_gen3d"), buildOpts); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Move default-on PBR synthesis off the GUI thread.
buildSceneNode() synchronously calls PBR synthesis when generatePbrMaps is set, and m_generatePbr defaults on whenever baking is enabled. That makes the default GUI path block the main thread during model load/inference, so Cancel/progress/UI repaint cannot respond until it returns.
🤖 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/ImageTo3D/MeshGenController.cpp` around lines 350 - 360, The default PBR
path is blocking the GUI because MeshGenBuilder::buildSceneNode is still doing
synchronous PBR synthesis when buildOpts.generatePbrMaps is enabled via
m_generatePbr. Move that work off the main thread (or make it explicitly
asynchronous) so the default mesh load path does not stall statusMessage
updates, progress handling, or Cancel/repaint responsiveness; keep the
main-thread call limited to scene-node integration after the background PBR work
completes.
The single progress bar hid WHICH stage was running. Now:
- MeshGenPredictor::ProgressFn carries a typed Stage (Encode / Decode /
Refine / Bake / Color); every pass reports per-chunk progress under its
own stage (total<=0 = pure cancellation check). MeshGenBaker::Options
gained an optional progress callback — the baker owns the Bake stage's
accounting since only it knows the true texel total.
- MeshGenController maps typed stages onto the string stages the QML keys
on, wires the upscaler's per-tile progress ('upscale'), drops the stale
early 'encode' post that would have skipped the background row, and
pumps the event loop once before the blocking build+PBR step so the list
repaints.
- QML: the AI section now shows a per-step checklist built from the enabled
checkboxes at Generate time — ✓ done rows, a live mini-bar (pulsing when
indeterminate) on the active row, dimmed pending rows.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…un atlas overwrites
The generated mesh/texture names used a per-PROCESS counter, so a second CLI
run (or a new GUI session after a restart) into the same directory wrote the
same 'qtmesh_gen3d_1_diffuse.png' — OVERWRITING the previous generation's
baked atlas. The older mesh's UVs then pointed into the wrong atlas and it
rendered as a scrambled chart-patchwork ('texture not well mapped'), which
resolution appeared to trigger only because different runs landed on the same
file name. Ogre's TextureManager caching by name made the aliasing worse.
Append an epoch-ms token to the unique base so mesh, node, material, and all
texture sidecars are unique across processes and sessions. Verified: 256-res
and 512-res generations into the same directory now keep separate atlases and
both render correctly mapped.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ImageTo3D/MeshGenPredictor.cpp (1)
371-404: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
sampleBufferreports progress before the chunk is processed, so it never reaches 100% for a stage.Unlike the Decode loop above (which reports
done = start + nafter running the chunk), this reportsdone = startbefore running it. The last iteration'sstartis always< count, sodonenever equalscountfor Refine/Color (and anyreport=truecaller) — the per-step bar will visibly stall just short of full before the step flips to ✓.🐛 Proposed fix — mirror the Decode loop's post-chunk reporting
for (size_t start = 0; start < count; start += static_cast<size_t>(chunk)) { - if (progress) { - const bool keep = report - ? progress(stage, static_cast<int>(start), - static_cast<int>(count)) - : progress(stage, -1, -1); // cancel check only - if (!keep) return false; - } const size_t n = std::min(static_cast<size_t>(chunk), count - start); const int64_t ptShape[3] = {1, static_cast<int64_t>(n), 3}; Ort::Value ptTensor = Ort::Value::CreateTensor<float>( mem, const_cast<float*>(pts) + start * 3, n * 3, ptShape, 3); Ort::Value scTensor = Ort::Value::CreateTensor<float>( mem, sceneCodes.data(), sceneCodes.size(), scShape.data(), scShape.size()); const char* decIn[] = { decScName.get(), decPtName.get() }; Ort::Value decInVals[] = { std::move(scTensor), std::move(ptTensor) }; auto decRes = decoder.Run(Ort::RunOptions{nullptr}, decIn, decInVals, 2, decOutNames.data(), decOutNames.size()); if (outDensity) { const float* dens = decRes[densityIdx].GetTensorData<float>(); std::copy(dens, dens + n, outDensity + start); } if (outRgb && colorIdx >= 0) { const float* col = decRes[colorIdx].GetTensorData<float>(); std::copy(col, col + n * 3, outRgb + start * 3); } + if (progress) { + const bool keep = report + ? progress(stage, static_cast<int>(start + n), + static_cast<int>(count)) + : progress(stage, -1, -1); // cancel check only + if (!keep) return false; + } }🤖 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/ImageTo3D/MeshGenPredictor.cpp` around lines 371 - 404, The sampleBuffer progress reporting in MeshGenPredictor::sampleBuffer is using the chunk start index before processing, so the stage never reports completion at 100%. Update the progress call to mirror the Decode loop by reporting after each chunk is processed using the completed amount (start + n) rather than start, while keeping the cancel-only path unchanged for report=false callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 1800-1830: The outgoing request from PropertiesPanel.qml is
sending generate_pbr and upscale_texture directly from mgPbr.checked and
mgUpscale.checked even when mgBake.checked is false, which can conflict with the
build-step logic. Update the payload passed to
MeshGenController.generateSelected so these flags are only true when
bake_texture is enabled, mirroring the existing step label gating around
mgBake.checked. Use the existing mgPbr, mgUpscale, and mgBake checks in the
onClicked handler to keep the request consistent with the UI state.
- Around line 1919-1933: The onProgress handler in PropertiesPanel.qml is
resetting mgActiveProgress to indeterminate whenever total <= 0, which breaks
the “cancel-check only” contract and can cause Bake-stage flicker. Update
onProgress so it only assigns mgActiveProgress when total > 0, and ignore total
<= 0 calls entirely for the active stage; keep the existing stage lookup and
mgActiveIdx transition logic in sync with this behavior.
---
Outside diff comments:
In `@src/ImageTo3D/MeshGenPredictor.cpp`:
- Around line 371-404: The sampleBuffer progress reporting in
MeshGenPredictor::sampleBuffer is using the chunk start index before processing,
so the stage never reports completion at 100%. Update the progress call to
mirror the Decode loop by reporting after each chunk is processed using the
completed amount (start + n) rather than start, while keeping the cancel-only
path unchanged for report=false callers.
🪄 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: dd9477ab-f1a6-4bf0-946c-35e82e315d20
📒 Files selected for processing (7)
qml/PropertiesPanel.qmlsrc/ImageTo3D/MeshGenBaker.cppsrc/ImageTo3D/MeshGenBaker.hsrc/ImageTo3D/MeshGenBuilder.cppsrc/ImageTo3D/MeshGenController.cppsrc/ImageTo3D/MeshGenPredictor.cppsrc/ImageTo3D/MeshGenPredictor.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/ImageTo3D/MeshGenBaker.h
- src/ImageTo3D/MeshGenBuilder.cpp
- src/ImageTo3D/MeshGenController.cpp
- src/ImageTo3D/MeshGenBaker.cpp
Comment on lines
+1800
to
+1830
| onClicked: { | ||
| var steps = [{ key: "prep", label: "Prepare models" }] | ||
| if (mgRemoveBg.checked) | ||
| steps.push({ key: "background", label: "Remove background" }) | ||
| steps.push({ key: "encode", label: "Encode image" }) | ||
| steps.push({ key: "decode", label: "Reconstruct 3D" }) | ||
| if (mgRefine.checked) | ||
| steps.push({ key: "refine", label: "Refine surface" }) | ||
| if (mgBake.checked) | ||
| steps.push({ key: "bake", label: "Bake texture" }) | ||
| else | ||
| steps.push({ key: "color", label: "Vertex colors" }) | ||
| if (mgUpscale.checked && mgBake.checked) | ||
| steps.push({ key: "upscale", label: "Upscale texture 2×" }) | ||
| steps.push({ key: "build", | ||
| label: (mgPbr.checked && mgBake.checked) | ||
| ? "Build mesh + PBR maps" : "Build mesh" }) | ||
| mgRoot.mgSteps = steps | ||
| mgRoot.mgActiveIdx = 0 | ||
| mgRoot.mgActiveProgress = -1 | ||
|
|
||
| MeshGenController.generateSelected( | ||
| mgResCombo.resValue, mgRemoveBg.checked, mgQualityCombo.currentIndex, | ||
| { | ||
| "smooth": mgSmooth.checked, | ||
| "refine": mgRefine.checked, | ||
| "bake_texture": mgBake.checked, | ||
| "generate_pbr": mgPbr.checked, | ||
| "upscale_texture": mgUpscale.checked | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
generate_pbr/upscale_texture aren't gated by bake_texture in the outgoing request, unlike the step-list label right above.
Line 1815 correctly computes the "build" step label as (mgPbr.checked && mgBake.checked), acknowledging PBR/upscale depend on baking. But mgPbr/mgUpscale stay checked (just visually disabled) if the user unchecks "Bake diffuse texture" after having checked them — and the payload below sends their raw checked values regardless of mgBake.checked, so generate_pbr/upscale_texture could be sent true while bake_texture is false.
♻️ Proposed fix
MeshGenController.generateSelected(
mgResCombo.resValue, mgRemoveBg.checked, mgQualityCombo.currentIndex,
{
"smooth": mgSmooth.checked,
"refine": mgRefine.checked,
"bake_texture": mgBake.checked,
- "generate_pbr": mgPbr.checked,
- "upscale_texture": mgUpscale.checked
+ "generate_pbr": mgPbr.checked && mgBake.checked,
+ "upscale_texture": mgUpscale.checked && mgBake.checked
})📝 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
| onClicked: { | |
| var steps = [{ key: "prep", label: "Prepare models" }] | |
| if (mgRemoveBg.checked) | |
| steps.push({ key: "background", label: "Remove background" }) | |
| steps.push({ key: "encode", label: "Encode image" }) | |
| steps.push({ key: "decode", label: "Reconstruct 3D" }) | |
| if (mgRefine.checked) | |
| steps.push({ key: "refine", label: "Refine surface" }) | |
| if (mgBake.checked) | |
| steps.push({ key: "bake", label: "Bake texture" }) | |
| else | |
| steps.push({ key: "color", label: "Vertex colors" }) | |
| if (mgUpscale.checked && mgBake.checked) | |
| steps.push({ key: "upscale", label: "Upscale texture 2×" }) | |
| steps.push({ key: "build", | |
| label: (mgPbr.checked && mgBake.checked) | |
| ? "Build mesh + PBR maps" : "Build mesh" }) | |
| mgRoot.mgSteps = steps | |
| mgRoot.mgActiveIdx = 0 | |
| mgRoot.mgActiveProgress = -1 | |
| MeshGenController.generateSelected( | |
| mgResCombo.resValue, mgRemoveBg.checked, mgQualityCombo.currentIndex, | |
| { | |
| "smooth": mgSmooth.checked, | |
| "refine": mgRefine.checked, | |
| "bake_texture": mgBake.checked, | |
| "generate_pbr": mgPbr.checked, | |
| "upscale_texture": mgUpscale.checked | |
| }) | |
| } | |
| MeshGenController.generateSelected( | |
| mgResCombo.resValue, mgRemoveBg.checked, mgQualityCombo.currentIndex, | |
| { | |
| "smooth": mgSmooth.checked, | |
| "refine": mgRefine.checked, | |
| "bake_texture": mgBake.checked, | |
| "generate_pbr": mgPbr.checked && mgBake.checked, | |
| "upscale_texture": mgUpscale.checked && mgBake.checked | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@qml/PropertiesPanel.qml` around lines 1800 - 1830, The outgoing request from
PropertiesPanel.qml is sending generate_pbr and upscale_texture directly from
mgPbr.checked and mgUpscale.checked even when mgBake.checked is false, which can
conflict with the build-step logic. Update the payload passed to
MeshGenController.generateSelected so these flags are only true when
bake_texture is enabled, mirroring the existing step label gating around
mgBake.checked. Use the existing mgPbr, mgUpscale, and mgBake checks in the
onClicked handler to keep the request consistent with the UI state.
Comment on lines
1919
to
1933
| function onProgress(stage, done, total) { | ||
| if (total > 0 && done >= 0) { | ||
| mgProgress.indeterminate = (stage === "prep" || stage === "background") | ||
| mgProgress.value = total > 0 ? (done / total) : 0 | ||
| // Advance the step list. Stages not in the list (e.g. the | ||
| // vertex-colour fallback after a failed bake) are ignored. | ||
| var idx = -1 | ||
| for (var i = 0; i < mgRoot.mgSteps.length; i++) | ||
| if (mgRoot.mgSteps[i].key === stage) { idx = i; break } | ||
| if (idx < 0) | ||
| return | ||
| if (idx > mgRoot.mgActiveIdx) { | ||
| mgRoot.mgActiveIdx = idx | ||
| mgRoot.mgActiveProgress = -1 | ||
| } | ||
| if (idx === mgRoot.mgActiveIdx) | ||
| mgRoot.mgActiveProgress = total > 0 ? done / total : -1 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
onProgress doesn't honor the "total<=0 = no bar update" contract, risking flicker on the Bake stage.
MeshGenPredictor::ProgressFn documents total <= 0 as a pure cancellation check that should not affect the bar. Here, any call for the active stage unconditionally sets mgActiveProgress = total > 0 ? done / total : -1. For Stage::Bake specifically, the C++ side feeds two producers into this same stage: the baker's own accurate texel progress (bakeOpts.progress) and the color-sampler's cancel-only pings (sampleBuffer(..., report=false), always (-1,-1)). Every cancel-only ping will reset a real, in-progress fraction back to indeterminate, causing the bake progress bar to flicker between a real percentage and the pulsing/indeterminate state.
🐛 Proposed fix
if (idx > mgRoot.mgActiveIdx) {
mgRoot.mgActiveIdx = idx
mgRoot.mgActiveProgress = -1
}
- if (idx === mgRoot.mgActiveIdx)
- mgRoot.mgActiveProgress = total > 0 ? done / total : -1
+ // total <= 0 is a pure cancellation-check ping (see
+ // MeshGenPredictor::ProgressFn) — don't clobber a real
+ // in-progress fraction with indeterminate on those.
+ if (idx === mgRoot.mgActiveIdx && total > 0)
+ mgRoot.mgActiveProgress = done / total📝 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
| function onProgress(stage, done, total) { | |
| if (total > 0 && done >= 0) { | |
| mgProgress.indeterminate = (stage === "prep" || stage === "background") | |
| mgProgress.value = total > 0 ? (done / total) : 0 | |
| // Advance the step list. Stages not in the list (e.g. the | |
| // vertex-colour fallback after a failed bake) are ignored. | |
| var idx = -1 | |
| for (var i = 0; i < mgRoot.mgSteps.length; i++) | |
| if (mgRoot.mgSteps[i].key === stage) { idx = i; break } | |
| if (idx < 0) | |
| return | |
| if (idx > mgRoot.mgActiveIdx) { | |
| mgRoot.mgActiveIdx = idx | |
| mgRoot.mgActiveProgress = -1 | |
| } | |
| if (idx === mgRoot.mgActiveIdx) | |
| mgRoot.mgActiveProgress = total > 0 ? done / total : -1 | |
| } | |
| function onProgress(stage, done, total) { | |
| // Advance the step list. Stages not in the list (e.g. the | |
| // vertex-colour fallback after a failed bake) are ignored. | |
| var idx = -1 | |
| for (var i = 0; i < mgRoot.mgSteps.length; i++) | |
| if (mgRoot.mgSteps[i].key === stage) { idx = i; break } | |
| if (idx < 0) | |
| return | |
| if (idx > mgRoot.mgActiveIdx) { | |
| mgRoot.mgActiveIdx = idx | |
| mgRoot.mgActiveProgress = -1 | |
| } | |
| // total <= 0 is a pure cancellation-check ping (see | |
| // MeshGenPredictor::ProgressFn) — don't clobber a real | |
| // in-progress fraction with indeterminate on those. | |
| if (idx === mgRoot.mgActiveIdx && total > 0) | |
| mgRoot.mgActiveProgress = done / total | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@qml/PropertiesPanel.qml` around lines 1919 - 1933, The onProgress handler in
PropertiesPanel.qml is resetting mgActiveProgress to indeterminate whenever
total <= 0, which breaks the “cancel-check only” contract and can cause
Bake-stage flicker. Update onProgress so it only assigns mgActiveProgress when
total > 0, and ignore total <= 0 calls entirely for the active stage; keep the
existing stage lookup and mgActiveIdx transition logic in sync with this
behavior.
… Archive crash on generated-mesh reload User-reported: exporting a generated mesh and reloading it in the same session crashed the app (.mesh reliably, others intermittently). Reproduced under lldb: EXC_BAD_ACCESS in Ogre::ResourceGroupManager::openResourceImpl dereferencing a freed Archive* — the same signature as the long-standing 'known GL/Xvfb' CI crashes in the CLI coverage suites. Mechanism: the same directory is routinely registered in MULTIPLE resource groups (the import path registers it under a dir-named group AND DEFAULT; MeshGenBuilder registers texture dirs under DEFAULT). Ogre's ArchiveManager shares ONE Archive instance per path across groups, and removeResourceLocation DESTROYS it — every other group's file index then points at freed memory and the next openResource crashes (nondeterministically, depending on which index entry is hit). Fix: refresh indexes by ADDING the location again — a re-add re-lists the directory into the group's index while ArchiveManager reuses the same Archive — in both MeshGenBuilder (texture dir) and MeshImporterExporter's registerImportDirectory (which also gains the missing DEFAULT-group re-initialise). Verified: the previously-crashing session sequence (generate → export .mesh → reload; glb reimport; older-session .mesh files; obj; second generation; fbx reload — 8 entities) now completes alive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ality-786 # Conflicts: # src/MCPServer.cpp
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Quality pass for AI Image→3D (#764): closes the visible gap between our TripoSR output and commercial services (Meshy/Tripo3D-class polish) — all local, all permissive-license. Grown over review into a full polish pipeline with user-selectable stages, per-step progress, and two crash/correctness fixes found while dogfooding.
The pipeline (each stage user-selectable, defaults in brackets)
MeshRefine::taubinSmooth, λ|μ alternating Laplacian (volume-preserving). Kills the marching-cubes stair-stepping.MeshRefine::isoProjectStep, one Newton step per vertex back onto the decoder's true zero level set (forward-difference gradients from 4 extra decoder probes/vertex). Recovers grid-quantized detail.MeshGenBaker: xatlas auto-unwrap → UV-space rasterization → per-texel decoder colour → chart dilation. Real UV0 + texture instead of per-vertex colour (which didn't survive export — rendered flat white through a glb round-trip).normal_map/roughnessslots + FFP wiring + RTSS normal-map SRS + recompile). This is what turns the flat diffuse result into a polished final product.Surfaces
--no-smooth --no-refine --no-bake-texture --no-pbr --texture-size N --upscale-texture; sidecars (*_diffuse/_normal/_roughness.png) land next to the export.smooth / refine / bake_texture / generate_pbr / texture_size / upscale_textureargs + schema; non-fatal degradations surface via awarningfield.Bugs found & fixed along the way
Archive*crash on generated-mesh reload (user-reported; reproduced under lldb): the same directory gets registered in multiple resource groups, Ogre shares oneArchiveper path, andremoveResourceLocationdestroys it — other groups' file indexes then dangle and the nextopenResourcedies inResourceGroupManager::openResourceImpl. Same signature as the long-standing "known GL/Xvfb" CI crashes. Fixed by refreshing indexes via re-addResourceLocation(re-lists without destroying) inMeshGenBuilder+MeshImporterExporter::registerImportDirectory.qtmesh_gen3d_1_diffuse.pngand older meshes' UVs pointed into the wrong atlas. Names now carry an epoch-ms token — globally unique.Tests
MeshRefine_test.cpp(noise reduction w/o volume collapse, analytic-sphere projection, clamping, degenerate safety) +MeshGenBaker_test.cpp(position-encoded-colour bake correctness, index validation, no-partial-data + typed cancellation) — pure-data, run on Linux CI. Verified end-to-end on macOS across glb/FBX/.mesh round-trips (FBX embeds the textures viaVideo.Content).Known follow-ups
docs/IMAGE_TO_3D_QUALITY.md+ prepared issue body in.triposg_issue_body.md.🤖 Generated with Claude Code
Summary by CodeRabbit
--texture-size(64–8192) and optional 2x upscaling via--upscale-texture.--no-smooth/--no-refine/--no-bake-texture/--no-pbr.