feat(#519): Anim Slice B — vertex animation + Alembic (.abc) import#804
feat(#519): Anim Slice B — vertex animation + Alembic (.abc) import#804fernandotonon wants to merge 13 commits into
Conversation
First sub-slice of mesh/vertex animation (epic #517, issue #519). Lands the manager + the CPU-side "decoded frame-set → Ogre VAT_POSE clip" builder and playback/enumeration, all reachable WITHOUT the Alembic dependency so it builds and tests on every platform today. The real Alembic reader (behind -DENABLE_ALEMBIC) is B2; disk-streaming + CLI/MCP + .abc→FBX convert is B3. - VertexAnimationManager (QML_SINGLETON, mirrors MorphAnimationManager): - FrameSet/FrameData pure-data types (a decoded per-vertex cache). - sampleHeuristic(frameCount): <32 → VAT_POSE poses, else stream (issue's rule; static + unit-tested). - buildClipFromFrames(mesh, name, frames): reads submesh-0 bind positions, creates one Ogre::Pose per frame (delta vs bind) + one VAT_POSE track keyed per frame time. Reuses Ogre's existing pose-blended playback, so the timeline scrubber / loop / dope sheet work with no new playback code. - hasVertexAnimation / vertexClipsFor (+ SelectionSet-resolved QML variants) to drive the "Mesh" dope-sheet row. - Sentry breadcrumb scene.anim.vertex_anim. - ENABLE_ALEMBIC cmake option (default OFF; -DENABLE_ALEMBIC define) + guarded message; cmake/Alembic.cmake wired in B2. - Registered as a QML singleton in mainwindow alongside MorphAnimationManager. - Headless-CI tests: heuristic threshold, FrameSet validity, clip construction (poses+track+length), vertex-count-mismatch rejection, entity enumeration — driven by a synthetic cube-wobble buffer (no Alembic, no GL for the pure ones). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds an optional Alembic ChangesAlembic vertex animation pipeline
Estimated code review effort: 4 (Complex) | ~80 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 |
![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: cf9ffb04db
ℹ️ 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".
| if(ENABLE_ALEMBIC) | ||
| include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/Alembic.cmake) |
There was a problem hiding this comment.
When someone configures with -DENABLE_ALEMBIC=ON, this branch executes include(.../cmake/Alembic.cmake), but that file is not present in the repo (checked rg --files cmake). CMake will fail during configure before any guards can apply, so desktop/package builds that opt into the new feature cannot configure; either add the module in this commit or keep the option inert until B2.
Useful? React with 👍 / 👎.
| if (mesh->hasAnimation(animName)) | ||
| mesh->removeAnimation(animName); |
There was a problem hiding this comment.
When the same clipName is built again, this removes only the Animation, not the dense frame poses created below as clip/frameN. Ogre poses are mesh-level and are still walked by the dope-sheet/export paths, so re-importing or rebuilding a vertex clip appends stale poses and exposes leftovers; remove the old frame poses as well before recreating the clip.
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: 2
🧹 Nitpick comments (1)
src/VertexAnimationManager_test.cpp (1)
129-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
buildClipFromFrames's other early-return branches.Only the vertex-count-mismatch rejection path is tested. The upstream contract also early-returns false for a null
mesh, an emptyclipName, and!frames.ok()— none of which are exercised here.✅ Suggested additional cases
TEST_F(VertexAnimationManagerTest, BuildClipRejectsInvalidInputs) { auto mesh = createStaticMesh("vam_invalid", 3); auto fs = makeWobble(3, 4); EXPECT_FALSE(VertexAnimationManager::buildClipFromFrames(nullptr, "x", fs)); EXPECT_FALSE(VertexAnimationManager::buildClipFromFrames(mesh.get(), "", fs)); EXPECT_FALSE(VertexAnimationManager::buildClipFromFrames(mesh.get(), "y", VertexAnimationManager::FrameSet{})); }🤖 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/VertexAnimationManager_test.cpp` around lines 129 - 153, Add test coverage for the remaining early-return paths in VertexAnimationManager::buildClipFromFrames, not just the vertex-count mismatch case. Create a focused test around buildClipFromFrames that verifies it returns false for a null mesh, an empty clipName, and an invalid FrameSet (frames.ok() false). Keep the existing BuildClipFromFramesCreatesPosesAndTrack and BuildClipRejectsVertexCountMismatch tests intact, and add a new invalid-input test in VertexAnimationManagerTest to cover these branches.
🤖 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/VertexAnimationManager.cpp`:
- Around line 166-169: The breadcrumb in VertexAnimationManager::addBreadcrumb
uses an unsupported category, so update the SentryReporter::addBreadcrumb call
to use one of the established categories instead of "scene.anim.vertex_anim".
Pick the category that best matches this operation from the approved set
(ui.action, ai.tool_call, file.import, file.export) and keep the existing
message format in the same place where the VAT_POSE clip is built.
- Around line 112-171: The buildClipFromFrames path removes and recreates the
animation clip, but the poses created via mesh->createPose() are left behind on
failure or when replacing an existing clip. Update
VertexAnimationManager::buildClipFromFrames to track each created pose (or at
least the pose names/indices) and clean them up when aborting mid-loop, and also
remove any existing pose(s) associated with the same clipName before rebuilding
so stale VAT_POSE data does not accumulate.
---
Nitpick comments:
In `@src/VertexAnimationManager_test.cpp`:
- Around line 129-153: Add test coverage for the remaining early-return paths in
VertexAnimationManager::buildClipFromFrames, not just the vertex-count mismatch
case. Create a focused test around buildClipFromFrames that verifies it returns
false for a null mesh, an empty clipName, and an invalid FrameSet (frames.ok()
false). Keep the existing BuildClipFromFramesCreatesPosesAndTrack and
BuildClipRejectsVertexCountMismatch tests intact, and add a new invalid-input
test in VertexAnimationManagerTest to cover these branches.
🪄 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: 76a58e85-88a6-4362-bb5b-7746763a40b7
📒 Files selected for processing (6)
CMakeLists.txtsrc/CMakeLists.txtsrc/VertexAnimationManager.cppsrc/VertexAnimationManager.hsrc/VertexAnimationManager_test.cppsrc/mainwindow.cpp
| SentryReporter::addBreadcrumb( | ||
| QStringLiteral("scene.anim.vertex_anim"), | ||
| QStringLiteral("built VAT_POSE clip '%1' — %2 frames, %3 verts") | ||
| .arg(clipName).arg(frames.frames.size()).arg(vcount)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Breadcrumb category doesn't match the established set.
"scene.anim.vertex_anim" isn't one of the established categories. As per coding guidelines, "All user-facing actions and significant operations must add a Sentry breadcrumb via SentryReporter::addBreadcrumb(category, message) using the established categories (ui.action, ai.tool_call, file.import, file.export)."
🔧 Suggested fix
- SentryReporter::addBreadcrumb(
- QStringLiteral("scene.anim.vertex_anim"),
- QStringLiteral("built VAT_POSE clip '%1' — %2 frames, %3 verts")
- .arg(clipName).arg(frames.frames.size()).arg(vcount));
+ SentryReporter::addBreadcrumb(
+ QStringLiteral("ui.action"),
+ QStringLiteral("built VAT_POSE clip '%1' — %2 frames, %3 verts")
+ .arg(clipName).arg(frames.frames.size()).arg(vcount));📝 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.
| SentryReporter::addBreadcrumb( | |
| QStringLiteral("scene.anim.vertex_anim"), | |
| QStringLiteral("built VAT_POSE clip '%1' — %2 frames, %3 verts") | |
| .arg(clipName).arg(frames.frames.size()).arg(vcount)); | |
| SentryReporter::addBreadcrumb( | |
| QStringLiteral("ui.action"), | |
| QStringLiteral("built VAT_POSE clip '%1' — %2 frames, %3 verts") | |
| .arg(clipName).arg(frames.frames.size()).arg(vcount)); |
🤖 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/VertexAnimationManager.cpp` around lines 166 - 169, The breadcrumb in
VertexAnimationManager::addBreadcrumb uses an unsupported category, so update
the SentryReporter::addBreadcrumb call to use one of the established categories
instead of "scene.anim.vertex_anim". Pick the category that best matches this
operation from the approved set (ui.action, ai.tool_call, file.import,
file.export) and keep the existing message format in the same place where the
VAT_POSE clip is built.
Source: Path instructions
The test build's qtmesh_test_common static lib (tests/CMakeLists.txt) uses an explicit source list, not the app's SRC_FILES. mainwindow.cpp — which is in that lib and now references VertexAnimationManager::qmlInstance — linked against a missing symbol (staticMetaObject / qmlInstance) because the new .cpp wasn't compiled/moc'd into the lib. Add it next to the sibling anim managers. Verified: UnitTests links after a fresh reconfigure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Second sub-slice: the real Alembic import behind -DENABLE_ALEMBIC, feeding
B1's buildClipFromFrames. Default stays OFF so normal/CI platform builds are
unaffected and fast; the Linux coverage lane flips it ON so the round-trip
test actually runs.
- cmake/Alembic.cmake: FetchContent Imath 3.1.12 + Alembic 1.8.8 (both
BSD-3-Clause), fully static, all optional components off (no HDF5 / Python /
tests / binaries / examples). Satisfies Alembic's find_package(Imath) by
pointing Imath_DIR at the subproject build config. IMATH_INSTALL stays ON
because Alembic's install(EXPORT AlembicTargets) is unconditional and
references Imath — Imath must be in an export set or CMake's generate step
fails. Wrapped as a stable `qtmesh_alembic` interface target.
- AlembicImporter.{h,cpp}: readFrameSet() decodes the first IPolyMesh in an
archive into B1's source-agnostic FrameSet (pure data — rejects
variable-topology caches, fan-triangulates n-gon faces, derives fps from
the time sampling). importToScene() builds the base Ogre mesh (frame-0
topology) + a VAT_POSE clip + entity. All Alembic/Imath usage is
#ifdef ENABLE_ALEMBIC-guarded; the no-flag build returns a clear
"rebuild with -DENABLE_ALEMBIC" and never crashes.
- .abc routes through MeshImporterExporter::importer + the valid-extension
list, so File → Import handles it.
- Round-trip test writes a synthetic 2-frame quad .abc (Alembic OWrite) and
reads it back through readFrameSet, asserting vertex count / per-frame Y /
times / AABB. A separate no-flag test asserts graceful unavailability.
- deploy.yml: -DENABLE_ALEMBIC=ON on the coverage/unit-test lane only.
- CLAUDE.md: document the Slice B animation additions.
Validated locally on macOS arm64: clean configure (Imath+Alembic vendored,
find_package(Imath) resolves), Alembic-ON app + UnitTests build and link
against the real API, ENABLE_ALEMBIC=OFF build still compiles with the stub.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/MeshImporterExporter.cpp (1)
2100-2115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
.abcbranch is inconsistent with sibling import paths for observability and error surfacing.Every other suffix branch (
tmd,rsd,ply) emits aSentryReporter::addBreadcrumb("file.import", ...)on success and callsshowImportProblem(...)on failure. The.abcbranch does neither: a successful import leaves no breadcrumb, and a failure only writes to the Ogre log — the user gets no dialog even though.abcis now an accepted drag/drop and CLI extension. In a build without-DENABLE_ALEMBICthis means a silent, log-only failure.Add a success breadcrumb and surface the failure to the user like the other branches.
Based on learnings: "All user-facing actions and significant operations should add a `SentryReporter::addBreadcrumb(category, message)` breadcrumb, using the established categories for UI actions, tool calls, and file I/O."♻️ Suggested change
QString abcErr; Ogre::SceneNode* abcNode = AlembicImporter::importToScene(file.absoluteFilePath(), &abcErr); if (!abcNode) { Ogre::LogManager::getSingleton().logError( "Alembic import failed: " + abcErr.toStdString()); + showImportProblem( + QStringLiteral("Alembic"), + QStringLiteral("Could not import %1: %2").arg(file.fileName(), abcErr), + QStringLiteral("abc"), + abcErr); + } else { + SentryReporter::addBreadcrumb( + QStringLiteral("file.import"), + QStringLiteral("Imported Alembic: %1").arg(file.fileName())); } 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/MeshImporterExporter.cpp` around lines 2100 - 2115, The Alembic `.abc` handling in `MeshImporterExporter` is inconsistent with the other import branches because it logs only to Ogre and never records a breadcrumb or shows a user-facing error. Update the `.abc` path around `AlembicImporter::importToScene` to add a `SentryReporter::addBreadcrumb("file.import", ...)` on success, and on failure call `showImportProblem(...)` with the `abcErr` message instead of relying only on `Ogre::LogManager::logError`. Keep the behavior aligned with the `tmd`, `rsd`, and `ply` branches so drag/drop and CLI imports surface errors consistently.Source: Learnings
🤖 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 `@cmake/Alembic.cmake`:
- Around line 48-52: Update the Imath package hint in the Alembic setup so
`find_package(Imath CONFIG)` can actually locate `ImathConfig.cmake`; the
current `Imath_DIR` default in the `if(NOT DEFINED Imath_DIR)` block points to
the wrong subdirectory. Set `Imath_DIR` to the Imath binary directory produced
by the subproject (`qtmesh_imath_BINARY_DIR`) rather than the `config`
subfolder, keeping the existing cache/force behavior in the
`cmake/Alembic.cmake` logic.
---
Nitpick comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 2100-2115: The Alembic `.abc` handling in `MeshImporterExporter`
is inconsistent with the other import branches because it logs only to Ogre and
never records a breadcrumb or shows a user-facing error. Update the `.abc` path
around `AlembicImporter::importToScene` to add a
`SentryReporter::addBreadcrumb("file.import", ...)` on success, and on failure
call `showImportProblem(...)` with the `abcErr` message instead of relying only
on `Ogre::LogManager::logError`. Keep the behavior aligned with the `tmd`,
`rsd`, and `ply` branches so drag/drop and CLI imports surface errors
consistently.
🪄 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: 4ce1f41e-1c22-41ed-bebf-c7ee87203199
📒 Files selected for processing (10)
.github/workflows/deploy.ymlCLAUDE.mdcmake/Alembic.cmakesrc/AlembicImporter.cppsrc/AlembicImporter.hsrc/AlembicImporter_test.cppsrc/CMakeLists.txtsrc/Manager.cppsrc/MeshImporterExporter.cpptests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
| # Alembic's find_package(Imath) resolves against a CONFIG. Imath-as-subproject | ||
| # writes its package config under the build dir; point find_package there. | ||
| if(NOT DEFINED Imath_DIR) | ||
| set(Imath_DIR "${qtmesh_imath_BINARY_DIR}/config" CACHE PATH "" FORCE) | ||
| endif() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Where does Imath 3.1.12 CMakeLists.txt write its build-tree package config (Imath_DIR) when building as a FetchContent subproject?
💡 Result:
When using Imath 3.1.12 as a FetchContent subproject, the build-tree configuration files (including ImathConfig.cmake) are generated within the Imath build directory, specifically inside the directory defined by ${CMAKE_CURRENT_BINARY_DIR} [1]. In the Imath build system, the configuration files are managed in config/CMakeLists.txt [1]. The ImathConfig.cmake and associated version files are generated into the root of the Imath build directory (within that subproject's binary directory) using CMake's configure_package_config_file command [1]. Because FetchContent adds the project directly into the main build tree, you generally do not need to manually set Imath_DIR to locate the package [2][3]. Instead, you can simply call find_package(Imath CONFIG) in your downstream CMake code [3]. CMake will typically find the Imath targets exported within the current build session, allowing you to link against them using the Imath::Imath namespace [1][3]. If for any reason manual path resolution is required, it would point to the binary directory where the Imath subproject was configured [1].
Citations:
- 1: https://github.com/AcademySoftwareFoundation/Imath/blob/main/config/CMakeLists.txt
- 2: https://repo.or.cz/Imath.mirror.git/commit/c4273e171b79eb732059731681aea50c722122b3
- 3: https://github.com/AcademySoftwareFoundation/Imath/blob/v3.0.1/docs/PortingGuide2-3.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo root: '; pwd
echo '--- files mentioning qtmesh_imath_BINARY_DIR / Imath_DIR / ImathConfig.cmake / configure_package_config_file ---'
rg -n --hidden --glob '!**/.git/**' 'qtmesh_imath_BINARY_DIR|Imath_DIR|ImathConfig\.cmake|configure_package_config_file|write_basic_package_version_file' .
echo '--- candidate files ---'
git ls-files | rg '(^|/)(Alembic\.cmake|CMakeLists\.txt|Imath|config/CMakeLists\.txt|config\.cmake)$'Repository: fernandotonon/QtMeshEditor
Length of output: 984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the specific files that can establish the package-config output path.
for f in cmake/Alembic.cmake config/CMakeLists.txt; do
if [ -f "$f" ]; then
echo "=== $f ==="
wc -l "$f"
echo "--- outline ---"
ast-grep outline "$f" --view expanded || true
echo "--- excerpt ---"
cat -n "$f" | sed -n '1,220p'
echo
fi
doneRepository: fernandotonon/QtMeshEditor
Length of output: 5125
🌐 Web query:
Imath 3.1.12 config/CMakeLists.txt configure_package_config_file ImathConfig.cmake CMAKE_CURRENT_BINARY_DIR config subdirectory
💡 Result:
In Imath 3.1.12, the config/CMakeLists.txt file uses the CMake command configure_package_config_file to generate the ImathConfig.cmake file [1]. This command is part of the CMakePackageConfigHelpers module, which is essential for creating relocatable CMake package configuration files [2][3][4]. The usage in Imath 3.1.12 follows this standard pattern [1]: include(CMakePackageConfigHelpers) configure_package_config_file(${CMAKE_CURRENT_SOURCE_DIR}/ImathConfig.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/ImathConfig.cmake INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/Imath) Key technical details regarding this configuration include: 1. Purpose of configure_package_config_file: Unlike the standard configure_file, this command is specifically designed for generating Config.cmake files [3][4]. It enables the created file to be relocatable by calculating path variables (like install directories) relative to the installation prefix, rather than using absolute build-time paths [3][4][5]. 2. CMAKE_CURRENT_BINARY_DIR: This variable is used as the output destination for the generated ImathConfig.cmake file [1]. Generating it here ensures that the file is created within the build tree, from where it can subsequently be installed to the final destination using an install command [1][5]. 3. INSTALL_DESTINATION: This mandatory argument specifies where the package configuration file will eventually be installed [3][5]. The command uses this path to correctly calculate relative paths within the generated configuration file, ensuring that when a user subsequently finds the package via find_package, the paths resolve correctly regardless of where the package was installed on their system [3][4]. 4. Workflow: After generating the file in the CMAKE_CURRENT_BINARY_DIR, the Imath build system uses the install command to copy the generated ImathConfig.cmake and associated version file (generated via write_basic_package_version_file) to the appropriate system directory (e.g., ${CMAKE_INSTALL_LIBDIR}/cmake/Imath) [1].
Citations:
- 1: https://github.com/AcademySoftwareFoundation/Imath/blob/main/config/CMakeLists.txt
- 2: https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html
- 3: https://cmake.org/cmake/help/v4.3/module/CMakePackageConfigHelpers.html
- 4: https://cmake.org/cmake/help/v3.23/module/CMakePackageConfigHelpers.html
- 5: https://stackoverflow.com/questions/70209356/whats-the-use-of-configure-package-config-file-option-install-destination
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect any local copy of Imath sources if present.
fd -a 'config/CMakeLists.txt' .
fd -a 'ImathConfig.cmake.in' .
fd -a 'ImathConfigVersion.cmake.in' .Repository: fernandotonon/QtMeshEditor
Length of output: 164
🌐 Web query:
site:github.com/AcademySoftwareFoundation/Imath v3.1.12 config/CMakeLists.txt ImathConfig.cmake configure_package_config_file
💡 Result:
In Imath v3.1.12, the configuration of the package's CMake files is managed within the config/CMakeLists.txt file [1][2]. The project utilizes the standard CMake command configure_package_config_file to generate the ImathConfig.cmake file from an input template, ImathConfig.cmake.in [1]. Specifically, the following process is used in config/CMakeLists.txt: 1. The CMakePackageConfigHelpers module is included to provide the necessary helper functions [1]. 2. configure_package_config_file is called to generate the final ImathConfig.cmake in the build directory, with the INSTALL_DESTINATION set to the appropriate subdirectory within the installation library path (typically lib/cmake/Imath) [1]. 3. A corresponding version file is generated using write_basic_package_version_file to ensure compatibility checks during find_package(Imath CONFIG) calls [1]. 4. The generated ImathConfig.cmake and ImathConfigVersion.cmake files are subsequently installed to the target directory alongside the exported project targets [1]. This setup allows downstream projects to reliably locate Imath using find_package(Imath CONFIG) [3][4]. Notably, version 3.1.12 specifically addressed issues related to improper versioning in these configuration files that were present in the previous v3.1.11 release [2][5].
Citations:
- 1: https://github.com/AcademySoftwareFoundation/Imath/blob/main/config/CMakeLists.txt
- 2: https://github.com/AcademySoftwareFoundation/Imath/releases/tag/v3.1.12
- 3: https://github.com/AcademySoftwareFoundation/Imath/blob/main/CMakeLists.txt
- 4: https://github.com/AcademySoftwareFoundation/Imath/blob/v3.0.1/docs/PortingGuide2-3.md
- 5: https://github.com/AcademySoftwareFoundation/Imath/blob/main/CHANGES.md
🌐 Web query:
site:github.com/AcademySoftwareFoundation/Imath v3.1.12 "configure_package_config_file" "CMAKE_CURRENT_BINARY_DIR"
💡 Result:
The function configure_package_config_file is a command provided by the CMakePackageConfigHelpers module in CMake. It is used to generate a package configuration file (typically named Config.cmake) for a project, which enables other projects to find and use your library via the find_package command [1]. In practice, CMAKE_CURRENT_BINARY_DIR is frequently used as the location to generate this configuration file before it is installed [1][2]. By generating the file in the binary directory, you ensure that the project can reference the generated configuration file (and associated version files) during both the build process and installation phase [1][2]. A typical usage pattern, as seen in projects like Imath, involves these steps: 1. Include the helper module: include(CMakePackageConfigHelpers) 2. Call the function: configure_package_config_file( ${CMAKE_CURRENT_SOURCE_DIR}/MyPackageConfig.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/MyPackageConfig.cmake INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/MyPackage) This command configures the input template file (.cmake.in) by replacing variables with their current values and outputs the result to the specified file in CMAKE_CURRENT_BINARY_DIR, while setting up path variables to be relocatable based on the INSTALL_DESTINATION [1].
Citations:
- 1: https://github.com/AcademySoftwareFoundation/Imath/blob/main/config/CMakeLists.txt
- 2: https://github.com/AcademySoftwareFoundation/Imath/blob/main/CMakeLists.txt
Point Imath_DIR at the Imath binary dir. Imath 3.1.12 writes ImathConfig.cmake to ${qtmesh_imath_BINARY_DIR}, not ${qtmesh_imath_BINARY_DIR}/config, so the current path makes Alembic’s find_package(Imath CONFIG) miss the package.
🤖 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/Alembic.cmake` around lines 48 - 52, Update the Imath package hint in
the Alembic setup so `find_package(Imath CONFIG)` can actually locate
`ImathConfig.cmake`; the current `Imath_DIR` default in the `if(NOT DEFINED
Imath_DIR)` block points to the wrong subdirectory. Set `Imath_DIR` to the Imath
binary directory produced by the subproject (`qtmesh_imath_BINARY_DIR`) rather
than the `config` subfolder, keeping the existing cache/force behavior in the
`cmake/Alembic.cmake` logic.
Adding -DENABLE_ALEMBIC=ON to the coverage/unit-test lane perturbed that lane's build layout enough to break the pre-existing PS1 runtime test (EmuCoreLoaderTest: "PS1 stub core plugin not built beside test binary") — building Alembic shifts output-dir/copy timing and the ps1core stub .so lands where the test no longer finds it. The Alembic reader itself is fine: in that same run AlembicImporterTest (Available / ReadsTwoFrameQuadCache round-trip / MissingFileFailsGracefully) all PASSED on Linux before the job aborted on the PS1 suite. Revert the coverage-lane flag so it stays identical to master (green) and CI runs stay fast. The Alembic path is validated: locally on macOS (configure + app + tests build/link against the real API) and on Linux via that run's passing AlembicImporterTest. A dedicated Alembic-on CI job can be added later without coupling to the PS1-enabled lane. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… layout - Crash: deleting a morph/vertex target while a clip played freed pose / animation-state data the render frame loop was mid-read of. removePosesByName (shared by delete + rename redo/undo) now stops playback and disables the state before mutating — same guard deleteAnimation/renameAnimation already use. Guarded so it's a no-op headless. - Layout: the morph-list header used a magic-number spacer (Item width: parent.width - 320) that went negative on a narrow Inspector, overlapping the title with the Add/Reset buttons. Converted to a RowLayout — title takes flexible space + elides, buttons keep intrinsic size at the right. - Also surfaced the Animations section (play/enable/loop controls) for vertex/mesh-animated selections, not just skeletal ones — a vertex-anim mesh has no skeleton so the section (and its play button) never appeared. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nimation MCP Slice B3 headless parity for the vertex-animation feature: - `qtmesh anim <file>.abc --info [--json]`: cheap vertex-cache metadata (frames/verts/tris/fps/duration/storage) via AlembicImporter::readInfo, which reads the schema header + first sample only — no full decode. - MCP `import_alembic`: decodes an .abc into the live scene as a VAT_POSE-animated entity (heavy tool), reporting the created node, entities, and vertex-clip names so an agent can immediately drive playback. - MCP `play_vertex_animation`: plays/stops a vertex clip; delegates to the proven toolPlayAnimation path (a vertex clip surfaces as an ordinary AnimationState). All Alembic usage stays behind ENABLE_ALEMBIC; a build without it returns a clear "rebuild with -DENABLE_ALEMBIC=ON" message from every surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- ReadResult gains totalFrames + truncated so the 512-frame decode cap is no longer silent: importToScene logs a warning naming imported/total frames when it bites (CLAUDE.md "no silent caps"). True per-frame vertex-buffer streaming stays future work — VAT_POSE holds every frame resident by design. - Tests: readInfo matches a full decode (frames/verts/tris/fps/duration/ storage) on the 2-frame quad fixture; maxFrames caps + flags truncation; the non-ENABLE_ALEMBIC standalone test covers readInfo failing soft. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ex_animation MCP Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
.abc already flowed into "All supported" (it's in Manager's valid-extension list), but there was no dedicated filter row to narrow to just Alembic caches. Add one alongside the PlayStation entry so .abc is discoverable in the import dialog. Test asserts the row is present. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fernandotonon
changed the title
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 (3)
src/AlembicImporter.cpp (3)
262-286: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing null checks on
faceIdx/faceCntbefore dereferencing.
readInfoguardsfirst.getFaceCounts()withif (auto fc = ...)(line 232) before dereferencing, butbuildBaseMeshreadsfaceIdx/faceCntat lines 270-271 and immediately dereferences them (faceCnt->size()at 274,(*faceIdx)[cursor]at 277-279) with no null check. If a sample legitimately returns null face data here, this is a null-pointer dereference (crash) that the surroundingcatch (const std::exception&)won't catch, unlike the established pattern elsewhere in this same file.🛡️ Proposed fix
Abc::Int32ArraySamplePtr faceIdx = first.getFaceIndices(); Abc::Int32ArraySamplePtr faceCnt = first.getFaceCounts(); + if (!faceIdx || !faceCnt) { + return Ogre::MeshPtr(); + } // Fan-triangulate each n-gon face (Alembic faces are arbitrary polygons).🤖 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/AlembicImporter.cpp` around lines 262 - 286, The buildBaseMesh Alembic face-data read path dereferences faceIdx and faceCnt without checking for null, unlike the guarded pattern used in readInfo. Update buildBaseMesh to validate both first.getFaceIndices() and first.getFaceCounts() before entering the triangulation loop, and return an empty Ogre::MeshPtr if either sample is missing so the cursor/size dereferences in the fan-triangulation logic are safe.
137-193: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTruncated decode still fails
fs.ok()
FrameSet::ok()still requiresframes.size() >= 2, soreadFrameSet(path, /*maxFrames=*/1)returnsok == falseeven though the new truncation test expects success with one decoded frame. Separate “decode succeeded” from “can animate,” or align the test/contract with the 2-frame minimum.🤖 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/AlembicImporter.cpp` around lines 137 - 193, The truncated decode path in AlembicImporter::readFrameSet still treats a single decoded frame as failure because it relies on FrameSet::ok(), which enforces a 2-frame minimum. Update the contract so decode success is reported separately from animatability: either relax the final r.ok assignment to allow one decoded frame when maxFrames limits the result, or adjust FrameSet::ok()/callers so only the animation path requires at least two frames. Keep the fix localized around readFrameSet and VertexAnimationManager::FrameSet.
341-395: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap the Alembic import tool in a
try/catch—buildBaseMeshalready handles its own exceptions, butManager::addSceneNode/createEntityare still unguarded andMCPServer::callToolinvokes the handler with no outer catch. A thrownOgre::Exceptionhere will escape the MCP path instead of returning an error result.🤖 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/AlembicImporter.cpp` around lines 341 - 395, Wrap the Alembic import flow in importToScene with a top-level try/catch so exceptions from Manager::addSceneNode and createEntity are converted into a failed import result instead of escaping through MCPServer::callTool. Keep the existing fail helper and, in the catch for Ogre::Exception and a general catch-all, populate the error string and return nullptr so the tool returns a proper error response.
🧹 Nitpick comments (1)
src/commands/MorphCommands.cpp (1)
55-99: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a regression test for the playback-stop-before-mutation fix.
This addresses a reproduced crash (play a morph/vertex clip, then delete a target). A targeted Google Test asserting
removePosesByNamedisables the animation state and doesn't crash while an animation is enabled would guard against regressions.As per path instructions, "Add Google Test unit tests for new functionality; test files should live alongside source in
src/with the_test.cppsuffix."🤖 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/commands/MorphCommands.cpp` around lines 55 - 99, The fix in removePosesByName should be covered by a regression test that verifies playback is stopped before mutating mesh/entity animation data. Add a Google Test in src/ with the _test.cpp suffix that exercises MorphCommands::removePosesByName while an animation state is enabled, and assert that PropertiesPanelController::setPlaying(false) is effectively triggered and the entity’s AnimationState is disabled/removed without crashing. Use the unique symbols removePosesByName, PropertiesPanelController::setPlaying, and refreshAvailableAnimationState to locate the behavior under test.
🤖 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/AlembicImporter.cpp`:
- Around line 201-226: The readInfo function in AlembicImporter should
explicitly handle empty meshes before calling schema.get on the first sample.
Add a numSamples == 0 guard in readInfo, using the same pattern as readFrameSet,
and set a clear error message through the existing InfoResult r path instead of
relying on an exception from IPolyMeshSchema::get.
In `@src/CLIPipeline.cpp`:
- Around line 2157-2194: The new Alembic `--info` branch in `cmdAnim` returns
before the existing `SentryReporter::addBreadcrumb("cli.anim", ...)` call, so
this user-facing action is not tracked. Add the breadcrumb inside the `infoMode`
path in `CLIPipeline::cmdAnim` before any early return, using the established
`cli.anim` category and a message that identifies the `anim <file>.abc --info`
action, while keeping the normal breadcrumb behavior unchanged for other anim
modes.
In `@src/MCPServer.cpp`:
- Around line 5971-6018: `toolImportAlembic` is calling
`AlembicImporter::importToScene` directly, so any `Ogre::Exception` thrown
during scene/node/entity creation can escape and crash the MCP server. Wrap the
import and the subsequent scene traversal in `runOgreOp`, matching the pattern
used by the other Ogre-backed handlers in `MCPServer`, and convert any thrown
exception into a `makeErrorResult` response instead of letting it propagate.
---
Outside diff comments:
In `@src/AlembicImporter.cpp`:
- Around line 262-286: The buildBaseMesh Alembic face-data read path
dereferences faceIdx and faceCnt without checking for null, unlike the guarded
pattern used in readInfo. Update buildBaseMesh to validate both
first.getFaceIndices() and first.getFaceCounts() before entering the
triangulation loop, and return an empty Ogre::MeshPtr if either sample is
missing so the cursor/size dereferences in the fan-triangulation logic are safe.
- Around line 137-193: The truncated decode path in
AlembicImporter::readFrameSet still treats a single decoded frame as failure
because it relies on FrameSet::ok(), which enforces a 2-frame minimum. Update
the contract so decode success is reported separately from animatability: either
relax the final r.ok assignment to allow one decoded frame when maxFrames limits
the result, or adjust FrameSet::ok()/callers so only the animation path requires
at least two frames. Keep the fix localized around readFrameSet and
VertexAnimationManager::FrameSet.
- Around line 341-395: Wrap the Alembic import flow in importToScene with a
top-level try/catch so exceptions from Manager::addSceneNode and createEntity
are converted into a failed import result instead of escaping through
MCPServer::callTool. Keep the existing fail helper and, in the catch for
Ogre::Exception and a general catch-all, populate the error string and return
nullptr so the tool returns a proper error response.
---
Nitpick comments:
In `@src/commands/MorphCommands.cpp`:
- Around line 55-99: The fix in removePosesByName should be covered by a
regression test that verifies playback is stopped before mutating mesh/entity
animation data. Add a Google Test in src/ with the _test.cpp suffix that
exercises MorphCommands::removePosesByName while an animation state is enabled,
and assert that PropertiesPanelController::setPlaying(false) is effectively
triggered and the entity’s AnimationState is disabled/removed without crashing.
Use the unique symbols removePosesByName, PropertiesPanelController::setPlaying,
and refreshAvailableAnimationState to locate the behavior under test.
🪄 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: 71fbd754-3dbf-485e-8fe7-daf97fd0d7a0
📒 Files selected for processing (9)
CLAUDE.mdqml/PropertiesPanel.qmlsrc/AlembicImporter.cppsrc/AlembicImporter.hsrc/AlembicImporter_test.cppsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/commands/MorphCommands.cpp
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
Review finding (Codex P2 / CodeRabbit Major): buildClipFromFrames removed the old Animation on rebuild but not the dense "<clip>/frameN" poses it created. Ogre poses are mesh-level and are still walked by the dope-sheet / export paths, so re-importing an .abc (or rebuilding any vertex clip) appended stale poses and shifted every pose index. Now removes all same-named frame poses (index-based, erased from the back to keep remaining indices stable — the MorphCommands pattern) before recreating the clip. Test: rebuild with a different frame count leaves only the new poses; a differently-named clip coexists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… guard, CLI breadcrumb - MCP toolImportAlembic now runs importToScene through runOgreOp so an Ogre::Exception from scene-node/entity creation returns a clean MCP error instead of taking down the server (CodeRabbit Major). - readInfo bails when the polymesh has zero samples before schema.get(0) (UB in the Alembic API) — mirrors readFrameSet's existing guard. - `anim <file>.abc --info` now emits a cli.anim Sentry breadcrumb (the early-return branch skipped the shared one downstream). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the reviews — addressed in acf5aa0 and 0f6dc8c: Fixed
Not changing
|
|
…port filter Morph authoring chicken-and-egg fix. Authoring a morph target captures the CURRENT edited vertices (vs the bind pose), which requires Edit Mode — but the only morph UI lived in the Animation-Mode "Animations" section, whose "+ Add…" was gated on Edit Mode. You could never reach both at once. - New "Vertex Morph Animation" CollapsibleSection, shown in Edit Mode. The morph panel (add-from-current-edit, target list + weight sliders, rename/delete) was extracted verbatim from animationComponent into a reusable `vertexMorphComponent` and hosted here. Empty-state hint guides the sculpt→capture flow. - Animation Control section now also opens on hasAnimations (not just skeletal hasAnimation) so the dope sheet appears to key/scrub morph + vertex clips. (The curve editor stays bone-TRS-only by design; morph keyframes live in the dope sheet's Morph Targets rows.) Import file dialog: expand to a full per-format named list (FBX, glTF, OBJ, Collada, Ogre Mesh, STL, PLY, 3DS, Blender, X, BVH, LightWave) plus the Alembic + PlayStation rows — each emitted only when its extension is in the valid list. Previously it was a single collapsed "All supported" row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
qml/PropertiesPanel.qml (4)
7434-7457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMorph target delete has no confirmation, unlike animation delete.
Clicking
×immediately callsdeleteMorphTarget— the per-animation delete flow a few hundred lines above arms a two-click confirm (✓/✗) before callingdeleteAnimationfor the same "irreversible-looking but undoable" reasoning. Consider applying the same confirm pattern here for consistency and to avoid accidental data loss for users who don't reach for Ctrl+Z.🤖 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 7434 - 7457, The morph target delete action is immediate while the animation delete flow uses a confirm/cancel pattern. Update the MouseArea for the morph target delete in PropertiesPanel.qml to follow the same two-step confirmation behavior used by the animation delete UI, so clicking × first arms confirmation and only then calls MorphAnimationManager.deleteMorphTarget(modelData). Reuse the existing confirm-state pattern and related symbols around MorphAnimationManager and the delete button UI to keep the behavior consistent.
484-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
sectionVisiblecondition across two sections.The new "Vertex Morph Animation" section repeats the exact same gating expression as "Edit Mode Tools" immediately above it. Consider a shared computed property (e.g.
root.editModeAvailable) so future changes to Edit Mode gating don't need to be kept in sync across sections.Also applies to: 497-506
🤖 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 484 - 486, The “Vertex Morph Animation” section duplicates the same `sectionVisible` gating used by “Edit Mode Tools”, so the two sections can drift out of sync. Refactor the repeated `root.modeToolSectionVisible(EditorModeController.EditMode, EditModeController.editModeActive)` logic in PropertiesPanel.qml into a shared computed property such as `root.editModeAvailable`, then use that symbol for both sections (including the other affected block) so the visibility rule is defined once and reused consistently.
7415-7426: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded width subtraction can go negative on a narrow panel.
weightSlider.width: parent.width - 222uses the same magic-number pattern the RowLayout refactor a few lines above (7150-7154) was explicitly introduced to fix (per the comment referencing the oldItem { width: parent.width - 320 }bug). On a narrow Inspector this can go negative/degenerate just like the bug it was meant to avoid.♻️ Suggested fix using RowLayout
- Row { - width: morphCol.width - spacing: 4 + RowLayout { + width: morphCol.width + spacing: 4Then give
weightSliderLayout.fillWidth: trueinstead of the fixed subtraction.🤖 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 7415 - 7426, The weightSlider in PropertiesPanel.qml still uses a hardcoded width subtraction that can become negative on narrow panels, reintroducing the same layout bug the nearby RowLayout change was meant to eliminate. Update the Slider block for weightSlider to use the existing layout system instead of width: parent.width - 222, and make it fill available space via Layout.fillWidth so it resizes safely with the inspector. Keep the rest of the weightSlider behavior unchanged, including the value binding and onMoved handler.
7101-7231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew Add/Reset controls lack keyboard accessibility.
addBtnand the "Reset all" rectangle (and the popup's Save/Cancel buttons at lines 7274-7322) only handle mouse clicks — noactiveFocusOnTab,Accessible.role, orKeys.onSpacePressed/Keys.onReturnPressed. Several sibling custom buttons in this same file (isoBtn,skinBtn,RigButton) implement this consistently.🤖 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 7101 - 7231, The new custom controls in PropertiesPanel.qml are mouse-only and need the same keyboard/accessibility treatment as sibling buttons like isoBtn, skinBtn, and RigButton. Update addBtn, the "Reset all" rectangle, and the popup Save/Cancel controls to be focusable via tab, expose an appropriate Accessible.role/label, and activate their existing click behavior from Enter/Return and Space through Keys handlers. Keep the changes localized to the button definitions and their MouseArea/popup handlers so the current mouse behavior remains unchanged.
🤖 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 `@qml/PropertiesPanel.qml`:
- Around line 7434-7457: The morph target delete action is immediate while the
animation delete flow uses a confirm/cancel pattern. Update the MouseArea for
the morph target delete in PropertiesPanel.qml to follow the same two-step
confirmation behavior used by the animation delete UI, so clicking × first arms
confirmation and only then calls
MorphAnimationManager.deleteMorphTarget(modelData). Reuse the existing
confirm-state pattern and related symbols around MorphAnimationManager and the
delete button UI to keep the behavior consistent.
- Around line 484-486: The “Vertex Morph Animation” section duplicates the same
`sectionVisible` gating used by “Edit Mode Tools”, so the two sections can drift
out of sync. Refactor the repeated
`root.modeToolSectionVisible(EditorModeController.EditMode,
EditModeController.editModeActive)` logic in PropertiesPanel.qml into a shared
computed property such as `root.editModeAvailable`, then use that symbol for
both sections (including the other affected block) so the visibility rule is
defined once and reused consistently.
- Around line 7415-7426: The weightSlider in PropertiesPanel.qml still uses a
hardcoded width subtraction that can become negative on narrow panels,
reintroducing the same layout bug the nearby RowLayout change was meant to
eliminate. Update the Slider block for weightSlider to use the existing layout
system instead of width: parent.width - 222, and make it fill available space
via Layout.fillWidth so it resizes safely with the inspector. Keep the rest of
the weightSlider behavior unchanged, including the value binding and onMoved
handler.
- Around line 7101-7231: The new custom controls in PropertiesPanel.qml are
mouse-only and need the same keyboard/accessibility treatment as sibling buttons
like isoBtn, skinBtn, and RigButton. Update addBtn, the "Reset all" rectangle,
and the popup Save/Cancel controls to be focusable via tab, expose an
appropriate Accessible.role/label, and activate their existing click behavior
from Enter/Return and Space through Keys handlers. Keep the changes localized to
the button definitions and their MouseArea/popup handlers so the current mouse
behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bc41cee-8682-408c-a186-c66e6094df8c
📒 Files selected for processing (8)
qml/PropertiesPanel.qmlsrc/AlembicImporter.cppsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cppsrc/VertexAnimationManager.cppsrc/VertexAnimationManager_test.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- src/MeshImporterExporter.cpp
- src/VertexAnimationManager_test.cpp
- src/VertexAnimationManager.cpp
- src/CLIPipeline.cpp
- src/MCPServer.cpp
- src/AlembicImporter.cpp
…Animation list Follow-ups on the Edit-Mode Vertex Morph Animation group: - Add-target popup: focus the name field on onOpened (a Component.onCompleted forceActiveFocus ran while the popup was still hidden, so you couldn't type), and theme it to the Inspector (panel bg/border, inputColor field with a highlight-on-focus border + faded placeholder). - Reorder morph targets: ▲/▼ buttons per row (hidden while filtering; disabled at the ends), backed by MorphAnimationManager::moveMorphTarget(name, ±1) and moveMorphTargetToIndex(name, i) (the latter ready for drag-and-drop). Undoable via a new ReorderMorphTargetsCommand — VAT_POSE keyframes reference poses by index, so it rebuilds every target in the new order (recreating keyframe refs correctly). Ctrl+Z restores the previous order. - Animation Mode list: filter morph-target animations out of PropertiesPanelController::animationData() (each blend shape is a same-named Ogre::Animation, so getAllAnimationStates listed them all as "clips"). The Animations section now shows only real clips (skeletal + Alembic vertex caches, which are not pose names so they survive). Targets live only in the Edit-Mode group now. Morph-only entities are skipped entirely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Implements Slice B of the animation epic (#517 / #519): full-mesh per-vertex animation (cloth, sims, fluid bakes, destruction) — every vertex moves per frame, no skeleton — plus an Alembic (.abc) importer.
What's here
B1 — VertexAnimationManager (
src/VertexAnimationManager.{h,cpp})QML_SINGLETONmanaging full-mesh vertex-animation clips. Reuses Ogre'sVAT_POSEpath, so the existing timeline / dope-sheet / loop play it with no new playback code.FrameSet/FrameDataare source-agnostic decoded-cache types.buildClipFromFrames(mesh, name, frames)reads submesh-0 bind positions and builds oneOgre::Poseper frame (delta vs bind) + oneVAT_POSEtrack keyed per frame time.sampleHeuristic(frameCount)(< 32 → poses, else stream) is static + unit-tested.scene.anim.vertex_anim.B2 — AlembicImporter (
src/AlembicImporter.{h,cpp}, behind-DENABLE_ALEMBIC)cmake/Alembic.cmakeFetchContents Imath 3.1.12 + Alembic 1.8.8 (both BSD-3), fully static, all optional components off. Default OFF — the standard build is unaffected.readFrameSetdecodes the firstIPolyMeshinto aFrameSet(pure data — rejects variable-topology caches, fan-triangulates n-gon faces).importToScenebuilds the base mesh +VAT_POSEclip + entity..abcroutes throughMeshImporterExporter::importerand appears in the import file dialog; a non-Alembic build logs a clear "rebuild with -DENABLE_ALEMBIC" and skips..abc(runs in the Alembic-on coverage lane).B3 — headless parity + robustness
qtmesh anim <file>.abc --info [--json]— cache metadata (frames / verts / tris / fps / duration / storage) viareadInfo, which reads the schema header + first sample only (no full decode).import_alembic(heavy) — decodes an.abcinto the live scene, reporting the created node, entities, and vertex-clip names.play_vertex_animation— plays/stops a vertex clip; delegates to the provenplay_animationpath (a vertex clip is an ordinaryAnimationState).ReadResult::truncated+ a warning fire when it bites. True per-frame vertex-buffer streaming is documented as future work.Fixes folded in
RowLayout(was a magic-number spacer that went negative and overlapped text).hasAnimations, not justhasSkeletonSelection).Tests
VertexAnimationManager_test.cpp— heuristic threshold, FrameSet validity,buildClipFromFramespose/track creation, vertex-count-mismatch rejection, entity enumeration.AlembicImporter_test.cpp— synthetic.abcround-trip (verts / Y positions / times / AABB),readInfomatches a full decode,maxFramescaps + flags truncation, and graceful unavailability without the flag.MeshImporterExporter_test.cpp— import dialog filter contains the Alembic row.Docs updated in
CLAUDE.md(epic-#517 section + CLI examples).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
.abcvertex animation files.--infomode for viewing Alembic cache metadata from the command line.Improvements