feat(#506): Draco mesh compression on glTF export + Assimp 6.0.5 - #942
Conversation
Adds `qtmesh convert <in> -o out.glb --compress draco`, producing KHR_draco_mesh_compression glTF/glb output. Assimp's glTF2 exporter has ZERO Draco support (ASSIMP_BUILD_DRACO=ON only wires the Draco *decoder* into the importer — verified against the 6.0.5 source), so the issue's "Path B" standalone encoder is required. - MeshDracoEncoder (Ogre-free, unit-tested): parses the Assimp-written glTF, Draco-encodes each indexed-triangle primitive (POSITION/NORMAL/ TEXCOORD/TANGENT, float-only; JOINTS/WEIGHTS left lossless), wires the KHR_draco_mesh_compression extension, strips compressed accessors' bufferViews, and garbage-collects orphaned geometry so the file actually shrinks. Skins/morphs/animations untouched. - CLI --compress draco with up-front validation + size report. - CMake ENABLE_DRACO option + cmake/Draco.cmake discovery (from the Assimp-with-Draco prefix or -DDRACO_ROOT); guarded by #ifdef ENABLE_DRACO. - CI: ASSIMP_BUILD_DRACO=ON on all 3 platforms; ENABLE_DRACO=ON on the Linux release/coverage + macOS builds; caches busted. - Bump Assimp 6.0.4 -> 6.0.5 (Ogre already 14.5.2, latest). Verified locally: Rumba .glb geometry 309KB->46KB (85% smaller), file 761KB->449KB, all 11 primitives decode back to exact face/vertex counts; 5 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe project adds optional Draco discovery and linking, a ChangesDraco Compression
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLIPipeline
participant Assimp
participant MeshDracoEncoder
participant glTFFile
User->>CLIPipeline: convert input with --compress draco
CLIPipeline->>Assimp: export glTF or GLB
Assimp-->>CLIPipeline: exported file
CLIPipeline->>MeshDracoEncoder: compressFile(path)
MeshDracoEncoder->>glTFFile: parse buffers and primitives
MeshDracoEncoder->>MeshDracoEncoder: encode geometry with Draco
MeshDracoEncoder->>glTFFile: rewrite extensions and buffers
MeshDracoEncoder-->>CLIPipeline: compression result and metrics
CLIPipeline-->>User: success or error message
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 3b40d4ce58
ℹ️ 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".
| uses: actions/cache@v3 | ||
| env: | ||
| cache-name: cache-assimp-linux | ||
| cache-name: cache-assimp-linux-605 |
There was a problem hiding this comment.
In the inspected Linux and macOS deploy jobs, enabling ASSIMP_BUILD_DRACO installs an additional Draco library and headers, but the Assimp cache paths still include only libassimp*, include/assimp, and include/contrib. Because the release and test jobs run on fresh runners, Draco.cmake cannot find the expected libdraco* and include/draco artifacts, silently sets ENABLE_DRACO back to OFF, and produces shipping binaries without the advertised compression support. Add the installed Draco library and header directories to every producer/consumer Assimp cache.
Useful? React with 👍 / 👎.
| for (const QString& sem : semantics) { | ||
| draco::GeometryAttribute::Type dtype; | ||
| int bits; | ||
| if (!dracoAttrForSemantic(sem, opts, dtype, bits)) continue; // skip JOINTS/WEIGHTS |
There was a problem hiding this comment.
When a primitive has data excluded here—such as JOINTS_n/WEIGHTS_n, normalized integer colors, or morph-target accessors—the TriangleSoup-built Draco mesh may reorder or deduplicate its points while those retained streams remain in the original vertex order. The decoded indices then address unrelated skin weights, colors, or morph deltas, visibly corrupting rigged and morph-animated exports. Either include all per-vertex streams losslessly and remap dependent data, or leave such primitives uncompressed.
Useful? React with 👍 / 👎.
|
|
||
| outCp.dracoBlob = QByteArray(buffer.data(), static_cast<int>(buffer.size())); | ||
| outCp.indicesAccessor = indicesAcc; | ||
| outCp.decodedPointCount = static_cast<int>(mesh->num_points()); |
There was a problem hiding this comment.
For primitives containing duplicate vertices with identical compressed attribute tuples, TriangleSoupMeshBuilder::Finalize() can merge points, so decodedPointCount becomes smaller than the original attribute accessor counts. This value is recorded here but never used, leaving the glTF accessors advertising more elements than the Draco stream decodes; validators or consumers may reject or misread the output. Preserve the original point mapping or rewrite the affected accessor metadata and dependent streams consistently.
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: 11
🧹 Nitpick comments (4)
CMakeLists.txt (1)
342-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlso update the cache entry when Draco is missing.
set(ENABLE_DRACO OFF)creates a normal variable only. The cache entry staysON. On every reconfigure CMake retries the discovery and prints the warning again, andcmake-gui/-Lstill reportsENABLE_DRACO=ONwhile the feature is disabled. Force the cache value so the reported state matches the build.♻️ Proposed change
else() message(WARNING "ENABLE_DRACO requested but Draco not found — disabling") - set(ENABLE_DRACO OFF) + set(ENABLE_DRACO OFF CACHE BOOL "Enable Draco mesh compression on glTF/glb export (`#506`)" FORCE) endif()🤖 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 `@CMakeLists.txt` around lines 342 - 353, Update the missing-Draco branch in the ENABLE_DRACO configuration block to force the ENABLE_DRACO cache entry to OFF, ensuring subsequent reconfigures and cache displays reflect that Draco is disabled.src/MeshDracoEncoder_test.cpp (1)
209-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the suite to the paths that carry the real risk.
The five Draco tests all use one glb with a single POSITION-only triangle primitive. The behaviors most likely to break are untested:
- A
.gltfinput with an external.binand with adata:URI buffer.resolveGltfBufferand the base64 re-embed path have no coverage.- A primitive that also carries
JOINTS_0/WEIGHTS_0. This is the path thatMeshDracoEncoder.hline 46 claims is lossless, and it is the case flagged in theencodePrimitivereview comment.- A primitive with a normalized
UNSIGNED_SHORTCOLOR_0, which the encoder deliberately skips.- A file with an unreferenced bufferView and an image bufferView, to lock down the garbage-collection remap.
- A multi-mesh, multi-primitive file, to confirm the
primitivesTotal/primitivesCompressedcounters.A skinned-primitive test would have caught the attribute-skipping defect before merge.
As per coding guidelines: "Add Google Test unit tests for new functionality."
🤖 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/MeshDracoEncoder_test.cpp` around lines 209 - 318, Expand the ENABLE_DRACO test suite around MeshDracoEncoder::compressFile to cover external .bin and data: URI .gltf buffers, JOINTS_0/WEIGHTS_0 preservation, normalized UNSIGNED_SHORT COLOR_0 skipping, unreferenced and image bufferView remapping, and multi-mesh/multi-primitive counter behavior. Add a skinned-primitive test that verifies the encoded result retains the joint and weight attributes, while preserving existing assertions for successful compression and round-trip validity.Source: Coding guidelines
src/MeshDracoEncoder.cpp (1)
524-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the reason a primitive was skipped.
perrreceives a specific failure reason fromencodePrimitiveand is then discarded. WhenprimitivesCompressedis smaller thanprimitivesTotal, the CLI reports only the counts, so a user cannot tell whether a primitive was a non-triangle mode, had no indices, or had an attribute the encoder refuses.Collect the distinct reasons into
Result(for example aQStringList skippedReasons) and print them fromCLIPipeline::cmdConvert.🤖 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/MeshDracoEncoder.cpp` around lines 524 - 529, Preserve each failed encodePrimitive reason by adding a distinct skippedReasons collection to Result and appending perr when a primitive is skipped in the compression loop. Update CLIPipeline::cmdConvert to print these reasons alongside the compressed/total counts when primitives were skipped.src/MeshDracoEncoder.h (1)
87-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the overload comment or remove it.
const Options& opts = Options()is valid in this context: default arguments for member functions are parsed in the enclosing class’s complete-class context, andOptionsis already complete whencompressFileis declared. The two overloads work, so this comment only misleads future readers.🤖 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/MeshDracoEncoder.h` around lines 87 - 92, Correct or remove the misleading comment above the compressFile overloads. Do not change the working declarations or overload behavior; ensure the documentation no longer incorrectly claims that a default Options argument is ill-formed.
🤖 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 @.github/workflows/deploy.yml:
- Line 211: Enable Draco in the Windows application CMake configure step by
adding the existing ENABLE_DRACO option alongside the other platform
configuration flags, matching the Linux and macOS workflows. Keep the Assimp
build and Windows release artifact configuration consistent so the shipped
binary supports Draco compression.
In `@cmake/Draco.cmake`:
- Around line 34-63: Update the Draco discovery failure branch in the CMake
logic around QTMESH_DRACO_FOUND so an explicitly enabled Draco build fails
configuration instead of only issuing a warning. Preserve the existing
diagnostics and successful qtmesh_draco setup when DRACO_INCLUDE_DIR and
DRACO_LIBRARY are found, but use a fatal configuration error when ENABLE_DRACO
is ON and discovery fails.
In `@src/CLIPipeline.cpp`:
- Around line 1927-1936: Update the Draco failure branch in CLIPipeline’s export
flow, specifically the !dr.ok handling after
MeshDracoEncoder::compressFile(absOutput), to report that the uncompressed
output was kept at absOutput. Preserve the existing error reporting and return
behavior, including for the non-fatal primitivesCompressed == 0 case.
- Around line 1937-1946: Update the conversion summary in CLIPipeline to
calculate and report the size change using Result::originalFileBytes and
Result::outputFileBytes rather than the geometry-only originalBinBytes and
compressedBinBytes. Keep the existing geometry-byte details in the message, but
make the percentage smaller value reflect the actual on-disk file sizes.
- Around line 1877-1882: Update the Draco output-extension validation in
CLIPipeline and the wantGlb selection in MeshDracoEncoder so .glb2 is accepted
and treated as a binary glTF container. Add glb2 alongside glb where
appropriate, ensuring it follows the binary serialization path rather than the
JSON serializer.
In `@src/MeshDracoEncoder_test.cpp`:
- Around line 90-97: Add the standard <algorithm> header to
MeshDracoEncoder_test.cpp so std::min and std::max used in the min/max
calculation have a direct include and do not rely on transitive headers.
In `@src/MeshDracoEncoder.cpp`:
- Around line 588-619: Make the bufferView garbage-collection/remapping flow
conservative: include references from bufferView and asset-root extensions,
including unrecognized extension objects, or skip the GC pass when such
references cannot be safely analyzed. Replace defaulting remap lookups in the
rewrite logic around the sparse, image, and Draco-extension handling with a
lookup that distinguishes missing mappings, and return an error when badRef is
detected instead of retargeting to bufferView 0; preserve valid remaps and the
existing compaction behavior.
- Around line 731-741: Update the output-writing flow in the shown encoder
method to write outBytes to a sibling temporary file instead of truncating the
target directly. Verify the write byte count equals outBytes.size(), require
flush to succeed, close the temporary file, and replace the target only after
all checks pass; on any failure, remove the temporary file, set r.error, and
return failure without modifying the existing export.
- Around line 625-637: Validate each buffer view’s off and len in the compaction
loop before calling compact.append: reject negative values and any range where
off exceeds newBin.size() or len exceeds newBin.size() - off, and propagate an
error through compressFile instead of copying invalid data. Preserve alignment
and remapping only for validated views.
- Around line 367-394: Update the primitive compression flow around the
semantic-planning loop and compressFile so any unsupported attribute, including
JOINTS_n, WEIGHTS_n, or non-FLOAT accessors, causes the entire primitive to
remain uncompressed rather than being partially encoded. Preserve normal
compression only when every primitive attribute is represented in Draco, and
report the skipped primitive and reason through the existing CLI logging path.
- Around line 396-410: Update the per-face processing around readIndices and the
AttrPlan loop to compute the maximum index once, then validate every attribute’s
accessor count before forming v0, v1, or v2. Reject the primitive with the
existing error-reporting path when any plan has fewer elements than maxIndex +
1, and only dereference plan.values after validation.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 342-353: Update the missing-Draco branch in the ENABLE_DRACO
configuration block to force the ENABLE_DRACO cache entry to OFF, ensuring
subsequent reconfigures and cache displays reflect that Draco is disabled.
In `@src/MeshDracoEncoder_test.cpp`:
- Around line 209-318: Expand the ENABLE_DRACO test suite around
MeshDracoEncoder::compressFile to cover external .bin and data: URI .gltf
buffers, JOINTS_0/WEIGHTS_0 preservation, normalized UNSIGNED_SHORT COLOR_0
skipping, unreferenced and image bufferView remapping, and
multi-mesh/multi-primitive counter behavior. Add a skinned-primitive test that
verifies the encoded result retains the joint and weight attributes, while
preserving existing assertions for successful compression and round-trip
validity.
In `@src/MeshDracoEncoder.cpp`:
- Around line 524-529: Preserve each failed encodePrimitive reason by adding a
distinct skippedReasons collection to Result and appending perr when a primitive
is skipped in the compression loop. Update CLIPipeline::cmdConvert to print
these reasons alongside the compressed/total counts when primitives were
skipped.
In `@src/MeshDracoEncoder.h`:
- Around line 87-92: Correct or remove the misleading comment above the
compressFile overloads. Do not change the working declarations or overload
behavior; ensure the documentation no longer incorrectly claims that a default
Options argument is ill-formed.
🪄 Autofix
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 Plus
Run ID: 18215125-0603-4bc4-bc7d-d820a74ad9de
📒 Files selected for processing (10)
.github/workflows/deploy.ymlCLAUDE.mdCMakeLists.txtREADME.mdcmake/Draco.cmakesrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/MeshDracoEncoder.cppsrc/MeshDracoEncoder.hsrc/MeshDracoEncoder_test.cpp
| find_path(DRACO_INCLUDE_DIR | ||
| NAMES draco/compression/encode.h | ||
| HINTS ${_draco_hint_roots} | ||
| PATH_SUFFIXES include | ||
| ) | ||
|
|
||
| # Draco installs as libdraco.a (static) or the per-module libs; the umbrella | ||
| # static archive is what a standalone or Assimp-vendored build produces. | ||
| find_library(DRACO_LIBRARY | ||
| NAMES draco libdraco draco_static | ||
| HINTS ${_draco_hint_roots} | ||
| PATH_SUFFIXES lib lib64 | ||
| ) | ||
|
|
||
| if(DRACO_INCLUDE_DIR AND DRACO_LIBRARY) | ||
| add_library(qtmesh_draco INTERFACE) | ||
| target_include_directories(qtmesh_draco INTERFACE "${DRACO_INCLUDE_DIR}") | ||
| target_link_libraries(qtmesh_draco INTERFACE "${DRACO_LIBRARY}") | ||
| set(QTMESH_DRACO_FOUND TRUE) | ||
| message(STATUS "Draco found: ${DRACO_LIBRARY}") | ||
| message(STATUS "Draco headers: ${DRACO_INCLUDE_DIR}") | ||
| else() | ||
| message(WARNING | ||
| "ENABLE_DRACO is ON but the Draco library was not found.\n" | ||
| " Searched roots: ${_draco_hint_roots}\n" | ||
| " DRACO_INCLUDE_DIR=${DRACO_INCLUDE_DIR}\n" | ||
| " DRACO_LIBRARY=${DRACO_LIBRARY}\n" | ||
| "Build Assimp with -DASSIMP_BUILD_DRACO=ON, or pass -DDRACO_ROOT=<dir> " | ||
| "pointing at a Draco install (with lib/ and include/draco).") | ||
| endif() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether Assimp 6.0.5 installs the Draco headers/library when ASSIMP_BUILD_DRACO=ON.
set -uo pipefail
tmp=$(mktemp -d)
git clone --depth 1 --branch v6.0.5 https://github.com/assimp/assimp "$tmp/assimp" 2>/dev/null || {
echo "clone failed"; exit 0; }
echo "=== assimp CMake references to draco ==="
rg -n --glob '*.cmake' --glob 'CMakeLists.txt' -C3 -i 'draco' "$tmp/assimp" | head -80
echo "=== draco contrib install() rules ==="
fd -t f 'CMakeLists.txt' "$tmp/assimp/contrib/draco" 2>/dev/null | while read -r f; do
echo "--- $f"
rg -n -C2 'install\(|DRACO_INSTALL|EXCLUDE_FROM_ALL' "$f" | head -40
doneRepository: fernandotonon/QtMeshEditor
Length of output: 6822
Fail CI when Draco was requested but not found.
ENABLE_DRACO=ON can still leave QTMESH_DRACO_FOUND FALSE, so CMake only warns and disables Draco. src/MeshDracoEncoder_test.cpp then compiles the #ifndef ENABLE_DRACO branch, so CI can pass without exercising Draco. Make this path non-silent when -DENABLE_DRACO=ON.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/Draco.cmake` around lines 34 - 63, Update the Draco discovery failure
branch in the CMake logic around QTMESH_DRACO_FOUND so an explicitly enabled
Draco build fails configuration instead of only issuing a warning. Preserve the
existing diagnostics and successful qtmesh_draco setup when DRACO_INCLUDE_DIR
and DRACO_LIBRARY are found, but use a fatal configuration error when
ENABLE_DRACO is ON and discovery fails.
Correctness (both reviewers, P1/critical): - Compression is now ALL-OR-NOTHING per primitive. Draco reorders/dedups points, so leaving JOINTS_n/WEIGHTS_n, morph targets, or non-FLOAT attrs uncompressed while compressing POSITION corrupted rigged/morph meshes. A primitive is compressed only if every per-vertex stream can go into Draco; otherwise it's left intact. Skinned/morph meshes are thus left uncompressed (a tracked follow-up will fold JOINTS/WEIGHTS into Draco losslessly). - Rewrite compressed attribute accessors' `count` to the Draco decoded point count (Draco may merge duplicate points; a stale count fails validators). - "0 primitives eligible" is now NON-fatal (nothingEligible): the valid uncompressed export is kept, CLI warns + exits 0 instead of erroring. Safety: - Bounds-check every index against its attribute element count before dereferencing (heap over-read on malformed glTF). - Bounds-check bufferView byteOffset+byteLength against the buffer in the compaction copy. - Atomic write: temp file + verified byte count + flush + rename, so a failed compression never truncates the already-written uncompressed export. - GC is now conservative: a generic recursive scan of the whole JSON keeps every bufferView referenced by any consumer (EXT_meshopt, structural metadata, vendor exts), not just the ones we model. Fixed a bug where the walk saw stale pre-strip accessors in c.json and kept all orphans alive (file grew instead of shrinking) — sync stripped arrays into c.json first. CI: - Assimp's own -DASSIMP_BUILD_DRACO is broken per-platform (macOS -Werror on draco headers, Linux exports assimp::draco -> uninstalled .so, Windows/MinGW `draco` install target missing). Build the vendored contrib/draco as a standalone static lib instead, install it beside Assimp, and add libdraco* + include/draco to every Assimp cache (producer + consumer) so ENABLE_DRACO doesn't silently turn OFF on fresh release/test runners. Windows stays off (like ONNX/mocap). Minor: - Accept .glb2 in --compress (was rejected by CLI while convert accepts it). - Report on-disk file-size delta, not only geometry bytes (a .gltf re-embeds base64, ~+33%, so geometry% alone could read "smaller" while the file grew). - On compression failure, tell the user an uncompressed file was written. - #include <algorithm> in the test. Verified locally: static mesh 194KB -> 54KB file (72%), 0 orphan bytes; skinned Rumba warns + exits 0 with a valid uncompressed glb; 6/6 unit tests pass (incl. new LeavesSkinnedPrimitiveUncompressed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review — all findings addressed in e70d09e. Summary: Critical / P1
CI
Correctness / robustness
Verified locally: static mesh 194 KB → 54 KB file (72% smaller, 0 orphan bytes); skinned Rumba warns + exits 0 with a valid uncompressed glb; 6/6 unit tests pass. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/MeshDracoEncoder_test.cpp (1)
392-394: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert preservation of both skin accessors.
Lines 392-394 verify only
POSITION. A regression could remove or rewriteJOINTS_0orWEIGHTS_0while this test still passes. Assert that both semantics still use accessors 2 and 3 and that those accessors retain theirbufferViewvalues.Proposed test extension
- int posAcc = p.value("attributes").toObject().value("POSITION").toInt(); - EXPECT_TRUE(json.value("accessors").toArray().at(posAcc).toObject().contains("bufferView")); + const QJsonObject attributes = p.value("attributes").toObject(); + EXPECT_EQ(attributes.value("POSITION").toInt(), 1); + EXPECT_EQ(attributes.value("JOINTS_0").toInt(), 2); + EXPECT_EQ(attributes.value("WEIGHTS_0").toInt(), 3); + const QJsonArray accessors = json.value("accessors").toArray(); + for (const int accessorIndex : {1, 2, 3}) { + EXPECT_TRUE(accessors.at(accessorIndex).toObject().contains("bufferView")); + }🤖 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/MeshDracoEncoder_test.cpp` around lines 392 - 394, Extend the assertions near the existing POSITION check to validate both JOINTS_0 and WEIGHTS_0 in the attributes object, confirming they reference accessors 2 and 3 respectively. Also verify the corresponding accessor entries retain their bufferView fields, while preserving the existing POSITION and extension assertions.
🤖 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/MeshDracoEncoder_test.cpp`:
- Around line 392-394: Extend the assertions near the existing POSITION check to
validate both JOINTS_0 and WEIGHTS_0 in the attributes object, confirming they
reference accessors 2 and 3 respectively. Also verify the corresponding accessor
entries retain their bufferView fields, while preserving the existing POSITION
and extension assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30eced02-cefc-49e7-8b97-abbe69919d03
📒 Files selected for processing (6)
.github/workflows/deploy.ymlCLAUDE.mdsrc/CLIPipeline.cppsrc/MeshDracoEncoder.cppsrc/MeshDracoEncoder.hsrc/MeshDracoEncoder_test.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/CLIPipeline.cpp
- src/MeshDracoEncoder.cpp
- src/MeshDracoEncoder.h
- CLAUDE.md
The tests/ auxiliary test executables (MaterialEditorQML_test, etc.) link a
static lib `qtmesh_test_common` built from an EXPLICIT source list in
tests/CMakeLists.txt (not src/'s SRC_FILES). That list carries CLIPipeline.cpp
— which now calls MeshDracoEncoder — but not MeshDracoEncoder.cpp itself, so
the coverage/unit-tests-linux job failed to link:
libqtmesh_test_common.a(CLIPipeline.cpp.o): undefined reference to
`MeshDracoEncoder::isSupported()` / `compressFile(QString const&)`
Add MeshDracoEncoder.{cpp,h} to the qtmesh_test_common source list and link
qtmesh_draco into it under ENABLE_DRACO (mirrors the ENABLE_ONNX/ALEMBIC
pattern already there). Verified locally: qtmesh_test_common now compiles
MeshDracoEncoder.cpp and links with no undefined references.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous run's build-windows failed in the jurplel/install-qt-action step (aqtinstall: "Specified path is bad: bin/cmake_automoc_parser.exe") — a transient Qt-download flake unrelated to this PR. All other jobs (Linux/macOS builds + unit-tests-linux incl. the 6 Draco tests) are green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SonarCloud flagged new_reliability_rating=3 on MeshDracoEncoder.cpp:422 — "1st function call argument is an uninitialized value". dracoAttrForSemantic() only writes dtype/bits on success, and two call sites ignored the return (comment said "validated above"), so on a false path dtype/bits would be used uninitialized. - Plan-build loop: initialize dtype/bits at declaration and guard the return. - Capture dracoType/quantBits into AttrPlan once, and reuse them in the encoder-quantization loop instead of a second ignored-return lookup (removes the other uninitialized-value path entirely). 6/6 unit tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed two follow-up fixes for the CI failures on the previous run:
The earlier |
|
1 participant
Closes #506.
What & why
Adds Draco mesh compression on glTF/glb export:
Key finding that shaped the design: Assimp's glTF2 exporter has zero Draco support —
ASSIMP_BUILD_DRACO=ONonly wires the Draco decoder into the importer (verified against the Assimp 6.0.5 source:glTF2Exporter.cpphas no draco references; the decoder lives behindASSIMP_ENABLE_DRACOinglTF2Asset.inl). So rebuilding Assimp with Draco (the issue's "Path A") only enables reading compressed files, never writing them. The issue's Path B — a standalone encoder module — is required, and that's what this PR implements.Changes
MeshDracoEncoder(src/MeshDracoEncoder.{h,cpp}) — Ogre-free, unit-tested post-processor. Parses the Assimp-written glTF (glb BIN chunk, or.gltf+ data-URI/external.bin) withQJsonDocument; for each indexed-triangle primitive it Draco-encodes the geometry (POSITION/NORMAL/TEXCOORD/TANGENT, float accessors only; normalized-integer attrs and JOINTS/WEIGHTS left uncompressed to stay lossless), adds theKHR_draco_mesh_compressionextension, strips the compressed accessors'bufferView, and garbage-collects the orphaned geometry so the file actually shrinks. Skins/morph targets/animations untouched. Re-emits glb or self-contained.gltf.--compress dracoonconvert, with up-front validation (unknown value / non-glTF output / build without Draco) and a size-reduction report.option(ENABLE_DRACO)+cmake/Draco.cmake(discovers libdraco from the Assimp-with-Draco prefix or-DDRACO_ROOT); all Draco calls#ifdef ENABLE_DRACO-guarded, clean "rebuild with -DENABLE_DRACO" error otherwise.-DASSIMP_BUILD_DRACO=ONon all 3 assimp builds;-DENABLE_DRACO=ONon the Linux release, Linux coverage/test, and macOS QtMeshEditor builds; caches busted. (Windows MinGW left off, matching ONNX/mocap.)MeshDracoEncoder_test.cpp), CLAUDE.md architecture entry, README, CLI help.Acceptance criteria
qtmesh convert in.fbx -o out.glb --compress dracoproduces compressed outputVerified locally (macOS arm64)
Rumba Dancing.fbx → .glb: all 11 primitives compressed, geometry 309 KB → 46 KB (85.1% smaller), whole file 761 KB → 449 KB.--compressvalue, non-glTF output) and the.gltfbase64 path verified.MeshDracoEncoderunit tests pass.Notes / follow-ups
--compress dracois a natural follow-up (this PR is the CLI + core module).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--compress dracosupport with compression statistics.Documentation
Tests