Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions src/AnimationControlController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,56 @@
return rows;
}

QVariantList AnimationControlController::allMorphRows() const

Check failure on line 902 in src/AnimationControlController.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 26 to the 25 allowed.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ42vS0_L63nvjzRv3Pe&open=AZ42vS0_L63nvjzRv3Pe&pullRequest=576
{
QVariantList rows;
auto* sel = SelectionSet::getSingleton();

Check warning on line 905 in src/AnimationControlController.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Make the type of this variable a pointer-to-const. The current type of "sel" is "class SelectionSet *".

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ42rHbCGVej_5GpY_ci&open=AZ42rHbCGVej_5GpY_ci&pullRequest=576
if (!sel) return rows;
const auto entities = sel->getResolvedEntities();
if (entities.isEmpty() || !entities.first()) return rows;
Ogre::Entity* entity = entities.first();

Check warning on line 909 in src/AnimationControlController.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Make the type of this variable a pointer-to-const. The current type of "entity" is "class Ogre::Entity *".

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ42rHbCGVej_5GpY_cj&open=AZ42rHbCGVej_5GpY_cj&pullRequest=576
if (!entity) return rows;
Comment on lines +905 to +910
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use the controller’s active entity before falling back to selection-set order.

allMorphRows() currently binds to entities.first(), which can mismatch the dope-sheet’s actively selected clip/entity under multi-selection and show morph rows from the wrong mesh.

Suggested fix
 QVariantList AnimationControlController::allMorphRows() const
 {
     QVariantList rows;
-    auto* sel = SelectionSet::getSingleton();
-    if (!sel) return rows;
-    const auto entities = sel->getResolvedEntities();
-    if (entities.isEmpty() || !entities.first()) return rows;
-    Ogre::Entity* entity = entities.first();
+    Ogre::Entity* entity = m_selectedEntity;
+    if (!entity) {
+        auto* sel = SelectionSet::getSingleton();
+        if (!sel) return rows;
+        const auto entities = sel->getResolvedEntities();
+        if (entities.isEmpty() || !entities.first()) return rows;
+        entity = entities.first();
+    }
     if (!entity) return rows;

Also applies to: 902-950

🤖 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/AnimationControlController.cpp` around lines 905 - 910, In
allMorphRows(), prefer the controller's active entity instead of directly using
SelectionSet::getSingleton()->getResolvedEntities().first(); obtain the
controller's active entity (e.g., via controller->getActiveEntity() or the
controller's activeEntity accessor) and if it's valid use that Ogre::Entity* as
the entity for generating morph rows; only fall back to
SelectionSet::getSingleton() and entities.first() when the controller has no
active entity. Update the logic in the block that currently references
SelectionSet::getSingleton(), entities.first(), and the local Ogre::Entity*
entity so it first queries the controller's active entity, then the
selection-set order (apply same change for the same pattern in the 902-950
region).

Ogre::MeshPtr mesh = entity->getMesh();
if (!mesh) return rows;
const Ogre::PoseList& poseList = mesh->getPoseList();
if (poseList.empty()) return rows;

// Each pose has a matching `Ogre::Animation` (see MeshProcessor:
// import-time one-animation-per-pose pattern). Read the keyframe
// times off the animation's VAT_POSE track; in A1 every animation
// carries a single t=0 keyframe, but future slices may add more.
for (const Ogre::Pose* pose : poseList) {
if (!pose) continue;
const Ogre::String poseName = pose->getName();
if (poseName.empty()) continue;

QVariantList keyTimes;
if (mesh->hasAnimation(poseName)) {
// The importer (MeshProcessor) groups same-named poses
// across submeshes into a single Animation with one
// VAT_POSE track per submesh. Pull only the track
// matching this pose's target handle — otherwise a
// shape that appears on body + head would show its
// diamonds twice in the dope sheet.
Ogre::Animation* anim = mesh->getAnimation(poseName);

Check warning on line 933 in src/AnimationControlController.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Make the type of this variable a pointer-to-const. The current type of "anim" is "class Ogre::Animation *".

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ42rHbCGVej_5GpY_ck&open=AZ42rHbCGVej_5GpY_ck&pullRequest=576
const unsigned short handle = pose->getTarget();
if (anim->hasVertexTrack(handle)) {
Ogre::VertexAnimationTrack* track = anim->getVertexTrack(handle);

Check warning on line 936 in src/AnimationControlController.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Make the type of this variable a pointer-to-const. The current type of "track" is "class Ogre::VertexAnimationTrack *".

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ42vS0_L63nvjzRv3Pg&open=AZ42vS0_L63nvjzRv3Pg&pullRequest=576
if (track && track->getAnimationType() == Ogre::VAT_POSE) {

Check failure on line 937 in src/AnimationControlController.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ42vS0_L63nvjzRv3Pf&open=AZ42vS0_L63nvjzRv3Pf&pullRequest=576
for (unsigned short i = 0; i < track->getNumKeyFrames(); ++i)
keyTimes.append(static_cast<double>(track->getKeyFrame(i)->getTime()));
}
}
}

QVariantMap row;
row[QStringLiteral("name")] = QString::fromStdString(poseName);
row[QStringLiteral("keyTimes")] = keyTimes;
rows.append(row);
}
return rows;
}

bool AnimationControlController::moveKeyframe(const QString& boneName,
double oldTime, double newTime)
{
Expand Down
9 changes: 9 additions & 0 deletions src/AnimationControlController.h
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,15 @@ class AnimationControlController : public QObject
/// Returns an empty list when no animation is selected.
Q_INVOKABLE QVariantList allBoneRows() const;

/// Slice A5: enumerate morph-target tracks for the selected
/// entity so the dope sheet can render them alongside bone
/// tracks. Each entry: `{ name: QString, keyTimes: [double] }`.
/// In A1's importer-emitted Animations, every pose's track has
/// a single keyframe at t=0; future slices may add per-time
/// keys when authoring lands. Returns empty when there's no
/// selection or the entity has no morph targets.
Q_INVOKABLE QVariantList allMorphRows() const;

/// Move a keyframe on `boneName`'s track from `oldTime` to `newTime`
/// (both seconds). Match tolerance is 1 ms — same as the existing
/// keyframe-tick comparison. Refuses the move if `newTime` collides
Expand Down
74 changes: 74 additions & 0 deletions src/AnimationControlController_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@
#include <OgreAnimation.h>
#include <OgreAnimationState.h>
#include <OgreKeyFrame.h>
#include <OgreMesh.h>
#include <OgreMeshManager.h>
#include <OgrePose.h>
#include <OgreSceneManager.h>
#include <OgreSceneNode.h>
#include <OgreSubMesh.h>
#include <OgreEntity.h>

class AnimationControlControllerTest : public ::testing::Test {
protected:
Expand Down Expand Up @@ -1763,3 +1770,70 @@ TEST_F(AnimationControlControllerTest, DeleteKeyframeIsUndoableViaQUndoStack) {
app->processEvents();
EXPECT_EQ(track->getNumKeyFrames(), countBefore - 1);
}

// ── Slice A5: allMorphRows for dope sheet ─────────────────────────────────

TEST_F(AnimationControlControllerTest, AllMorphRowsEmptyWhenNoSelection) {
SelectionSet::getSingleton()->clear();
auto* ctrl = AnimationControlController::instance();
EXPECT_TRUE(ctrl->allMorphRows().isEmpty());
}

TEST_F(AnimationControlControllerTest, AllMorphRowsEmptyForMeshWithoutPoses) {
// createAnimatedTestEntity has bones but no morph targets;
// allMorphRows must return an empty list (not crash).
ASSERT_TRUE(canLoadMeshFiles());
Ogre::Entity* entity = setupAnimatedEntity("Morph_NoPoses");
ASSERT_NE(entity, nullptr);
auto* ctrl = AnimationControlController::instance();
EXPECT_TRUE(ctrl->allMorphRows().isEmpty());
}

TEST_F(AnimationControlControllerTest, AllMorphRowsListsPoseNamesAndKeyTimes) {
ASSERT_TRUE(canLoadMeshFiles());

// Build a fresh mesh + entity with two named poses + matching
// VAT_POSE animations — mirrors what MeshProcessor produces from
// an FBX blend shape. Pose targets handle 0 (shared-vertex
// submesh) to match createInMemoryTriangleMesh's layout.
auto mesh = createInMemoryTriangleMesh("Morph_AllRows");
ASSERT_NE(mesh, nullptr);
{
Ogre::Pose* p = mesh->createPose(0, "JawOpen");
p->addVertex(0, Ogre::Vector3(0, -0.1f, 0));
}
{
Ogre::Pose* p = mesh->createPose(0, "Smile");
p->addVertex(1, Ogre::Vector3(0.05f, 0.02f, 0));
}
const auto& poseList = mesh->getPoseList();
for (unsigned short pi = 0; pi < poseList.size(); ++pi) {
Ogre::Animation* a = mesh->createAnimation(poseList[pi]->getName(), 0.0f);
auto* track = a->createVertexTrack(poseList[pi]->getTarget(), Ogre::VAT_POSE);
auto* kf = track->createVertexPoseKeyFrame(0.0f);
kf->addPoseReference(pi, 1.0f);
}

auto* node = Manager::getSingleton()->addSceneNode("Morph_AllRows_Node");
auto* entity = Manager::getSingleton()->getSceneMgr()->createEntity(node->getName(), mesh->getName());
node->attachObject(entity);
SelectionSet::getSingleton()->selectOne(node);
app->processEvents();

auto* ctrl = AnimationControlController::instance();
QVariantList rows = ctrl->allMorphRows();
ASSERT_EQ(rows.size(), 2);
QSet<QString> names;
for (const QVariant& v : rows) {
const QVariantMap row = v.toMap();
names.insert(row["name"].toString());
// Each pose's Animation has a single keyframe at t=0.
const QVariantList times = row["keyTimes"].toList();
ASSERT_EQ(times.size(), 1);
EXPECT_NEAR(times.first().toDouble(), 0.0, 1e-6);
}
EXPECT_TRUE(names.contains(QStringLiteral("JawOpen")));
EXPECT_TRUE(names.contains(QStringLiteral("Smile")));
}


Loading