feat(lod): meshoptimizer-backed LOD generator (#398) - #680
Merged
Conversation
📝 WalkthroughWalkthroughAdds meshoptimizer as an alternative LOD backend with a MeshOptimizerLod implementation and tests, wires meshoptimizer into the build, exposes an Algorithm selector across MeshLodController/MeshDecimator, and surfaces backend choice via CLI (--algo), MCP, and QML; fixes cached face-binding issues during LOD export. ChangesMeshoptimizer-backed LOD generation with algorithm selection
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5184c8c3be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+224
to
+228
| if (reduction <= 0.0f || reduction >= 1.0f) { | ||
| if (logger) logger->logMessage( | ||
| "[MeshOptimizerLod] skipping reduction " + std::to_string(reduction) + | ||
| " — must be in (0,1)"); | ||
| continue; |
There was a problem hiding this comment.
When the default meshopt backend is used with count == 4 and no explicit reductions, MeshLodController::generateLods supplies the legacy fallback 0.25f * (i + 1), so the fourth requested level is 1.0. This guard then skips that level entirely, meaning qtmesh lod model --count 4/MCP/UI requests can silently create only three reduced LODs while still reporting success for the requested count. Since the public API allows 1–4 levels, clamp the generated fallback below 1.0 or otherwise handle the 100% case instead of dropping it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/CLIPipeline.cpp`:
- Line 2360: CLIPipeline::printUsage() still shows the old "lod" syntax; update
its usage/help string to match the revised error message (the new "qtmesh lod
<file> --count N [--reductions r,...] [--algo meshopt|ogre] [-o output]" form)
so the --help output advertises the new flags and options consistently; locate
CLIPipeline::printUsage() and replace or augment the printed "lod" usage line to
include the --count, --reductions, --algo, and -o options and ensure formatting
matches the err() message.
- Around line 2339-2347: The parser currently accepts --algo regardless of the
chosen operation so flags like "--auto" or "--info" can silently ignore the
requested algorithm; modify the argument handling around the arg == "--algo"
branch to validate that the current operation/mode (the variable controlling
paths for --auto, --info, --remove, --count) is the one that uses the algorithm
(only the explicit "--count" path) and otherwise emit an error and return
non-zero; specifically, in the block that reads argv[++i] into algo (the const
QString val / algo assignment), add a guard that checks the mode (or the flags
that set auto/info/remove/count) and rejects --algo unless the mode is count,
mirroring the existing validation for allowed algo values, and apply the same
change to the other occurrence of this branch later in the file.
In `@src/MeshLodController.cpp`:
- Around line 157-161: The shared generateLods() currently emits a
caller-category breadcrumb using SentryReporter::addBreadcrumb based on the
algo, which misclassifies events because generateLods() is invoked from both the
Inspector (UI) and MCP paths; remove or stop emitting caller-specific
breadcrumbs from generateLods() and instead add
SentryReporter::addBreadcrumb("ui.action", ...) at the Inspector UI/menu/toolbar
entry point that calls generateLods() and add
SentryReporter::addBreadcrumb("ai.tool_call", ...) at the MCP tool-invocation
entry point; keep backend details (algoName, LOD count) in the message payload
if desired but only emit the category-specific breadcrumb at the actual callers.
- Around line 183-187: The controller currently allows explicit or fallback
reduction values to reach 1.0f, which can drop the last LOD; in the method that
builds the reductions vector (the loop using variables count, reductions and
r.push_back, calling reductions[i].toFloat()), ensure both the explicit branch
and the fallback branch clamp values strictly below 1.0 (e.g. use std::min(...,
a value < 1.0f) or nextafter(1.0f, 0.0f) instead of 1.0f) and keep the existing
lower bound (e.g. 0.01f); apply the same change to the second similar block
later in this file so both paths obey the same contract.
In `@src/MeshOptimizerLod_test.cpp`:
- Line 101: Replace the GTEST_SKIP prerequisite checks with hard assertions so
failures surface: in MeshOptimizerLod_test.cpp change the precondition lines
that call canLoadMeshFiles() and any GL/OGRE checks to use
ASSERT_TRUE(tryInitOgre()) followed by ASSERT_TRUE(canLoadMeshFiles()) instead
of GTEST_SKIP(); apply this replacement in all three test cases referenced (the
lines currently using GTEST_SKIP(), e.g., the check that calls
canLoadMeshFiles()), ensuring the tests call tryInitOgre() first then
canLoadMeshFiles() via ASSERT_TRUE so missing prerequisites cause test failures.
In `@src/MeshOptimizerLod.cpp`:
- Around line 241-278: The simplify path calls meshopt_simplify and
meshopt_simplifyWithAttributes with src.positions.data(), src.vertexCount and
src.indices even when positions may be empty or vertexCount is zero; add a guard
before calling these functions (and before any subsequent vertex-cache
optimization) that verifies src.positions is non-empty, src.vertexCount > 0, and
src.indices.size() >= 3; if the check fails, skip simplification/vertex-cache
optimize for that source (leave simplified/ newIdxCount as-is or copy original
indices) to avoid passing invalid vertex_positions to meshopt. Reference the
loop over sources, the local variable src, and the functions meshopt_simplify
and meshopt_simplifyWithAttributes when adding the validation.
🪄 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: 2ab64ab1-e83a-4a0f-91f2-55d69440fc56
📒 Files selected for processing (12)
CLAUDE.mdCMakeLists.txtqml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MeshLodController.cppsrc/MeshLodController.hsrc/MeshOptimizerLod.cppsrc/MeshOptimizerLod.hsrc/MeshOptimizerLod_test.cpptests/CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MeshLodController.h (1)
48-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocumentation contradicts implementation.
The comment says "anything else falls back to meshopt with a warning", but the implementation in
generateLodsWithAlgo(line 145-147 of the .cpp) falls back toAlgorithm::Ogrefor anything other than"meshopt". Update the comment to match the actual behavior.📝 Suggested fix
// QML-facing backend-selector variant. `algo` is "meshopt" or - // "ogre"; anything else falls back to meshopt with a warning. + // "ogre"; anything else falls back to ogre. Q_INVOKABLE void generateLodsWithAlgo(int count, QVariantList reductions, const QString& algo);🤖 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/MeshLodController.h` around lines 48 - 51, Update the documentation comment for the Q_INVOKABLE method generateLodsWithAlgo to reflect the actual implementation: state that `algo` should be "meshopt" or "ogre" and that any other value falls back to Algorithm::Ogre (i.e. ogre) with a warning, so change the phrase "falls back to meshopt" to "falls back to ogre" and keep the rest of the comment intact; refer to generateLodsWithAlgo for the location to modify.src/CLIPipeline.cpp (1)
2339-2347:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject bare
--algoflags.Both parsers silently keep the default backend when
--algois the last token. Commands likeqtmesh lod model.fbx --count 2 --algoandqtmesh decimate model.fbx -o out.fbx --reduction 0.5 --algoshould fail instead of unexpectedly running Ogre.Suggested fix
- if (arg == "--algo" && i + 1 < argc) { + if (arg == "--algo") { + if (i + 1 >= argc) { + err() << "Error: --algo requires a value ('meshopt' or 'ogre')." << Qt::endl; + return 2; + } const QString val = QString(argv[++i]).toLower(); if (val != "meshopt" && val != "ogre") { err() << "Error: --algo must be 'meshopt' or 'ogre' (got '" << val << "')." << Qt::endl; return 2; } algo = val; continue; }- if (arg == "--algo" && i < argc) { + if (arg == "--algo") { + if (i >= argc) { + err() << "Error: --algo requires a value ('ogre' or 'meshopt')." << Qt::endl; + return 0; + } const QString val = QString::fromLocal8Bit(argv[i++]).toLower(); if (val != "ogre" && val != "meshopt") { err() << "Error: --algo must be 'ogre' or 'meshopt' (got '" << val << "')." << Qt::endl; return 0; } out.algo = val; return 1; }Also applies to: 4630-4638
🤖 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 2339 - 2347, The code currently accepts a bare "--algo" at end-of-command by skipping the handler and keeping the default; change the handler in the CLI parsing block that references arg and sets algo so that it always matches "--algo" and explicitly checks for a following token: if "--algo" is present but i+1 >= argc, call err() with a clear message and return error code (same style as the existing message) instead of silently continuing; when present, validate the next token (QString(argv[++i]).toLower()) as before and assign to algo. Apply the same change to the other parser block around the symbols referenced at lines 4630-4638 so both parsers reject bare "--algo".
🤖 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 2771-2773: The LOD backend defaults to "ogre" because model:
["ogre", "meshopt"] with currentIndex: 0 selects the first entry; change the
selection so meshopt is the default by either reordering the model to
["meshopt", "ogre"] or setting currentIndex to the index of "meshopt" (e.g.,
currentIndex: 1), updating the properties where model and currentIndex are
defined in the PropertiesPanel component.
- Around line 2923-2933: The comment stating the LOD backend default is
"meshopt" is inconsistent with the UI: update the comment and/or the default
selection so they match the actual control; specifically, either change the
explanatory comment near the LOD backend selector to indicate the default is
"ogre" (since lodBackendCombo uses model: ["ogre","meshopt"] and currentIndex:
0) or set lodBackendCombo.currentIndex to 1 to make "meshopt" the default; also
remove or amend the incorrect note about palette being an invalid API for the
Column (palette inside Column id: decimateContent is valid in Qt6) so the
comment no longer blocks the palette usage.
In `@src/MCPServer.cpp`:
- Line 2752: The default algorithm string is still set to "ogre" in several
places (e.g., where args["algo"] is parsed); change those defaults to "meshopt"
so meshoptimizer becomes the default backend. Locate each occurrence of the algo
parsing logic (instances that read args["algo"].toString().toLower(), the
MCPServer methods handling request options, and any fallback assignments around
the symbols like args["algo"] and defaultAlgo) and replace the literal
QStringLiteral("ogre") (and any equivalent "ogre" default) with
QStringLiteral("meshopt") (or the string "meshopt") ensuring casing is
normalized to lower-case as done currently. Verify all listed occurrences (the
blocks around the current defaults) are updated so callers without an explicit
algo choice will use meshopt.
---
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 2339-2347: The code currently accepts a bare "--algo" at
end-of-command by skipping the handler and keeping the default; change the
handler in the CLI parsing block that references arg and sets algo so that it
always matches "--algo" and explicitly checks for a following token: if "--algo"
is present but i+1 >= argc, call err() with a clear message and return error
code (same style as the existing message) instead of silently continuing; when
present, validate the next token (QString(argv[++i]).toLower()) as before and
assign to algo. Apply the same change to the other parser block around the
symbols referenced at lines 4630-4638 so both parsers reject bare "--algo".
In `@src/MeshLodController.h`:
- Around line 48-51: Update the documentation comment for the Q_INVOKABLE method
generateLodsWithAlgo to reflect the actual implementation: state that `algo`
should be "meshopt" or "ogre" and that any other value falls back to
Algorithm::Ogre (i.e. ogre) with a warning, so change the phrase "falls back to
meshopt" to "falls back to ogre" and keep the rest of the comment intact; refer
to generateLodsWithAlgo for the location to modify.
🪄 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: 35cad893-d124-469d-ad8a-2273abf5ae81
📒 Files selected for processing (10)
CLAUDE.mdqml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MeshDecimator.cppsrc/MeshDecimator.hsrc/MeshDecimatorController.cppsrc/MeshDecimatorController.hsrc/MeshLodController.cppsrc/MeshLodController.h
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
Comment on lines
+2771
to
+2773
| model: ["ogre", "meshopt"] | ||
| currentIndex: 0 | ||
| font.pixelSize: 11 |
There was a problem hiding this comment.
LOD backend default currently resolves to ogre, not meshopt.
With model: ["ogre", "meshopt"] and currentIndex: 0, first-run behavior is ogre. This conflicts with the nearby comment and the PR objective for meshopt-first LOD generation.
Suggested fix
ThemedComboBox {
id: lodBackendCombo
width: 90; height: 26
model: ["ogre", "meshopt"]
- currentIndex: 0
+ currentIndex: 1
font.pixelSize: 11
}🤖 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 2771 - 2773, The LOD backend defaults
to "ogre" because model: ["ogre", "meshopt"] with currentIndex: 0 selects the
first entry; change the selection so meshopt is the default by either reordering
the model to ["meshopt", "ogre"] or setting currentIndex to the index of
"meshopt" (e.g., currentIndex: 1), updating the properties where model and
currentIndex are defined in the PropertiesPanel component.
Comment on lines
+2923
to
+2933
| palette { | ||
| window: ThemeManager.panelColor | ||
| windowText: ThemeManager.textColor | ||
| base: ThemeManager.inputColor | ||
| text: ThemeManager.textColor | ||
| button: ThemeManager.headerColor | ||
| buttonText: ThemeManager.textColor | ||
| highlight: ThemeManager.highlightColor | ||
| highlightedText: "white" | ||
| mid: ThemeManager.borderColor | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Qt Quick Controls 2, can a QtQuick Columndeclarepalette { ... }directly, or must palette be set viaControl.palette.* attached properties (or on a Control-derived type)?
💡 Result:
In modern Qt (specifically since Qt 6.0), the palette property is available on the Item QML type [1]. Because Column inherits from Item, it can indeed declare palette { ... } directly [2][1]. In older versions of Qt (such as Qt 5), palette was primarily a property of Control (from Qt Quick Controls 2) [3][4]. While you could not set a palette directly on a standard Column in those versions, the palette property is designed to propagate; if you set the palette on a parent Control or ApplicationWindow, it will correctly propagate to the children inside a Column [3][4]. Starting with Qt 6.0, the palette property was moved to the base Item type [1]. This means you can now define a palette on any Item-derived type, including Column, and the settings will propagate to all of its children [1]. Summary: - Qt 6.0+: Yes, Column can declare palette { ... } directly because it inherits the property from Item [1]. - Pre-Qt 6.0: No, Column does not have a palette property; you must set it on a Control-derived parent or ApplicationWindow [3][4]. Note that even in modern versions, you can still use the traditional Control.palette.* attached property approach if you prefer or need to target specific controls [3][5].
Citations:
- 1: https://doc.qt.io/qt-6/qml-qtquick-item.html
- 2: https://doc.qt.io/qt-6/qml-qtquick-column.html
- 3: https://doc.qt.io/archives/qt-5.11/qml-qtquick-controls2-control.html
- 4: https://felgo.com/doc/qt5/qml-qtquick-controls2-control/
- 5: https://doc.qt.io/qt-6.5/qml-qtquick-palette.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List candidate build files
fd -t f -e pro -e cmake -e txt -e ini -e sh -e ps1 -e qmlproject -e json . \
| head -n 200
# Search for Qt version indicators
rg -n --hidden --glob '!**/.git/**' \
-e "Qt6|Qt 6|QT_VERSION_MAJOR|find_package\\(Qt6|QT_MAJOR_VERSION|QT_VERSION" \
-e "find_package\\(Qt5|Qt5|QT_VERSION_MAJOR.*5|Qt 5" \
-S .
# Search for .pro / qmlproject / cmake that typically pins Qt version
fd -t f -e qmlproject -e pro . | head -n 50
fd -t f -e CMakeLists.txt -e cmake . | head -n 50Repository: fernandotonon/QtMeshEditor
Length of output: 20310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="qml/PropertiesPanel.qml"
echo "== palette block context (around 2910-2940) =="
nl -ba "$FILE" | sed -n '2890,2950p'
echo
echo "== check decimateContent declaration (around 2760-2790) =="
nl -ba "$FILE" | sed -n '2740,2810p'
echo
echo "== check lod backend combo / comment (around 3030-3080) =="
nl -ba "$FILE" | sed -n '3000,3100p'
echo
echo "== check any other nearby defaults (around 2930-2970) =="
nl -ba "$FILE" | sed -n '2920,2980p'Repository: fernandotonon/QtMeshEditor
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="qml/PropertiesPanel.qml"
show_range () {
local start="$1" end="$2"
awk -v s="$start" -v e="$end" '{ln=NR; if(ln>=s && ln<=e) printf("%d:%s\n", ln, $0)}' "$FILE"
}
echo "== palette block context (around 2890-2950) =="
show_range 2890 2950
echo
echo "== check decimateContent declaration (around 2740-2810) =="
show_range 2740 2810
echo
echo "== check lod backend combo / comment (around 3000-3100) =="
show_range 3000 3100Repository: fernandotonon/QtMeshEditor
Length of output: 11774
Fix LOD backend default comment mismatch; palette-on-Column is fine in Qt6
palette { ... }insideColumn { id: decimateContent }is valid for this Qt6 project (so the palette “invalid API” concern shouldn’t block).- In the LOD backend selector, the comment says default is
meshopt, butlodBackendComboismodel: ["ogre", "meshopt"]withcurrentIndex: 0(defaults toogre); update the comment and/or the default index to match.
🤖 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 2923 - 2933, The comment stating the
LOD backend default is "meshopt" is inconsistent with the UI: update the comment
and/or the default selection so they match the actual control; specifically,
either change the explanatory comment near the LOD backend selector to indicate
the default is "ogre" (since lodBackendCombo uses model: ["ogre","meshopt"] and
currentIndex: 0) or set lodBackendCombo.currentIndex to 1 to make "meshopt" the
default; also remove or amend the incorrect note about palette being an invalid
API for the Column (palette inside Column id: decimateContent is valid in Qt6)
so the comment no longer blocks the palette usage.
| // preserves UV seams + skin weights but in practice tends to | ||
| // produce a softer silhouette than Ogre's path. | ||
| QString algoStr = args.contains("algo") | ||
| ? args["algo"].toString().toLower() : QStringLiteral("ogre"); |
There was a problem hiding this comment.
Default backend is wired to ogre, not meshopt
Lines 2752 and 2984 default algo to "ogre", and the MCP schema/docs also advertise ogre as default. That conflicts with this PR’s stated objective to make meshoptimizer the default path, so MCP callers will silently run the legacy backend unless they opt in.
Suggested fix
- QString algoStr = args.contains("algo")
- ? args["algo"].toString().toLower() : QStringLiteral("ogre");
+ QString algoStr = args.contains("algo")
+ ? args["algo"].toString().toLower() : QStringLiteral("meshopt");- QString algoStr = args.contains("algo")
- ? args["algo"].toString().toLower() : QStringLiteral("ogre");
+ QString algoStr = args.contains("algo")
+ ? args["algo"].toString().toLower() : QStringLiteral("meshopt");- "LOD backend. 'ogre' (default) uses Ogre's stock MeshLodGenerator. "
+ "LOD backend. 'meshopt' (default) uses meshoptimizer's attribute-aware simplify. "
+ "'ogre' uses Ogre's stock MeshLodGenerator. "- "Decimation backend. 'ogre' (default) uses Ogre's stock MeshLodGenerator. "
- "'meshopt' uses meshoptimizer's attribute-aware simplify — preserves UV "
+ "Decimation backend. 'meshopt' (default) uses meshoptimizer's attribute-aware simplify — preserves UV "
"seams + skin weights but typically gives a softer silhouette. Same option "
"set as `generate_lods`."}};- "`reduction` (0..0.95), `target_tris`, or `target_verts`. Backend is "
- "selected via `algo` (default `ogre`). The response includes a human-readable "
+ "`reduction` (0..0.95), `target_tris`, or `target_verts`. Backend is "
+ "selected via `algo` (default `meshopt`). The response includes a human-readable "Also applies to: 2983-2985, 5323-5325, 5426-5429, 5434-5436
🤖 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` at line 2752, The default algorithm string is still set to
"ogre" in several places (e.g., where args["algo"] is parsed); change those
defaults to "meshopt" so meshoptimizer becomes the default backend. Locate each
occurrence of the algo parsing logic (instances that read
args["algo"].toString().toLower(), the MCPServer methods handling request
options, and any fallback assignments around the symbols like args["algo"] and
defaultAlgo) and replace the literal QStringLiteral("ogre") (and any equivalent
"ogre" default) with QStringLiteral("meshopt") (or the string "meshopt")
ensuring casing is normalized to lower-case as done currently. Verify all listed
occurrences (the blocks around the current defaults) are updated so callers
without an explicit algo choice will use meshopt.
Adds a meshoptimizer-backed LOD path alongside Ogre's stock
`MeshLodGenerator`. New default everywhere (CLI `--algo meshopt`,
MCP `generate_lods` `algo`, Inspector backend dropdown). The
meshopt path runs `simplifyWithAttributes` with UV0 as a weighted
attribute stream so UV seams stay intact on character meshes, then
post-passes `optimizeVertexCache` (Forsyth) on every LOD for free
post-T&L cache wins. Validated on Rumba Dancing.fbx: 10220 →
{7659, 5101, 2622} tris through the standard 25/50/75 reduction
ladder with skeleton + animations preserved.
* `MeshOptimizerLod` is a thin namespace facade (free functions,
no singleton) — pure-CPU work doesn't need the AIAssistManager
scaffolding epic #397 sketched out. That comes when the first
ML-using feature lands.
* `Mesh::_setLodInfo(N)` pre-sizes `SubMesh::mLodFaceList` to N-1
nullptr slots; the meshopt commit assigns by index rather than
`push_back`-ing on top, which doubled the slot count.
* CLI's per-LOD export loop now temporarily erases the
`qtme.faces.<i>` n-gon bindings before swapping `indexData`,
because `FBXExporter` prefers those bindings over `indexData`
and would otherwise emit the base mesh on every LOD level.
* `SentryReporter::addBreadcrumb` uses category `ai.assist.lod`
on the meshopt path per epic #397 acceptance.
* Unit tests cover reduce / multi-level monotonicity / empty
reductions / null mesh.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI fix: the dedicated `MaterialEditorQML_*` test targets in `tests/CMakeLists.txt` build with their own source list and don't pick up `src/MeshOptimizerLod.cpp`, so they failed to link against `MeshLodController::generateLods(int, …, Algorithm)`. Add the source + header to their TEST_SRC_FILES / TEST_HEADER_FILES and add `meshoptimizer` to COMMON_TEST_LIBRARIES. Codex review fix (P2): when count=4 with no explicit reductions, `MeshLodController` fills the fallback `0.25*(i+1)` which gives exactly 1.0 for the fourth slot. `MeshOptimizerLod::generateLods` was rejecting `reduction >= 1.0` outright, silently producing only 3 LODs while the caller saw success for the requested 4. Clamp `>= 1.0` down to 0.99 (still leaves the LOD a single tri's worth of geometry, which is what `count=4` is actually asking for) instead of dropping the level. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Flips the default back to Ogre for both LOD and decimation — meshoptimizer's attribute-aware simplify preserves UV seams and skin weights but its silhouette ends up softer on character meshes than Ogre's MeshLodGenerator, so Ogre stays the primary choice. Meshopt is kept as a selectable option for callers that need explicit seam / weight preservation. * LOD: default in C++ enum / QML dropdown / CLI / MCP all switch to `ogre`. ComboBox shows `["ogre", "meshopt"]`. * Decimate: same backend selector. New `MeshDecimator::Algorithm` enum and `decimateEntity(..., algo)` overload; QML `MeshDecimatorController.applyReductionWithAlgo`; CLI `qtmesh decimate ... --algo ogre|meshopt`; MCP `decimate_mesh` `algo` param. New Inspector backend dropdown in the Decimate section, identical style to the LOD one. * `MeshDecimator::promoteFirstLodToBase` now erases the `qtme.faces.<i>` n-gon bindings — without this the post- decimation FBX export silently re-emits the un-decimated triangle list off the cached binding (same gotcha the LOD per-LOD export already handled). Confirmed on Rumba Dancing with both backends: 10220 → 5101 (meshopt) / 5047 (ogre). * Sentry breadcrumb category `ai.assist.decimate` records the meshopt path so usage telemetry mirrors the LOD setup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three coupled fixes for the Decimate (single-pass) section:
* Eager-load the Component (`expanded: true` like LOD Generation
above it) so Qt Quick Controls instantiates during initial QML
evaluation. Without this, the Slider + ComboBox loaded lazily
on first expand and macOS Qt occasionally fell back to native
Aqua chrome — visible as a giant white round Slider handle and
a light-gray ComboBox fill while the same widget types in the
LOD section above rendered with the proper dark Basic style.
* Pin an explicit `palette { ... }` on the decimate Column root,
sourced from `ThemeManager.*Color`. Belt-and-suspenders against
future refactors that re-introduce lazy loading.
* Add `previewReductionWithAlgo(double, QString)` so the slider's
live preview honors the backend dropdown choice. Previously the
preview was hard-coded to Ogre — picking `meshopt` and dragging
showed Ogre's preview, then Apply ran meshopt against a mesh
whose viewport bias still pointed at the Ogre preview LOD,
making it look like meshopt never ran.
* Wire the backend dropdown's `onCurrentIndexChanged` to restart
the debounce so flipping backend mid-session re-renders the
preview immediately.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Reject `--algo` for `--auto` / `--remove` / `--info` modes (CodeRabbit). Previously `qtmesh lod model.fbx --auto --algo meshopt` silently ran Ogre's auto-config heuristic — the flag was being parsed but had no effect. Fail with a usage error instead. * Update `qtmesh --help` to advertise `--algo ogre|meshopt` on both `lod` and `decimate` (CodeRabbit) — was only discoverable from the error path. * Move caller-category Sentry breadcrumbs out of the shared `MeshLodController::generateLods()` entry point (CodeRabbit) — it's invoked from both Inspector (UI) and MCP paths, so picking `ai.assist.lod` vs `ui.action` based on the backend misclassified events. Use a neutral `mesh.lod` category in the shared method; tag `ui.action` from `generateLodsWithAlgo` (QML entry) and rely on MCPServer's per-tool `ai.tool_call` breadcrumb. Same shape for the decimate path. * Clamp fallback reductions strictly below 1.0 in `MeshLodController::generateLods` (CodeRabbit). Previously `count=4` with omitted reductions produced a fourth fallback slot of exactly 1.0, which collapsed to a single-triangle LOD on the Ogre path and was dropped entirely on the meshopt path before the controller-side guard kicked in. * Replace `GTEST_SKIP()` precondition checks with `ASSERT_TRUE` in `MeshOptimizerLod_test.cpp` (CodeRabbit) — Ogre-dependent tests should fail loudly when GL prerequisites are missing, not skip silently. Skipped: CodeRabbit's "default to meshopt" suggestion. The user explicitly chose Ogre as the primary backend in the Inspector / CLI / MCP because meshopt's silhouette ends up softer on the character meshes we test against; meshopt stays a selectable option, not the default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/ai-assist-meshopt-lod
branch
from
May 27, 2026 14:54
15dd7aa to
5cdbd23
Compare
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 `@src/MeshLodController.cpp`:
- Around line 145-153: The code currently maps any non-"meshopt" string to
Algorithm::Ogre and logs the raw algo in SentryReporter::addBreadcrumb; change
this to validate algo explicitly and reject unknown values: check algo.toLower()
against "meshopt" and "ogre" and if neither, emit an error (or return/fail the
operation) instead of silently choosing Algorithm::Ogre, then set the Algorithm
to either Algorithm::Meshopt or Algorithm::Ogre accordingly and ensure the
breadcrumb records the validated backend name (not the raw input) before calling
generateLods or returning; update the branch that uses
Algorithm::Meshopt/Algorithm::Ogre and the SentryReporter::addBreadcrumb call to
reflect this validation.
🪄 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: 45737f05-9bc1-41c1-875d-4b55c5b5a456
📒 Files selected for processing (16)
CLAUDE.mdCMakeLists.txtqml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MeshDecimator.cppsrc/MeshDecimator.hsrc/MeshDecimatorController.cppsrc/MeshDecimatorController.hsrc/MeshLodController.cppsrc/MeshLodController.hsrc/MeshOptimizerLod.cppsrc/MeshOptimizerLod.hsrc/MeshOptimizerLod_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 (14)
- tests/CMakeLists.txt
- src/CMakeLists.txt
- src/MeshDecimatorController.h
- CMakeLists.txt
- src/MeshDecimator.h
- src/MeshLodController.h
- src/MeshOptimizerLod_test.cpp
- src/MCPServer.cpp
- src/MeshOptimizerLod.cpp
- qml/PropertiesPanel.qml
- src/MeshDecimator.cpp
- src/MeshDecimatorController.cpp
- src/MeshOptimizerLod.h
- src/CLIPipeline.cpp
CodeRabbit review fixes on the rebased branch: * `MeshLodController::generateLodsWithAlgo`, `MeshDecimatorController::applyReductionWithAlgo`, and `MeshDecimatorController::previewReductionWithAlgo` all silently fell back to Ogre on any non-`meshopt` string. Reject unknown values with `emit error(...)` so QML typos and missing enum cases surface instead of looking like successful backend picks. Matches `MCPServer::toolGenerateLods` which already validated. Uses the normalised (trimmed + lowercased) name in the breadcrumb too so log messages match what the comparison actually saw. * `MeshOptimizerLod::generateLods` now skips degenerate submeshes (empty indices / positions / vertexCount=0 / targetRounded=0) with an empty `IndexData*` placeholder rather than passing null pointers to `meshopt_simplify`, whose docs require both positions and indices to be non-null with positive counts. The per-LOD swap loop in `CLIPipeline::cmdLod` walks by submesh index, so emitting a placeholder keeps slot ordering aligned. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MeshOptimizerLod.cpp (1)
243-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat tiny but valid submeshes as degenerate.
For a 1-triangle submesh, any positive reduction makes
targetIdxCounttruncate to0, so this branch emits an empty LOD and the geometry disappears outright. That is a valid “can't simplify further” case, not invalid input. Clamp valid targets to at least one triangle and reserve the empty placeholder for truly unusable sources only.💡 Minimal fix
- const size_t targetRounded = (targetIdxCount / 3) * 3; + const size_t targetRounded = + std::max<size_t>(3, (targetIdxCount / 3) * 3); @@ - if (src.indices.empty() || src.positions.empty() || - src.vertexCount == 0 || targetRounded == 0) { - level.indices.push_back(OGRE_NEW Ogre::IndexData()); + if (src.indices.size() < 3 || src.positions.empty() || + src.vertexCount == 0) { + level.indices.push_back(buildIndexData({})); level.actualReductions.push_back(0.0f); 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/MeshOptimizerLod.cpp` around lines 243 - 259, The current check treats any targetRounded == 0 as degenerate and emits an empty LOD, which drops tiny but valid submeshes (e.g., one triangle); instead clamp the computed targetRounded to at least one triangle when the source actually contains triangles and only treat truly unusable sources as degenerate. Concretely: after computing targetIdxCount/targetRounded, if src.indices.size() >= 3 and src.positions is non-empty and src.vertexCount > 0 and targetRounded == 0, set targetRounded to 3 (one triangle) so simplification is skipped but the LOD retains the original geometry; keep the early-return branch for genuine empty inputs (src.indices.empty(), src.positions.empty(), src.vertexCount == 0) but do not use targetRounded == 0 alone to decide degeneracy. Reference symbols: targetIdxCount, targetRounded, src.indices, src.positions, src.vertexCount, level.indices.push_back, level.actualReductions.
🤖 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.
Outside diff comments:
In `@src/MeshOptimizerLod.cpp`:
- Around line 243-259: The current check treats any targetRounded == 0 as
degenerate and emits an empty LOD, which drops tiny but valid submeshes (e.g.,
one triangle); instead clamp the computed targetRounded to at least one triangle
when the source actually contains triangles and only treat truly unusable
sources as degenerate. Concretely: after computing targetIdxCount/targetRounded,
if src.indices.size() >= 3 and src.positions is non-empty and src.vertexCount >
0 and targetRounded == 0, set targetRounded to 3 (one triangle) so
simplification is skipped but the LOD retains the original geometry; keep the
early-return branch for genuine empty inputs (src.indices.empty(),
src.positions.empty(), src.vertexCount == 0) but do not use targetRounded == 0
alone to decide degeneracy. Reference symbols: targetIdxCount, targetRounded,
src.indices, src.positions, src.vertexCount, level.indices.push_back,
level.actualReductions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 310c6843-97c6-4b52-8941-73cf1e3876b4
📒 Files selected for processing (3)
src/MeshDecimatorController.cppsrc/MeshLodController.cppsrc/MeshOptimizerLod.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MeshDecimatorController.cpp
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First slice of epic #397 — AI-assisted 3D workflows: a meshoptimizer-backed LOD generator behind
--algo meshopt|ogre, defaultmeshopt(closes #398).meshopt_simplifyWithAttributeswith UV0 as a weighted attribute stream → UV seams stay intact on character meshes that the stock OgreMeshLodGeneratorshreds.meshopt_optimizeVertexCache(Forsyth) on every LOD for free post-T&L cache wins.qtmesh lod ... --algo meshopt|ogre, MCPgenerate_lodsalgoparam, Inspector backend dropdown — all default tomeshopt.SentryReporterbreadcrumb categoryai.assist.lodrecords the chosen backend per epic acceptance.Validated on
paint/Rumba Dancing.fbxat the standard 25/50/75 reduction ladder:Skeleton (70 bones) and animation preserved across all levels. The Ogre legacy path (
--algo ogre) is kept for round-trip testing and produces comparable numbers (7666 / 5047 / 2466).Implementation notes
MeshOptimizerLodis a thin namespace facade, not a singleton. Pure-CPU work doesn't need theAIAssistManagerscaffolding the epic sketched — that comes when the first ML-using feature lands (e.g. depth-conditioned sd.cpp under AI: Mesh-aware texture generation (depth-conditioned sd.cpp) #403).Mesh::_setLodInfo(N)pre-sizesSubMesh::mLodFaceListto N-1 nullptr slots. The meshopt commit assigns by index rather thanpush_back-ing on top of those slots, which doubled the slot count in early drafts.FBXExportern-gon binding gotcha: when an FBX carries theqtme.faces.<i>quad-migration binding (Epic: Quad-based mesh representation (n-gon support) #326), the exporter prefers it overSubMesh::indexData, which silently emits the base mesh on every LOD. The CLI per-LOD export loop temporarily erases those bindings before swappingindexDataand restores them after.Follow-up epics
This also opens epic #678 to drive meshoptimizer through the scan + fix pipelines (vertex-cache reorder, overdraw / vertex-fetch optimization, weld-dedupe) for automated mesh quality.
Test plan
qtmesh lod Rumba\ Dancing.fbx --count 3 --algo meshopt -o ...reduces tris by ladder and preserves skeletonqtmesh lod Rumba\ Dancing.fbx --count 3 --algo ogre -o ...keeps legacy path workingqtmesh lod --info --jsonstill reports per-level triangle counts--algo foorejected with a usage errorMeshOptimizerLod_test.cpp: reduce / multi-level monotonicity / empty reductions / null mesh🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests