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 18 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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional Alembic ChangesAlembic vertex animation and morph workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 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>
… vertex edit Slice 1 (Blender-style non-destructive authoring): - EditModeController gains a morph sculpt session (beginMorphSculpt / endMorphSculpt + morphSculptActive). beginMorphSculpt snapshots the base vertex positions; endMorphSculpt (and exitEditMode) RESTORE them, so morph authoring never permanently changes the base mesh — matching Blender shape keys / Maya blend shapes. The captured target lives on as a Pose + weight, independent of the base. - EditableMesh::commitToEntity now refreshes the pose/vertex-animation buffer for entities with vertex animation (getAllAnimationStates()->_notifyDirty() + _updateAnimation()). Without this, moving vertices on a mesh that carries morph targets didn't update the render, because Ogre draws from the per-frame pose buffer (not the mesh VBO) and the frame loop is paused during authoring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…port
Slice 2 — animate morph target weights on the timeline, and export the result.
Backend (MorphAnimationManager):
- setMorphWeightKeyframe(name, time, weight) / clearMorphWeightKeyframe /
morphWeightKeyframeTimes. A single mesh Animation named kWeightClipName
("MorphAnim") holds one VAT_POSE track per target's pose; each keyframe
references the pose at influence == the weight at that time. Plays via the
existing AnimationState path; the clip length auto-extends to cover keys.
UI (Edit-Mode Vertex Morph Animation group):
- Per-target "◈ Key" button records the current weight at the timeline
playhead (AnimationControlController.sliderValue). Diamonds show on the dope
sheet — allMorphRows() now prefers the MorphAnim clip's per-pose keyframe
times over the static shape-only clip.
Export (glTF):
- buildAiScene now emits an aiMeshMorphAnim weight-animation channel from the
MorphAnim clip (blend-shape TARGET export via aiMesh::mAnimMeshes already
existed). Skeletal + morph animations are gathered into one list and sized
together; no-skeleton morph-only meshes now export their animation too.
FBX blend-shape weight export remains a follow-up.
Tests: reorder command + moveMorphTarget API, and the weight-keyframe clip
(create/update-in-place/clear/length + no-op rejections).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…port Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…BJ authoring fixes - Multiple named morph clips (smile/angry/surprised) over shared targets: MorphAnimationManager gains activeMorphClip + create/delete/rename/list; keyframe methods write to the active clip; clip selector dropdown in the Vertex Morph group; glTF exports every clip as its own morph-weights anim. - Drag-drop target reorder (grip handle, live cursor follow + insertion line), replacing the up/down buttons; middle-ellipsis on target names. - Full dope-sheet morph editing (add/drag/delete diamonds); "◈ Key" activates the clip so keying is immediately playable/scrubbable. - Timeline length + scrub now work for mesh-level (VAT_POSE) clips, not just skeletal (AnimationControlController::selectAnimation/setAnimationLength). - OBJ authoring fix: loadFromAssimpFile (n-gon path) now snapshots originalPositions so morph capture diffs correctly (was orig=0 → "no vertex edited"). createMorphClip requires ≥1 target (no crash on unmorphed OBJ). - Crash fixes: rename of a mesh morph clip routes to renameMorphClip (was the skeleton path → null-skeleton crash); SkeletonTransform::renameAnimation guards hasSkeleton. Dope-sheet shows for morph-only meshes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ertex-alembic # Conflicts: # CMakeLists.txt # qml/PropertiesPanel.qml # src/CLIPipeline.cpp # src/MeshImporterExporter.cpp # src/MorphAnimationManager.cpp
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MeshImporterExporter.cpp (2)
2412-2421: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRecord Alembic imports with a
file.importbreadcrumb.As per coding guidelines, all significant I/O operations must use the corresponding
file.import/file.exportbreadcrumb category.🤖 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 2412 - 2421, Record a file.import breadcrumb for the Alembic I/O operation in the Alembic handling block of MeshImporterExporter, before or when calling AlembicImporter::importToScene, using the project’s standard breadcrumb logging mechanism and including the imported file context.Source: Coding guidelines
2420-2426: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not bypass the shared post-import finalization.
continueskips Lines 2943-2968, so Alembic nodes miss position normalization, small-mesh auto-scaling, andconfigureCamera(). Assignsn/enfromabcNodeand fall through to the common tail.Proposed integration
Ogre::SceneNode* abcNode = AlembicImporter::importToScene(file.absoluteFilePath(), &abcErr); if (!abcNode) { Ogre::LogManager::getSingleton().logError( "Alembic import failed: " + abcErr.toStdString()); + continue; } -continue; +sn = abcNode; +en = nullptr; +for (unsigned short i = 0; i < sn->numAttachedObjects(); ++i) { + Ogre::MovableObject* object = sn->getAttachedObject(i); + if (object && object->getMovableType() == "Entity") { + en = static_cast<Ogre::Entity*>(object); + break; + } +} +if (!en) + 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 2420 - 2426, Replace the Alembic branch’s unconditional continue with shared-tail flow: after import, assign the imported node to the variables used by the common finalization path (sn/en) and preserve error handling for a null abcNode. Ensure execution reaches the post-import logic that performs normalization, small-mesh scaling, and configureCamera(), while avoiding finalization when import fails.
🧹 Nitpick comments (1)
src/MorphAnimationManager.cpp (1)
291-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd breadcrumbs for destructive morph edits.
Successful key deletion, clip deletion, and clip rename currently emit no breadcrumb. Record them using the established
scene.anim.morphcategory.As per coding guidelines, “Track all user-facing actions and significant operations with
SentryReporter::addBreadcrumb.”Also applies to: 493-495, 535-538
🤖 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/MorphAnimationManager.cpp` around lines 291 - 292, Add Sentry breadcrumbs in the successful branches of the key deletion, clip deletion, and clip rename operations in MorphAnimationManager, immediately before their existing success returns or change signals. Use SentryReporter::addBreadcrumb with the established “scene.anim.morph” category and descriptive messages identifying each destructive edit.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/MorphCommands.cpp`:
- Around line 283-294: Update the reorder logic around removePosesByName and
buildPosesFromSlices to remap every mesh-level VAT_POSE clip’s keyframe pose
indices from the old morph-target order to the new order. Preserve the remapped
clip data across both redo() and undo(), and ensure clips referencing unchanged
targets remain valid.
In `@src/MeshImporterExporter.cpp`:
- Around line 1291-1334: Preserve morph targets and morph animations in
buildSceneAiScene() by mirroring buildAiScene()’s attachMorphTargetsToAiMesh()
and morph-clip collection logic. Apply it to each mesh using the entity’s global
mesh-index offset, resolve the correct mesh node name, skip shape clips, and
append generated animations to the scene’s existing animation list without
overwriting skeletal animations.
In `@src/MorphAnimationManager.cpp`:
- Around line 514-517: Stop playback before rebuilding the renamed morph clip:
in the rename logic around the source animation lookup and recreation, tear down
or disable the active animation state using the same cleanup approach as the
delete path before calling createAnimation and removing the old animation.
- Around line 204-207: Update setMorphWeightKeyframe() to preserve existing
VAT_POSE entries: when setting or clearing a target, modify/remove only the
entry matching its poseIndex rather than clearing the entire keyframe, and
remove the keyframe only when no pose references remain. Add a regression test
covering two targets on the same clip, submesh track, and timestamp, verifying
both references survive and can be cleared independently.
In `@src/PropertiesPanelController.cpp`:
- Around line 1159-1160: Resolve the target entity from entityName before
handling the rename, rather than relying on the first selected entity. Use that
entity for morph-clip existence checks and pass it explicitly to the
entity-scoped APIs used by renameMorphClip, ensuring the request routes to the
morph path for the named entity.
---
Outside diff comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 2412-2421: Record a file.import breadcrumb for the Alembic I/O
operation in the Alembic handling block of MeshImporterExporter, before or when
calling AlembicImporter::importToScene, using the project’s standard breadcrumb
logging mechanism and including the imported file context.
- Around line 2420-2426: Replace the Alembic branch’s unconditional continue
with shared-tail flow: after import, assign the imported node to the variables
used by the common finalization path (sn/en) and preserve error handling for a
null abcNode. Ensure execution reaches the post-import logic that performs
normalization, small-mesh scaling, and configureCamera(), while avoiding
finalization when import fails.
---
Nitpick comments:
In `@src/MorphAnimationManager.cpp`:
- Around line 291-292: Add Sentry breadcrumbs in the successful branches of the
key deletion, clip deletion, and clip rename operations in
MorphAnimationManager, immediately before their existing success returns or
change signals. Use SentryReporter::addBreadcrumb with the established
“scene.anim.morph” category and descriptive messages identifying each
destructive edit.
🪄 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: d648d201-cd16-4025-8f6d-ebc128624193
📒 Files selected for processing (24)
CLAUDE.mdCMakeLists.txtqml/AnimationDopeSheet.qmlqml/PropertiesPanel.qmlsrc/AnimationControlController.cppsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/EditModeController.cppsrc/EditModeController.hsrc/EditableMesh.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/Manager.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cppsrc/MorphAnimationManager.cppsrc/MorphAnimationManager.hsrc/MorphAnimationManager_test.cppsrc/PropertiesPanelController.cppsrc/SkeletonTransform.cppsrc/commands/MorphCommands.cppsrc/commands/MorphCommands.hsrc/mainwindow.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 (9)
- src/MCPServer.h
- CMakeLists.txt
- src/CMakeLists.txt
- src/mainwindow.cpp
- tests/CMakeLists.txt
- src/Manager.cpp
- src/CLIPipeline.cpp
- src/MeshImporterExporter_test.cpp
- src/MCPServer.cpp
| // Pose indices are positional and VAT_POSE keyframes reference them by | ||
| // index, so the only safe reorder is a full teardown + rebuild in the | ||
| // desired name-order. removePosesByName drops each target's poses + | ||
| // Animation; buildPosesFromSlices recreates them (and their keyframe | ||
| // references) fresh, in call order → the new display order. | ||
| for (const QString& n : order) | ||
| removePosesByName(mesh.get(), n, mEntity); | ||
| for (const QString& n : order) { | ||
| auto it = mSnapshot.find(n); | ||
| if (it != mSnapshot.end()) | ||
| buildPosesFromSlices(mesh.get(), n, it->second, mEntity); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby symbols.
git ls-files 'src/commands/MorphCommands.cpp'
ast-grep outline src/commands/MorphCommands.cpp --view expanded
# Read the relevant section with line numbers.
sed -n '1,420p' src/commands/MorphCommands.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 14766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all morph-animation related code paths and pose-reference handling.
rg -n "MorphAnim|weight clip|addPoseReference|removeAnimationState|refreshAvailableAnimationState|VAT_POSE|getPoseList|getAnimation\(" src
# Read the command header for the class/state it stores.
sed -n '1,220p' src/commands/MorphCommands.h | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared morph-weight clip implementation and clip rebuild/rename paths.
sed -n '170,560p' src/MorphAnimationManager.cpp | cat -n
# Inspect the reorder-related tests for expected behavior.
sed -n '430,520p' src/MorphAnimationManager_test.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 23973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reorder API and any weight-clip handling around it.
sed -n '600,760p' src/MorphAnimationManager.cpp | cat -n
# Inspect the morph command declarations for any extra state stored for reorder.
sed -n '1,220p' src/commands/MorphCommands.h | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 11044
Remap mesh-level VAT_POSE clips when reordering morph targets
src/commands/MorphCommands.cpp:283-294 rebuilds the poses in a new index order, but leaves existing VAT_POSE clips untouched. Their keyframes still point at the old pose indices, so redo() and undo() can make morph playback drive the wrong target.
🤖 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 283 - 294, Update the reorder
logic around removePosesByName and buildPosesFromSlices to remap every
mesh-level VAT_POSE clip’s keyframe pose indices from the old morph-target order
to the new order. Preserve the remapped clip data across both redo() and undo(),
and ensure clips referencing unchanged targets remain valid.
| // Skeletal animations first (unchanged), then optionally one morph-weight | ||
| // animation from the mesh's "MorphAnim" clip. Both are gathered into a | ||
| // vector so the mAnimations array is sized to hold exactly what exists — | ||
| // this covers the no-skeleton + morph-only case (mAnimations was previously | ||
| // left null) and the has-skeleton + morph case (grow past the skeleton | ||
| // count) without special-casing either. | ||
| std::vector<aiAnimation*> animations; | ||
| if (hasSkeleton && skeleton->getNumAnimations() > 0) | ||
| { | ||
| scene->mNumAnimations = skeleton->getNumAnimations(); | ||
| scene->mAnimations = new aiAnimation*[scene->mNumAnimations]; | ||
| for (unsigned short ai = 0; ai < skeleton->getNumAnimations(); ++ai) | ||
| scene->mAnimations[ai] = buildAiAnimation(skeleton->getAnimation(ai)); | ||
| animations.push_back(buildAiAnimation(skeleton->getAnimation(ai))); | ||
| } | ||
|
|
||
| // The node every submesh attaches to (see node wiring above): the dedicated | ||
| // "<name>_mesh" node when there's a skeleton, else the root node itself. | ||
| const std::string meshNodeName = hasSkeleton | ||
| ? std::string(entity->getName()) + "_mesh" | ||
| : std::string(entity->getName()); | ||
| // Export EVERY morph (weight) clip — smile / angry / surprised — as its own | ||
| // glTF morph-weights animation. A morph clip is a mesh Animation carrying a | ||
| // VAT_POSE track that is NOT a per-target shape clip (named exactly a pose | ||
| // name). Shape clips are the static single-key blend-shape definitions; the | ||
| // targets themselves are emitted via aiMesh::mAnimMeshes. | ||
| for (unsigned short ai = 0; ai < mesh->getNumAnimations(); ++ai) | ||
| { | ||
| Ogre::Animation* a = mesh->getAnimation(ai); | ||
| if (!a) continue; | ||
| const std::string nm = a->getName(); | ||
| // Skip per-target shape clips (name == a pose name). | ||
| bool isShapeClip = false; | ||
| for (const Ogre::Pose* p : mesh->getPoseList()) | ||
| if (p && p->getName() == nm) { isShapeClip = true; break; } | ||
| if (isShapeClip) continue; | ||
| if (aiAnimation* morphAnim = | ||
| buildMorphWeightAiAnimation(scene, mesh, meshNodeName, nm)) | ||
| animations.push_back(morphAnim); | ||
| } | ||
|
|
||
| if (!animations.empty()) | ||
| { | ||
| scene->mNumAnimations = static_cast<unsigned int>(animations.size()); | ||
| scene->mAnimations = new aiAnimation*[scene->mNumAnimations]; // NOSONAR — Assimp owns | ||
| for (unsigned int ai = 0; ai < scene->mNumAnimations; ++ai) | ||
| scene->mAnimations[ai] = animations[ai]; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve morph targets and clips in whole-scene exports.
This wiring exists only in buildAiScene(). buildSceneAiScene() neither calls attachMorphTargetsToAiMesh() nor appends morph animations, so saving a scene silently drops authored morph data. Reuse this logic there with the entity’s global mesh-index offset.
🤖 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 1291 - 1334, Preserve morph
targets and morph animations in buildSceneAiScene() by mirroring
buildAiScene()’s attachMorphTargetsToAiMesh() and morph-clip collection logic.
Apply it to each mesh using the entity’s global mesh-index offset, resolve the
correct mesh node name, skip shape clips, and append generated animations to the
scene’s existing animation list without overwriting skeletal animations.
| if (anim->hasVertexTrack(handle)) | ||
| return anim->getVertexTrack(handle); | ||
| if (!create) return nullptr; | ||
| return anim->createVertexTrack(handle, Ogre::VAT_POSE); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant methods.
ast-grep outline src/MorphAnimationManager.cpp --view expanded || true
echo
echo "== Relevant line ranges =="
sed -n '180,320p' src/MorphAnimationManager.cpp | cat -n
echo
echo "== Search for poseIndex / vertex track handling =="
rg -n "poseIndex|hasVertexTrack|createVertexTrack|getVertexTrack|removeVertexTrack|VAT_POSE|clear.*Morph|rename.*Morph" src/MorphAnimationManager.cppRepository: fernandotonon/QtMeshEditor
Length of output: 10584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the clip rename/copy path that clones VAT_POSE keyframes.
sed -n '498,530p' src/MorphAnimationManager.cpp | cat -n
echo
echo "== Pose-reference writes =="
rg -n "addPoseReference|removeAllPoseReferences|removeKeyFrame|getPoseReferences" src/MorphAnimationManager.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for morph/animation tests and any existing multi-reference expectations.
git ls-files | rg -n "test|spec|MorphAnimationManager|Morph|Animation" || true
echo
echo "== Morph-related tests or fixtures =="
rg -n "setMorphWeightKeyframe|clearMorphWeightKeyframe|renameMorphClip|addPoseReference|getPoseReferences|removeAllPoseReferences" . \
-g '!**/build/**' -g '!**/dist/**' -g '!**/node_modules/**' || trueRepository: fernandotonon/QtMeshEditor
Length of output: 20839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the existing MorphAnimationManager tests around weight keyframes and clip renames.
sed -n '240,360p' src/MorphAnimationManager_test.cpp | cat -n
echo
echo "== Test helpers / mesh setup =="
sed -n '1,140p' src/MorphAnimationManager_test.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 12652
Preserve other pose references on shared VAT_POSE keyframes.
setMorphWeightKeyframe() clears the whole keyframe before adding the current pose, so a second target at the same time on the same submesh track overwrites the first one, and clearing one target deletes the shared keyframe entirely. Update/remove only the matching poseIndex, and drop the keyframe only when no pose references remain. Add a same-clip, same-time multi-target regression test.
🤖 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/MorphAnimationManager.cpp` around lines 204 - 207, Update
setMorphWeightKeyframe() to preserve existing VAT_POSE entries: when setting or
clearing a target, modify/remove only the entry matching its poseIndex rather
than clearing the entire keyframe, and remove the keyframe only when no pose
references remain. Add a regression test covering two targets on the same clip,
submesh track, and timestamp, verifying both references survive and can be
cleared independently.
| // Rebuild the Animation under the new name copying every VAT_POSE track + | ||
| // keyframe + pose reference, then drop the old one. (Ogre has no rename.) | ||
| Ogre::Animation* src = mesh->getAnimation(on); | ||
| Ogre::Animation* dst = mesh->createAnimation(nn, src->getLength()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the referenced lines
file="src/MorphAnimationManager.cpp"
wc -l "$file"
sed -n '470,560p' "$file"
# Search for related playback/state control and rename handling
rg -n "setPlaying\\(|getAllAnimationStates\\(|getAnimationState\\(|rename|createAnimation\\(|removeAnimation\\(" src include . -g '!**/build/**' -g '!**/dist/**' -g '!**/node_modules/**' | sed -n '1,220p'Repository: fernandotonon/QtMeshEditor
Length of output: 26422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1140,1195p' src/PropertiesPanelController.cpp
sed -n '1,220p' src/MorphAnimationManager.cpp | sed -n '498,540p'
rg -n "renameMorphClip\\(" src -g '!**/build/**' -g '!**/dist/**' -g '!**/node_modules/**'Repository: fernandotonon/QtMeshEditor
Length of output: 3165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect morph-clip specific tests and any playback/rename assumptions
sed -n '300,380p' src/MorphAnimationManager_test.cpp
sed -n '560,620p' src/PropertiesPanelController_test.cpp
sed -n '1140,1215p' src/PropertiesPanelController.cppRepository: fernandotonon/QtMeshEditor
Length of output: 9966
Stop playback before rebuilding a renamed morph clip. src/MorphAnimationManager.cpp:514-534 recreates and removes the animation while its state can still be active; mirror the delete path and tear playback down first to avoid stale animation-state references.
🤖 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/MorphAnimationManager.cpp` around lines 514 - 517, Stop playback before
rebuilding the renamed morph clip: in the rename logic around the source
animation lookup and recreation, tear down or disable the active animation state
using the same cleanup approach as the delete path before calling
createAnimation and removing the old animation.
| if (MorphAnimationManager::instance()->morphClips().contains(oldName)) | ||
| return MorphAnimationManager::instance()->renameMorphClip(oldName, newName); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve entityName before routing the rename.
morphClips() and renameMorphClip() operate on the first selected entity, while this API explicitly names the target entity. With multiple selections, this can rename a different entity’s morph clip or route the requested morph clip through the skeletal path.
Find the matching entity first and pass it explicitly to entity-scoped morph-clip inspection and rename APIs.
🤖 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/PropertiesPanelController.cpp` around lines 1159 - 1160, Resolve the
target entity from entityName before handling the rename, rather than relying on
the first selected entity. Use that entity for morph-clip existence checks and
pass it explicitly to the entity-scoped APIs used by renameMorphClip, ensuring
the request routes to the morph path for the named entity.
|
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
.abcvertex animations, plus ananim ... --infoCLI mode for cache metadata.