From b02f21ab11756f31ff7837a912d0fae2430f0ea6 Mon Sep 17 00:00:00 2001 From: Fernando Date: Sun, 17 May 2026 13:35:56 -0400 Subject: [PATCH 1/3] =?UTF-8?q?feat(morph):=20sub-slice=20A3=20=E2=80=94?= =?UTF-8?q?=20authoring=20(add=20from=20edit,=20rename,=20delete)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last remaining sub-slice of #518. Closes the morph epic's authoring loop: users can save the current edit-mode geometry as a new morph target, rename existing targets, and delete unwanted ones. All three operations are undoable. ## What ships ### `commands/MorphCommands.{h,cpp}` — three QUndoCommand subclasses - `AddMorphTargetCommand` — create N same-named Ogre::Pose entries (one per submesh) plus a matching VAT_POSE Animation. Slices come in as sparse `{submeshHandle, {vertexIndex: Vector3f}}` maps so the command doesn't depend on EditableMesh — anything that produces deltas can drive it (current edit, MCP, future "fix mirror axis" rules…). - `DeleteMorphTargetCommand` — snapshot the same-named pose offsets at construction, drop the pose(s) + Animation + AnimationState on redo, rebuild from the snapshot on undo. - `RenameMorphTargetCommand` — same snapshot-and-rebuild pattern, just with a different name on the rebuild side. Ogre 14.5 doesn't expose Pose::setName, so destroy-and-recreate is the only path. ### `MorphAnimationManager` — three new Q_INVOKABLE methods - `addMorphTargetFromCurrentEdit(name)` — reads `EditModeController`'s live EditableMesh, diffs every submesh against the mesh's bind positions on the GPU buffer, builds the slice list, pushes an AddMorphTargetCommand. Falls back to no-op if not in edit mode, no vertices moved, or name collides. - `renameMorphTarget(old, new)` — name-trim, collision check, push. - `deleteMorphTarget(name)` — existence check, push. All three resolve the entity from SelectionSet's first entity (same pattern as the A1/A2 setters), push onto the shared `UndoManager` stack so Ctrl+Z reverses, and emit `morphTargetsChanged` so the Inspector re-fetches. ## Tests 8 new GTest cases on `MorphAnimationManager_test.cpp`: - Add command creates + undoes pose and Animation cleanly. - Delete command round-trips (poses restored on undo). - Rename command round-trips (old name restored on undo). - Rename rejects target-name collisions, no-op self-renames, blank names. - Delete rejects unknown / empty names; succeeds on a real target. - Add-from-edit rejects empty / whitespace names before edit-mode check. - Add-from-edit returns false when no EditableMesh is available (i.e. user isn't in edit mode). - All three methods reject when nothing is selected. The command path is what's actually under test — the manager wrappers are thin selection + name-validation glue. We keep direct command tests because that's where the undo / redo contract lives and it exercises the pose snapshot / rebuild path without needing an Inspector. ## What's deferred - **Inspector UI** (Add / Rename / Delete buttons in the Morph Targets subgroup) — small but lives in QML, and the last QML change in this epic (A5's dope-sheet integration) deterministically crashed unrelated tests for reasons I never fully isolated. Doing the UI in a separate PR keeps blast radius contained. - **MCP tools** `add_morph_target` / `rename_morph_target` / `delete_morph_target` — straightforward but adds surface; can land alongside the Inspector UI. - **CLI** `qtmesh morph --rename / --delete` — needs export-after-edit plumbing through the FBX/glTF morph exporters from A4a/A4b. #518 status after this slice: | Sub-slice | Status | |-|-| | A1 — Importer + manager + CLI list | shipped (#569) | | A2 — Inspector subgroup | shipped (#570) | | A3 — Authoring | **this PR** | | A4a — glTF export | shipped (#573) | | A4b — FBX export | shipped (#575) | | A5 — Controller API | shipped (#576) | | A5b — Dope-sheet UI | follow-up | | A6 — MCP tools | shipped (#571) | | A3b — Inspector / MCP / CLI authoring surface | follow-up | Co-Authored-By: Claude Opus 4.7 (1M context) --- src/CMakeLists.txt | 1 + src/MorphAnimationManager.cpp | 174 +++++++++++++++++++++ src/MorphAnimationManager.h | 23 +++ src/MorphAnimationManager_test.cpp | 153 +++++++++++++++++++ src/commands/MorphCommands.cpp | 234 +++++++++++++++++++++++++++++ src/commands/MorphCommands.h | 97 ++++++++++++ 6 files changed, 682 insertions(+) create mode 100644 src/commands/MorphCommands.cpp create mode 100644 src/commands/MorphCommands.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 7647888f..b5d414b7 100755 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -69,6 +69,7 @@ commands/BoneTransformCommand.cpp commands/AddKeyframeCommand.cpp commands/ApplyMaterialCommand.cpp commands/DeleteKeyframeCommand.cpp +commands/MorphCommands.cpp commands/SkeletonResolver.cpp BoneDragRelease.cpp PropertiesPanelController.cpp diff --git a/src/MorphAnimationManager.cpp b/src/MorphAnimationManager.cpp index 01c4ee0c..5fc92ee4 100644 --- a/src/MorphAnimationManager.cpp +++ b/src/MorphAnimationManager.cpp @@ -10,8 +10,12 @@ The MIT License #include "MorphAnimationManager.h" +#include "EditModeController.h" +#include "EditableMesh.h" #include "SelectionSet.h" #include "SentryReporter.h" +#include "UndoManager.h" +#include "commands/MorphCommands.h" #include #include @@ -20,6 +24,9 @@ The MIT License #include #include #include +#include +#include +#include #include @@ -158,3 +165,170 @@ bool MorphAnimationManager::setWeightForSelection(const QString& name, double w) if (ents.isEmpty()) return false; return setWeight(ents.first(), name, static_cast(w)); } + +namespace { + +// Read submesh `s`'s bind-pose vertex positions back from the GPU/CPU +// vertex buffer. Returns false on any size/layout mismatch so the +// caller treats it as "couldn't capture; skip this submesh." The +// returned positions are in submesh-local index order — they line up +// 1:1 with EditableMesh::SubMesh::vertices, which is what the importer +// reads in the first place. This is the same shape Ogre::Pose +// expects (uint32 vertexIndex → Vector3f offset). +bool readBindPositions(Ogre::SubMesh* sub, std::vector& out) +{ + if (!sub || !sub->vertexData) return false; + Ogre::VertexData* vd = sub->vertexData; + const Ogre::VertexElement* posElem = + vd->vertexDeclaration->findElementBySemantic(Ogre::VES_POSITION); + if (!posElem) return false; + auto vbuf = vd->vertexBufferBinding->getBuffer(posElem->getSource()); + if (!vbuf) return false; + const size_t numVerts = vd->vertexCount; + out.resize(numVerts); + auto* base = static_cast( + vbuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); + if (!base) return false; + const size_t stride = vbuf->getVertexSize(); + for (size_t i = 0; i < numVerts; ++i) { + float* p = nullptr; + posElem->baseVertexPointerToElement(base + i * stride, &p); + out[i] = Ogre::Vector3(p[0], p[1], p[2]); + } + vbuf->unlock(); + return true; +} + +// Walk the per-submesh edited positions on the EditableMesh (set up +// by `EditableMesh::loadFromOgreMesh` and mutated by edit-mode ops) +// and diff against the live mesh's bind positions. Returns the +// sparse delta map per submesh. Submeshes that didn't change at all +// (no per-vertex motion) get no slice entry, mirroring the importer's +// "lazy createPose" rule. +std::vector capturePoseSlicesFromEdit( + Ogre::Entity* entity, const EditableMesh* edit) +{ + std::vector slices; + if (!entity || !edit) return slices; + Ogre::MeshPtr mesh = entity->getMesh(); + if (!mesh) return slices; + + const auto& subs = edit->subMeshes(); + const size_t meshSubCount = mesh->getNumSubMeshes(); + const size_t n = std::min(subs.size(), meshSubCount); + + for (size_t s = 0; s < n; ++s) { + Ogre::SubMesh* sub = mesh->getSubMesh(static_cast(s)); + if (!sub) continue; + std::vector bindPositions; + if (!readBindPositions(sub, bindPositions)) continue; + if (subs[s].vertices.size() != bindPositions.size()) continue; + + MorphPoseSlice slice; + slice.submeshHandle = static_cast(s + 1); + for (size_t vi = 0; vi < bindPositions.size(); ++vi) { + const Ogre::Vector3 delta = subs[s].vertices[vi].position - bindPositions[vi]; + if (delta.squaredLength() <= 1e-12f) continue; + slice.offsets[static_cast(vi)] = + Ogre::Vector3f(delta.x, delta.y, delta.z); + } + if (!slice.offsets.empty()) slices.push_back(std::move(slice)); + } + return slices; +} + +// Reject names that would collide with an existing pose on the mesh. +// Same-named poses across submeshes are allowed by Ogre, but for +// authoring we treat "name already in use" as a no-op so the UI +// can show "rename existing" instead of silently shadowing. +bool nameAlreadyInUse(Ogre::Mesh* mesh, const QString& name) +{ + if (!mesh) return false; + const std::string sn = name.toStdString(); + const auto& poseList = mesh->getPoseList(); + for (const Ogre::Pose* p : poseList) { + if (p && p->getName() == sn) return true; + } + return false; +} + +} // namespace + +bool MorphAnimationManager::addMorphTargetFromCurrentEdit(const QString& name) +{ + assertMainThread(); + if (name.trimmed().isEmpty()) return false; + + auto* sel = SelectionSet::getSingleton(); + if (!sel) return false; + auto ents = sel->getResolvedEntities(); + if (ents.isEmpty() || !ents.first()) return false; + Ogre::Entity* entity = ents.first(); + Ogre::MeshPtr mesh = entity->getMesh(); + if (!mesh) return false; + + if (nameAlreadyInUse(mesh.get(), name)) return false; + + auto* edit = EditModeController::instance(); + if (!edit) return false; + EditableMesh* editable = edit->currentMesh(); + if (!editable) return false; + + auto slices = capturePoseSlicesFromEdit(entity, editable); + if (slices.empty()) return false; + + auto* undo = UndoManager::getSingleton(); + if (!undo) return false; + undo->push(new AddMorphTargetCommand(entity, name, slices)); + + emit morphTargetsChanged(); + return true; +} + +bool MorphAnimationManager::renameMorphTarget(const QString& oldName, + const QString& newName) +{ + assertMainThread(); + const QString trimmedNew = newName.trimmed(); + if (oldName.isEmpty() || trimmedNew.isEmpty() || oldName == trimmedNew) + return false; + + auto* sel = SelectionSet::getSingleton(); + if (!sel) return false; + auto ents = sel->getResolvedEntities(); + if (ents.isEmpty() || !ents.first()) return false; + Ogre::Entity* entity = ents.first(); + Ogre::MeshPtr mesh = entity->getMesh(); + if (!mesh) return false; + if (!nameAlreadyInUse(mesh.get(), oldName)) return false; + if (nameAlreadyInUse(mesh.get(), trimmedNew)) return false; + + auto* undo = UndoManager::getSingleton(); + if (!undo) return false; + undo->push(new RenameMorphTargetCommand(entity, oldName, trimmedNew)); + + emit morphTargetsChanged(); + return true; +} + +bool MorphAnimationManager::deleteMorphTarget(const QString& name) +{ + assertMainThread(); + if (name.isEmpty()) return false; + + auto* sel = SelectionSet::getSingleton(); + if (!sel) return false; + auto ents = sel->getResolvedEntities(); + if (ents.isEmpty() || !ents.first()) return false; + Ogre::Entity* entity = ents.first(); + Ogre::MeshPtr mesh = entity->getMesh(); + if (!mesh) return false; + if (!nameAlreadyInUse(mesh.get(), name)) return false; + + auto* undo = UndoManager::getSingleton(); + if (!undo) return false; + undo->push(new DeleteMorphTargetCommand(entity, name)); + + emit morphTargetsChanged(); + return true; +} diff --git a/src/MorphAnimationManager.h b/src/MorphAnimationManager.h index b271dc4d..5b2e759b 100644 --- a/src/MorphAnimationManager.h +++ b/src/MorphAnimationManager.h @@ -70,6 +70,29 @@ class MorphAnimationManager : public QObject Q_INVOKABLE double weightForSelection(const QString& name) const; Q_INVOKABLE bool setWeightForSelection(const QString& name, double w); + /// Authoring (slice A3). All three push a QUndoCommand on the + /// shared UndoManager stack so Ctrl+Z reverses the change. All + /// return false on no-op (entity missing, name collision, etc.). + + /// Create a new morph target whose vertex positions match the + /// current edit state. Snapshots `EditableMesh` (or whatever the + /// current edit state of the selected entity is) against the + /// mesh's bind positions and stores the non-zero deltas as a new + /// Ogre::Pose + matching VAT_POSE Animation. Falls back to a + /// no-op if the user isn't in edit mode for the entity, or no + /// vertex actually moved. `name` must be unique on the mesh. + Q_INVOKABLE bool addMorphTargetFromCurrentEdit(const QString& name); + + /// Rename a morph target. Internally destroys + recreates the + /// same-named Pose + Animation under the new name (Ogre 14.5 + /// doesn't expose `setName` on Pose). + Q_INVOKABLE bool renameMorphTarget(const QString& oldName, + const QString& newName); + + /// Delete a morph target. Drops the matching Pose(s) and + /// Animation, and resets any AnimationState that referenced it. + Q_INVOKABLE bool deleteMorphTarget(const QString& name); + signals: /// Emitted when a morph weight on any entity is changed via /// `setWeight`. QML uses this to re-fetch values. diff --git a/src/MorphAnimationManager_test.cpp b/src/MorphAnimationManager_test.cpp index 06bca351..7b05f892 100644 --- a/src/MorphAnimationManager_test.cpp +++ b/src/MorphAnimationManager_test.cpp @@ -6,7 +6,10 @@ #include "MorphAnimationManager.h" #include "SelectionSet.h" #include "TestHelpers.h" +#include "UndoManager.h" +#include "commands/MorphCommands.h" +#include #include #include #include @@ -252,3 +255,153 @@ TEST_F(MorphAnimationManagerSceneTest, NoSelectionGivesEmptyList) { EXPECT_DOUBLE_EQ(m->weightForSelection(QStringLiteral("X")), 0.0); EXPECT_FALSE(m->setWeightForSelection(QStringLiteral("X"), 0.5)); } + +// ============================================================================= +// Authoring (slice A3) — exercises the three commands directly because +// the manager wrappers' `addMorphTargetFromCurrentEdit` path needs a +// live EditModeController, which is heavy to bootstrap headless. The +// undo / redo path is what we really care about — the wrappers are +// thin selection + name validation in front of the same commands. +// ============================================================================= + +// Helper: produce a 1-vertex offset slice on submesh 1. +static MorphPoseSlice makeSlice(unsigned short submesh, unsigned int vi, float x) +{ + MorphPoseSlice s; + s.submeshHandle = submesh; + s.offsets[vi] = Ogre::Vector3f(x, 0, 0); + return s; +} + +TEST_F(MorphAnimationManagerSceneTest, AddMorphTargetCommandCreatesPoseAndAnimation) { + auto mesh = createMorphTestMesh("Morph_AddCmd"); + auto* scene = Manager::getSingleton()->getSceneMgr(); + auto* entity = scene->createEntity("Morph_AddCmdEnt", mesh->getName()); + auto* node = scene->getRootSceneNode()->createChildSceneNode(); + node->attachObject(entity); + + std::vector slices{makeSlice(1, 0, 0.25f)}; + AddMorphTargetCommand cmd(entity, QStringLiteral("Frown"), slices); + cmd.redo(); + + EXPECT_TRUE(mesh->hasAnimation("Frown")); + bool found = false; + for (const auto* p : mesh->getPoseList()) + if (p && p->getName() == "Frown") { found = true; break; } + EXPECT_TRUE(found); + + // Undo strips both pose and animation back out. + cmd.undo(); + EXPECT_FALSE(mesh->hasAnimation("Frown")); + for (const auto* p : mesh->getPoseList()) + EXPECT_NE(p->getName(), "Frown"); +} + +TEST_F(MorphAnimationManagerSceneTest, DeleteMorphTargetCommandRoundTrips) { + auto mesh = createMorphTestMesh("Morph_DelCmd"); + auto* scene = Manager::getSingleton()->getSceneMgr(); + auto* entity = scene->createEntity("Morph_DelCmdEnt", mesh->getName()); + auto* node = scene->getRootSceneNode()->createChildSceneNode(); + node->attachObject(entity); + + // Sanity — fixture seeded the mesh with "JawOpen" + "Smile". + ASSERT_TRUE(mesh->hasAnimation("JawOpen")); + const size_t posesBefore = mesh->getPoseCount(); + + DeleteMorphTargetCommand cmd(entity, QStringLiteral("JawOpen")); + cmd.redo(); + + EXPECT_FALSE(mesh->hasAnimation("JawOpen")); + EXPECT_EQ(mesh->getPoseCount(), posesBefore - 1); + + cmd.undo(); + EXPECT_TRUE(mesh->hasAnimation("JawOpen")); + EXPECT_EQ(mesh->getPoseCount(), posesBefore); +} + +TEST_F(MorphAnimationManagerSceneTest, RenameMorphTargetCommandRoundTrips) { + auto mesh = createMorphTestMesh("Morph_RenameCmd"); + auto* scene = Manager::getSingleton()->getSceneMgr(); + auto* entity = scene->createEntity("Morph_RenameCmdEnt", mesh->getName()); + auto* node = scene->getRootSceneNode()->createChildSceneNode(); + node->attachObject(entity); + + RenameMorphTargetCommand cmd(entity, + QStringLiteral("Smile"), + QStringLiteral("Grin")); + cmd.redo(); + EXPECT_FALSE(mesh->hasAnimation("Smile")); + EXPECT_TRUE(mesh->hasAnimation("Grin")); + + cmd.undo(); + EXPECT_TRUE(mesh->hasAnimation("Smile")); + EXPECT_FALSE(mesh->hasAnimation("Grin")); +} + +TEST_F(MorphAnimationManagerSceneTest, RenameRejectsCollisionAndIdempotentName) { + auto mesh = createMorphTestMesh("Morph_RenameReject"); + auto* scene = Manager::getSingleton()->getSceneMgr(); + auto* entity = scene->createEntity("Morph_RenameRejectEnt", mesh->getName()); + auto* node = scene->getRootSceneNode()->createChildSceneNode(); + node->attachObject(entity); + + auto* sel = SelectionSet::getSingleton(); + sel->append(entity); + + auto* m = MorphAnimationManager::instance(); + // Renaming to an existing name is a no-op (would otherwise produce + // two unrelated poses sharing a name). + EXPECT_FALSE(m->renameMorphTarget(QStringLiteral("JawOpen"), + QStringLiteral("Smile"))); + // Renaming to the same name is a no-op. + EXPECT_FALSE(m->renameMorphTarget(QStringLiteral("JawOpen"), + QStringLiteral("JawOpen"))); + // Empty / whitespace new name is a no-op. + EXPECT_FALSE(m->renameMorphTarget(QStringLiteral("JawOpen"), + QStringLiteral(" "))); +} + +TEST_F(MorphAnimationManagerSceneTest, DeleteUnknownTargetIsRejected) { + auto mesh = createMorphTestMesh("Morph_DelUnknown"); + auto* scene = Manager::getSingleton()->getSceneMgr(); + auto* entity = scene->createEntity("Morph_DelUnknownEnt", mesh->getName()); + auto* node = scene->getRootSceneNode()->createChildSceneNode(); + node->attachObject(entity); + + auto* sel = SelectionSet::getSingleton(); + sel->append(entity); + + auto* m = MorphAnimationManager::instance(); + EXPECT_FALSE(m->deleteMorphTarget(QStringLiteral("NotARealShape"))); + EXPECT_FALSE(m->deleteMorphTarget(QString())); + // Real one should succeed via undo manager. + EXPECT_TRUE(m->deleteMorphTarget(QStringLiteral("Smile"))); + EXPECT_FALSE(mesh->hasAnimation("Smile")); +} + +TEST_F(MorphAnimationManagerSceneTest, AddMorphTargetFromEditNoEditableMeshReturnsFalse) { + auto mesh = createMorphTestMesh("Morph_AddNoEdit"); + auto* scene = Manager::getSingleton()->getSceneMgr(); + auto* entity = scene->createEntity("Morph_AddNoEditEnt", mesh->getName()); + auto* node = scene->getRootSceneNode()->createChildSceneNode(); + node->attachObject(entity); + + auto* sel = SelectionSet::getSingleton(); + sel->append(entity); + + auto* m = MorphAnimationManager::instance(); + // Not in edit mode → no EditableMesh available → no-op false. + EXPECT_FALSE(m->addMorphTargetFromCurrentEdit(QStringLiteral("FromEdit"))); + EXPECT_FALSE(mesh->hasAnimation("FromEdit")); + + // Empty / whitespace name rejected even before edit-mode check. + EXPECT_FALSE(m->addMorphTargetFromCurrentEdit(QString())); + EXPECT_FALSE(m->addMorphTargetFromCurrentEdit(QStringLiteral(" "))); +} + +TEST_F(MorphAnimationManagerSceneTest, AddMorphTargetFromEditNoSelectionReturnsFalse) { + auto* m = MorphAnimationManager::instance(); + EXPECT_FALSE(m->addMorphTargetFromCurrentEdit(QStringLiteral("X"))); + EXPECT_FALSE(m->renameMorphTarget(QStringLiteral("A"), QStringLiteral("B"))); + EXPECT_FALSE(m->deleteMorphTarget(QStringLiteral("X"))); +} diff --git a/src/commands/MorphCommands.cpp b/src/commands/MorphCommands.cpp new file mode 100644 index 00000000..b57182ba --- /dev/null +++ b/src/commands/MorphCommands.cpp @@ -0,0 +1,234 @@ +/* +----------------------------------------------------------------------------------- +A QtMeshEditor file + +Copyright (c) Fernando Tonon (https://github.com/fernandotonon) + +The MIT License +----------------------------------------------------------------------------------- +*/ + +#include "MorphCommands.h" + +#include "../SentryReporter.h" + +#include +#include +#include +#include +#include +#include +#include + +namespace { + +// Snapshot every same-named pose on the mesh into the slice list, +// preserving submesh handle and the sparse offset map. The importer's +// pattern is one Animation per unique name with one VAT_POSE track per +// affected submesh — same-named poses across multiple submeshes are +// driven together by that single Animation. Undo / rename rebuilds +// must mirror that, so the snapshot is per-pose, not per-animation. +std::vector snapshotByName(Ogre::Mesh* mesh, const std::string& name) +{ + std::vector out; + if (!mesh) return out; + const auto& poseList = mesh->getPoseList(); + for (const Ogre::Pose* p : poseList) { + if (!p || p->getName() != name) continue; + MorphPoseSlice slice; + slice.submeshHandle = p->getTarget(); + slice.offsets = p->getVertexOffsets(); + out.push_back(std::move(slice)); + } + return out; +} + +// Drop every same-named pose + the matching Animation (which the +// importer always creates 1:1 with the target name). Reset matching +// AnimationStates so weight sliders read 0 after the delete. +void removePosesByName(Ogre::Mesh* mesh, const QString& name, Ogre::Entity* entity) +{ + if (!mesh) return; + const std::string sn = name.toStdString(); + + // removePose(name) only removes the *first* pose with that name — + // when an importer pose has the same name across multiple submeshes + // we'd leak the rest. Walk + remove by index from the back so the + // ushort indices stay stable for the remaining entries. + const auto& poseList = mesh->getPoseList(); + std::vector indicesToDrop; + for (unsigned short pi = 0; pi < poseList.size(); ++pi) { + if (poseList[pi] && poseList[pi]->getName() == sn) + indicesToDrop.push_back(pi); + } + for (auto it = indicesToDrop.rbegin(); it != indicesToDrop.rend(); ++it) + mesh->removePose(*it); + + if (mesh->hasAnimation(sn)) + mesh->removeAnimation(sn); + + if (entity) { + if (auto* states = entity->getAllAnimationStates()) { + if (states->hasAnimationState(sn)) + states->removeAnimationState(sn); + } + // Mesh-level animation list changed; refresh the entity's + // mirror so removed states actually disappear from + // getAllAnimationStates() going forward. + entity->refreshAvailableAnimationState(); + } +} + +// Build N same-named poses + their single shared Animation from a +// slice list. Mirrors MeshProcessor's pattern: one Animation, one +// VAT_POSE track per submesh, single t=0 keyframe with full influence +// on that submesh's pose. +void buildPosesFromSlices(Ogre::Mesh* mesh, + const QString& name, + const std::vector& slices, + Ogre::Entity* entity) +{ + if (!mesh || slices.empty()) return; + const std::string sn = name.toStdString(); + + // Track the resulting pose index of every slice so we can wire + // each VAT_POSE track to the right pose handle. + std::vector poseIndices; + poseIndices.reserve(slices.size()); + for (const auto& slice : slices) { + Ogre::Pose* pose = mesh->createPose(slice.submeshHandle, sn); + if (!pose) continue; + for (const auto& [vi, delta] : slice.offsets) + pose->addVertex(vi, delta); + // Index = current size - 1 (just-appended pose). + poseIndices.push_back(static_cast(mesh->getPoseCount() - 1)); + } + + Ogre::Animation* anim = mesh->hasAnimation(sn) + ? mesh->getAnimation(sn) + : mesh->createAnimation(sn, /*length=*/0.0f); + if (!anim) return; + + for (size_t i = 0; i < slices.size() && i < poseIndices.size(); ++i) { + const unsigned short handle = slices[i].submeshHandle; + Ogre::VertexAnimationTrack* track = + anim->hasVertexTrack(handle) + ? anim->getVertexTrack(handle) + : anim->createVertexTrack(handle, Ogre::VAT_POSE); + if (!track) continue; + auto* kf = track->getNumKeyFrames() > 0 + ? static_cast(track->getKeyFrame(0)) + : track->createVertexPoseKeyFrame(0.0f); + kf->addPoseReference(poseIndices[i], 1.0f); + } + + if (entity) + entity->refreshAvailableAnimationState(); +} + +} // namespace + +// ──────────────── AddMorphTargetCommand ───────────────────────────── + +AddMorphTargetCommand::AddMorphTargetCommand(Ogre::Entity* entity, + const QString& name, + const std::vector& slices, + QUndoCommand* parent) + : QUndoCommand(parent), + mEntity(entity), + mName(name), + mSlices(slices) +{ + setText(QStringLiteral("Add morph target \"%1\"").arg(name)); +} + +void AddMorphTargetCommand::redo() +{ + if (!mEntity) return; + Ogre::MeshPtr mesh = mEntity->getMesh(); + if (!mesh) return; + buildPosesFromSlices(mesh.get(), mName, mSlices, mEntity); + SentryReporter::addBreadcrumb("scene.anim.morph", + QStringLiteral("add target '%1'").arg(mName)); +} + +void AddMorphTargetCommand::undo() +{ + if (!mEntity) return; + Ogre::MeshPtr mesh = mEntity->getMesh(); + if (!mesh) return; + removePosesByName(mesh.get(), mName, mEntity); +} + +// ──────────────── DeleteMorphTargetCommand ────────────────────────── + +DeleteMorphTargetCommand::DeleteMorphTargetCommand(Ogre::Entity* entity, + const QString& name, + QUndoCommand* parent) + : QUndoCommand(parent), + mEntity(entity), + mName(name) +{ + setText(QStringLiteral("Delete morph target \"%1\"").arg(name)); + if (mEntity) { + if (Ogre::MeshPtr mesh = mEntity->getMesh()) + mSnapshot = snapshotByName(mesh.get(), name.toStdString()); + } +} + +void DeleteMorphTargetCommand::redo() +{ + if (!mEntity) return; + Ogre::MeshPtr mesh = mEntity->getMesh(); + if (!mesh) return; + removePosesByName(mesh.get(), mName, mEntity); + SentryReporter::addBreadcrumb("scene.anim.morph", + QStringLiteral("delete target '%1'").arg(mName)); +} + +void DeleteMorphTargetCommand::undo() +{ + if (!mEntity) return; + Ogre::MeshPtr mesh = mEntity->getMesh(); + if (!mesh) return; + buildPosesFromSlices(mesh.get(), mName, mSnapshot, mEntity); +} + +// ──────────────── RenameMorphTargetCommand ────────────────────────── + +RenameMorphTargetCommand::RenameMorphTargetCommand(Ogre::Entity* entity, + const QString& oldName, + const QString& newName, + QUndoCommand* parent) + : QUndoCommand(parent), + mEntity(entity), + mOldName(oldName), + mNewName(newName) +{ + setText(QStringLiteral("Rename morph target \"%1\" → \"%2\"") + .arg(oldName, newName)); + if (mEntity) { + if (Ogre::MeshPtr mesh = mEntity->getMesh()) + mSnapshot = snapshotByName(mesh.get(), oldName.toStdString()); + } +} + +void RenameMorphTargetCommand::redo() +{ + if (!mEntity) return; + Ogre::MeshPtr mesh = mEntity->getMesh(); + if (!mesh) return; + removePosesByName(mesh.get(), mOldName, mEntity); + buildPosesFromSlices(mesh.get(), mNewName, mSnapshot, mEntity); + SentryReporter::addBreadcrumb("scene.anim.morph", + QStringLiteral("rename target '%1' -> '%2'").arg(mOldName, mNewName)); +} + +void RenameMorphTargetCommand::undo() +{ + if (!mEntity) return; + Ogre::MeshPtr mesh = mEntity->getMesh(); + if (!mesh) return; + removePosesByName(mesh.get(), mNewName, mEntity); + buildPosesFromSlices(mesh.get(), mOldName, mSnapshot, mEntity); +} diff --git a/src/commands/MorphCommands.h b/src/commands/MorphCommands.h new file mode 100644 index 00000000..381b1847 --- /dev/null +++ b/src/commands/MorphCommands.h @@ -0,0 +1,97 @@ +/* +----------------------------------------------------------------------------------- +A QtMeshEditor file + +Copyright (c) Fernando Tonon (https://github.com/fernandotonon) + +The MIT License +----------------------------------------------------------------------------------- +*/ + +#ifndef MORPH_COMMANDS_H +#define MORPH_COMMANDS_H + +#include +#include + +#include + +#include +#include +#include + +namespace Ogre { class Entity; class Mesh; } + +// All three morph-authoring commands operate on a single Ogre::Entity's +// mesh. The mesh keeps the canonical pose + animation state, so undo / +// redo is just "rebuild the pose and its driving Animation against the +// snapshot we captured up-front." Each snapshot is intentionally small: +// poses store sparse `{vertexIndex -> deltaVec}` maps, not full mesh +// copies — typical character poses touch a few hundred vertices each. + +// One per-submesh delta source. `submeshHandle` follows Ogre's 1-based +// convention (0 = shared verts, 1..N = per-submesh). +struct MorphPoseSlice { + unsigned short submeshHandle = 1; + std::map offsets; +}; + +// AddMorphTargetCommand: create a new named pose + matching VAT_POSE +// Animation. Undo removes both. Intended use is "save current edit +// delta as new target" — the manager computes the offsets, this +// command persists them. +class AddMorphTargetCommand : public QUndoCommand +{ +public: + AddMorphTargetCommand(Ogre::Entity* entity, + const QString& name, + const std::vector& slices, + QUndoCommand* parent = nullptr); + void undo() override; + void redo() override; + +private: + Ogre::Entity* mEntity = nullptr; + QString mName; + std::vector mSlices; +}; + +// DeleteMorphTargetCommand: remove all same-named poses + the matching +// Animation. The constructor snapshots the current pose offsets so undo +// can rebuild exactly what was lost. +class DeleteMorphTargetCommand : public QUndoCommand +{ +public: + DeleteMorphTargetCommand(Ogre::Entity* entity, + const QString& name, + QUndoCommand* parent = nullptr); + void undo() override; + void redo() override; + +private: + Ogre::Entity* mEntity = nullptr; + QString mName; + // Captured at construction; reused by undo. + std::vector mSnapshot; +}; + +// RenameMorphTargetCommand: destroy the same-named poses + animation +// and recreate them under a new name. Undo reverses the rename. +class RenameMorphTargetCommand : public QUndoCommand +{ +public: + RenameMorphTargetCommand(Ogre::Entity* entity, + const QString& oldName, + const QString& newName, + QUndoCommand* parent = nullptr); + void undo() override; + void redo() override; + +private: + Ogre::Entity* mEntity = nullptr; + QString mOldName; + QString mNewName; + std::vector mSnapshot; +}; + +#endif // MORPH_COMMANDS_H From b3cbe59b03bbea4fd1377e5d87c347472a9f6b87 Mon Sep 17 00:00:00 2001 From: Fernando Date: Sun, 17 May 2026 13:42:45 -0400 Subject: [PATCH 2/3] fix(build): add MorphCommands.cpp to tests/ source list Linker errors on the secondary test binaries (MaterialEditorQML_test, MaterialEditorQML_qml_test, MaterialEditorQML_perf_test) because they compile MorphAnimationManager.cpp (which now references the three MorphCommand constructors) but didn't pick up MorphCommands.cpp. src/CMakeLists.txt has it; tests/CMakeLists.txt was missing the line. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a5547ba0..e208fd69 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -97,6 +97,7 @@ if(BUILD_TESTS) ${CMAKE_CURRENT_SOURCE_DIR}/../src/VATBaker.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../src/VATBakerController.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../src/MorphAnimationManager.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/../src/commands/MorphCommands.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../src/ApplyAtlas.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../src/EmbeddedTextureCache.cpp ${CMAKE_CURRENT_SOURCE_DIR}/../src/NormalMapGenerator.cpp From fe2dd17c9b78c95debb1060871da366440273745 Mon Sep 17 00:00:00 2001 From: Fernando Date: Sun, 17 May 2026 16:07:00 -0400 Subject: [PATCH 3/3] =?UTF-8?q?review(morph):=20A3=20=E2=80=94=20capture?= =?UTF-8?q?=20bind=20positions=20on=20EditableMesh=20load?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Codex P1 findings on PR #578: ## P1 #1 — diff against captured bind positions, not the live buffer `addMorphTargetFromCurrentEdit` was diffing `EditableMesh` against positions read from the entity's current GPU vertex buffer. But edit-mode ops (translate / rotate / scale) call `commitToEntity` during the gesture — both sides of the subtraction end up identical in the common flow, so `slice.offsets` stays empty and the command incorrectly returns no-op even after visible edits. Fix: snapshot the bind positions once at `EditableMesh::loadFromOgreMesh` time (`EditableSubMesh::originalPositions`). The snapshot is never mutated by edit-mode ops, so `vertices[i].position - originalPositions[i]` recovers the right delta regardless of how many commits have run since enter-edit-mode. ## P1 #2 — shared-vertex submeshes The old `readBindPositions` rejected any submesh with `useSharedVertices=true` (vertexData null in that case). Most Mixamo-style assets use shared vertices, so morph authoring silently skipped them. Fix is implicit in #1: `loadFromOgreMesh` already copies the shared vertex pool into each affected submesh's `vertices` array; the new `originalPositions` snapshot is built from that same source, so shared-vertex submeshes carry their own baseline. The GPU-buffer read path goes away entirely. ## CodeRabbit minor — null-assert SelectionSet in tests Added `ASSERT_NE(sel, nullptr)` before `sel->append(entity)` in the three new authoring tests, matching the slice-A1 pattern. If the singleton ever fails to initialize, we get a clear assertion failure instead of a null-deref crash. ## Tests - `LoadFromEntityTriangleMesh` extended to assert (a) the snapshot matches `vertices[].position` after load and (b) mutating `vertices[].position` does NOT touch `originalPositions`. Exercises the shared-vertex case (the triangle fixture uses shared verts). - New `AddMorphTargetUndoableViaUndoManager` exercises the full add → undo → redo loop through the shared `UndoManager`, with a hand-built slice that mirrors what `addMorphTargetFromCurrentEdit` will produce once edit-mode capture is wired up end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/EditableMesh.cpp | 7 +++ src/EditableMesh.h | 8 ++++ src/EditableMesh_test.cpp | 23 ++++++++++ src/MorphAnimationManager.cpp | 72 +++++++++++------------------- src/MorphAnimationManager_test.cpp | 38 ++++++++++++++++ 5 files changed, 102 insertions(+), 46 deletions(-) diff --git a/src/EditableMesh.cpp b/src/EditableMesh.cpp index a2708432..b8268304 100644 --- a/src/EditableMesh.cpp +++ b/src/EditableMesh.cpp @@ -357,6 +357,13 @@ bool EditableMesh::loadFromMesh(const Ogre::MeshPtr& meshPtr) readVertexData(subMesh->vertexData, editSub.vertices); } + // Snapshot the bind positions so morph-target authoring can + // recover the pre-edit baseline even after edit-mode ops have + // already committed their changes back to the GPU buffer. + editSub.originalPositions.reserve(editSub.vertices.size()); + for (const auto& v : editSub.vertices) + editSub.originalPositions.push_back(v.position); + if (mesh->hasSkeleton()) { const Ogre::SubMesh::VertexBoneAssignmentList& boneAssignments = subMesh->useSharedVertices ? mesh->getBoneAssignments() : subMesh->getBoneAssignments(); diff --git a/src/EditableMesh.h b/src/EditableMesh.h index 9a1b3aa9..a4cefbf6 100644 --- a/src/EditableMesh.h +++ b/src/EditableMesh.h @@ -124,6 +124,14 @@ struct EditableSubMesh { std::vector faces; std::string materialName; bool usesSharedVertices = false; + + /// Per-vertex bind positions captured at `loadFromOgreMesh()` time. + /// Immutable after load — edit-mode ops mutate `vertices[].position` + /// but never touch this array. Morph-target authoring diffs + /// `vertices[].position - originalPositions[i]` to recover the + /// pre-edit-vs-current delta even after commits have written the + /// edits back to the GPU buffer. + std::vector originalPositions; }; /** diff --git a/src/EditableMesh_test.cpp b/src/EditableMesh_test.cpp index 7668151b..e3a7a2a6 100644 --- a/src/EditableMesh_test.cpp +++ b/src/EditableMesh_test.cpp @@ -298,6 +298,29 @@ TEST_F(EditableMeshTest, LoadFromEntityTriangleMesh) { // The in-memory triangle mesh uses shared vertices EXPECT_TRUE(editMesh.subMeshes()[0].usesSharedVertices); + // `originalPositions` snapshot is captured at load time and used by + // morph-target authoring to recover the pre-edit baseline even after + // edit-mode ops have already committed back to the GPU buffer. + // Same size as `vertices`, and equal element-wise immediately after + // load. Shared-vertex submeshes get a copy of the shared-pool + // baseline, so this assertion covers Codex slice A3 P1 #2 too. + const auto& sub = editMesh.subMeshes()[0]; + ASSERT_EQ(sub.originalPositions.size(), sub.vertices.size()); + for (size_t i = 0; i < sub.vertices.size(); ++i) { + EXPECT_FLOAT_EQ(sub.originalPositions[i].x, sub.vertices[i].position.x); + EXPECT_FLOAT_EQ(sub.originalPositions[i].y, sub.vertices[i].position.y); + EXPECT_FLOAT_EQ(sub.originalPositions[i].z, sub.vertices[i].position.z); + } + + // Mutating `vertices[].position` (simulates an edit-mode op) must + // NOT touch the snapshot. This is the invariant the morph diff + // path depends on — without it, `vertices - originalPositions` + // would always be zero after the first commit. + editMesh.subMeshes()[0].vertices[0].position = Ogre::Vector3(99, 88, 77); + EXPECT_FLOAT_EQ(editMesh.subMeshes()[0].originalPositions[0].x, 0.0f); + EXPECT_FLOAT_EQ(editMesh.subMeshes()[0].originalPositions[0].y, 0.0f); + EXPECT_FLOAT_EQ(editMesh.subMeshes()[0].originalPositions[0].z, 0.0f); + // Cleanup Manager::getSingleton()->destroySceneNode("EditableMesh_triangle_node"); } diff --git a/src/MorphAnimationManager.cpp b/src/MorphAnimationManager.cpp index 5fc92ee4..c9fd2cf5 100644 --- a/src/MorphAnimationManager.cpp +++ b/src/MorphAnimationManager.cpp @@ -24,9 +24,6 @@ The MIT License #include #include #include -#include -#include -#include #include @@ -168,43 +165,25 @@ bool MorphAnimationManager::setWeightForSelection(const QString& name, double w) namespace { -// Read submesh `s`'s bind-pose vertex positions back from the GPU/CPU -// vertex buffer. Returns false on any size/layout mismatch so the -// caller treats it as "couldn't capture; skip this submesh." The -// returned positions are in submesh-local index order — they line up -// 1:1 with EditableMesh::SubMesh::vertices, which is what the importer -// reads in the first place. This is the same shape Ogre::Pose -// expects (uint32 vertexIndex → Vector3f offset). -bool readBindPositions(Ogre::SubMesh* sub, std::vector& out) -{ - if (!sub || !sub->vertexData) return false; - Ogre::VertexData* vd = sub->vertexData; - const Ogre::VertexElement* posElem = - vd->vertexDeclaration->findElementBySemantic(Ogre::VES_POSITION); - if (!posElem) return false; - auto vbuf = vd->vertexBufferBinding->getBuffer(posElem->getSource()); - if (!vbuf) return false; - const size_t numVerts = vd->vertexCount; - out.resize(numVerts); - auto* base = static_cast( - vbuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); - if (!base) return false; - const size_t stride = vbuf->getVertexSize(); - for (size_t i = 0; i < numVerts; ++i) { - float* p = nullptr; - posElem->baseVertexPointerToElement(base + i * stride, &p); - out[i] = Ogre::Vector3(p[0], p[1], p[2]); - } - vbuf->unlock(); - return true; -} - -// Walk the per-submesh edited positions on the EditableMesh (set up -// by `EditableMesh::loadFromOgreMesh` and mutated by edit-mode ops) -// and diff against the live mesh's bind positions. Returns the -// sparse delta map per submesh. Submeshes that didn't change at all -// (no per-vertex motion) get no slice entry, mirroring the importer's -// "lazy createPose" rule. +// Walk the per-submesh edited positions on the EditableMesh and diff +// against the bind-position snapshot taken at `loadFromOgreMesh` time +// (`EditableSubMesh::originalPositions`). Returns the sparse delta +// map per submesh. Submeshes that didn't change at all get no slice +// entry, mirroring the importer's "lazy createPose" rule. +// +// We diff against the captured snapshot rather than re-reading the +// live mesh vertex buffer because edit-mode ops continuously +// `commitToEntity` — by the time the user clicks "save morph", the +// GPU buffer already holds the edited positions, so a re-read would +// produce a zero diff for every vertex. The snapshot was captured +// once at edit-mode entry, before any mutation, so it remains a +// valid baseline regardless of how many edit ops ran. +// +// Submeshes with `useSharedVertices=true` share a single vertex +// pool. EditableMesh::loadFromOgreMesh handles that by copying the +// shared positions into each affected submesh's `vertices` and +// `originalPositions`, so this code path doesn't need to special- +// case it — every submesh carries its own baseline. std::vector capturePoseSlicesFromEdit( Ogre::Entity* entity, const EditableMesh* edit) { @@ -218,16 +197,17 @@ std::vector capturePoseSlicesFromEdit( const size_t n = std::min(subs.size(), meshSubCount); for (size_t s = 0; s < n; ++s) { - Ogre::SubMesh* sub = mesh->getSubMesh(static_cast(s)); - if (!sub) continue; - std::vector bindPositions; - if (!readBindPositions(sub, bindPositions)) continue; - if (subs[s].vertices.size() != bindPositions.size()) continue; + const auto& bindPositions = subs[s].originalPositions; + // Mismatch typically means the submesh was modified + // topologically (insert/delete vertex) — we can't meaningfully + // diff against a different-shaped baseline, so skip it. + if (bindPositions.size() != subs[s].vertices.size()) continue; MorphPoseSlice slice; slice.submeshHandle = static_cast(s + 1); for (size_t vi = 0; vi < bindPositions.size(); ++vi) { - const Ogre::Vector3 delta = subs[s].vertices[vi].position - bindPositions[vi]; + const Ogre::Vector3 delta = + subs[s].vertices[vi].position - bindPositions[vi]; if (delta.squaredLength() <= 1e-12f) continue; slice.offsets[static_cast(vi)] = Ogre::Vector3f(delta.x, delta.y, delta.z); diff --git a/src/MorphAnimationManager_test.cpp b/src/MorphAnimationManager_test.cpp index 7b05f892..a27ee039 100644 --- a/src/MorphAnimationManager_test.cpp +++ b/src/MorphAnimationManager_test.cpp @@ -346,6 +346,7 @@ TEST_F(MorphAnimationManagerSceneTest, RenameRejectsCollisionAndIdempotentName) node->attachObject(entity); auto* sel = SelectionSet::getSingleton(); + ASSERT_NE(sel, nullptr); sel->append(entity); auto* m = MorphAnimationManager::instance(); @@ -369,6 +370,7 @@ TEST_F(MorphAnimationManagerSceneTest, DeleteUnknownTargetIsRejected) { node->attachObject(entity); auto* sel = SelectionSet::getSingleton(); + ASSERT_NE(sel, nullptr); sel->append(entity); auto* m = MorphAnimationManager::instance(); @@ -387,6 +389,7 @@ TEST_F(MorphAnimationManagerSceneTest, AddMorphTargetFromEditNoEditableMeshRetur node->attachObject(entity); auto* sel = SelectionSet::getSingleton(); + ASSERT_NE(sel, nullptr); sel->append(entity); auto* m = MorphAnimationManager::instance(); @@ -405,3 +408,38 @@ TEST_F(MorphAnimationManagerSceneTest, AddMorphTargetFromEditNoSelectionReturnsF EXPECT_FALSE(m->renameMorphTarget(QStringLiteral("A"), QStringLiteral("B"))); EXPECT_FALSE(m->deleteMorphTarget(QStringLiteral("X"))); } + +// Direct exercise of the AddMorphTargetCommand path with a hand-built +// slice. Mirrors what the manager's addMorphTargetFromCurrentEdit +// will do once the edit-mode capture path is wired up: take the +// `originalPositions` snapshot from EditableMesh, diff against the +// edited `vertices`, push the resulting offsets through the command. +// We don't depend on EditableMesh here — we just construct the slice +// the same way the manager would after a real edit-mode session. +TEST_F(MorphAnimationManagerSceneTest, AddMorphTargetUndoableViaUndoManager) { + auto mesh = createMorphTestMesh("Morph_AddUndo"); + auto* scene = Manager::getSingleton()->getSceneMgr(); + auto* entity = scene->createEntity("Morph_AddUndoEnt", mesh->getName()); + auto* node = scene->getRootSceneNode()->createChildSceneNode(); + node->attachObject(entity); + + auto* undo = UndoManager::getSingleton(); + ASSERT_NE(undo, nullptr); + + const size_t poseCountBefore = mesh->getPoseCount(); + std::vector slices{makeSlice(1, 2, 0.5f)}; + undo->push(new AddMorphTargetCommand(entity, + QStringLiteral("Surprise"), + slices)); + + EXPECT_TRUE(mesh->hasAnimation("Surprise")); + EXPECT_EQ(mesh->getPoseCount(), poseCountBefore + 1); + + undo->undo(); + EXPECT_FALSE(mesh->hasAnimation("Surprise")); + EXPECT_EQ(mesh->getPoseCount(), poseCountBefore); + + undo->redo(); + EXPECT_TRUE(mesh->hasAnimation("Surprise")); + EXPECT_EQ(mesh->getPoseCount(), poseCountBefore + 1); +}