test: add unit tests to raise Sonar coverage - #359
Merged
Conversation
…ubMesh edges - BevelGizmo: null SceneManager safety, axis/visibility/pick/screen scale, parallel ray - Euler: forward/right/up, quaternion order, round-trip, direction, normalise - ScanConfig: loadFromFile YAML/JSON, missing path, invalid JSON fallback - SubMeshTransform: null entity no-ops, invalid submesh index Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds comprehensive GoogleTest test coverage for four core classes: BevelGizmo (with fixture-based Ogre/Qt environment initialization), Euler (quaternion conversions and angle normalization), ScanConfig (file loading with YAML/JSON parsing), and SubMeshTransform (edge cases with null pointers and invalid indices). ChangesBevelGizmo Test Suite
Euler Test Suite
ScanConfig File Loading Test Suite
SubMeshTransform Edge Cases Test Suite
Estimated Code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 3 minutes and 10 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/ScanEngine_test.cpp (1)
1695-1751: CloseQTemporaryFilebeforeloadFromFile()for clarity and best practiceAll three tests (
ValidYaml,ValidJson,InvalidJsonFallsBackToDefaults) callScanConfig::loadFromFile(file.fileName())while the file is still open. While Qt'sQTemporaryFilegrantsFILE_SHARE_READby default on Windows and concurrent reading will succeed, closing the file afterflush()and before the read operation is a clearer, more conventional pattern that avoids ambiguity.Add
file.close();afterfile.flush();in all three tests.QTemporaryFileretains ownership of the path and auto-removes on destruction even afterclose().Suggested fix (apply to all three tests)
file.write(yaml); // (or json content, or invalid content) file.flush(); + file.close(); ScanConfig c = ScanConfig::loadFromFile(file.fileName());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScanEngine_test.cpp` around lines 1695 - 1751, The tests call ScanConfig::loadFromFile while the QTemporaryFile is still open; close the temporary file explicitly after file.flush() to avoid ambiguity on Windows and follow best practice. In each test function (LoadFromFile_ValidYaml, LoadFromFile_ValidJson, LoadFromFile_InvalidJsonFallsBackToDefaults) add file.close(); immediately after the existing file.flush(); so the file is closed before calling ScanConfig::loadFromFile(file.fileName()).src/Euler_test.cpp (1)
7-55: 💤 Low valueWell-structured test suite — one optional coverage gap noted.
The quaternion equivalence checks using
|q.dotProduct(q2)| ≈ 1are the correct idiom for rotation comparison; the tolerances are appropriate. A couple of optional observations worth considering if you want to strengthen the suite:
positionEqualsvsdirectionEquals(lines 10–12) —positionEqualsis semantically designed for spatial positions (component-wise absolute tolerance), whiledirectionEqualsuses angle-based tolerance suited for direction vectors. Works correctly here since the expected values are exact axis-aligned unit vectors, butdirectionEqualsis the more semantically precise choice.
RelativeYawPitchRollfrom identity (lines 40–47) —yaw(Radian),pitch(Radian),roll(Radian)appear to be relative (additive) operations given the distinctsetYaw/setPitch/setRollabsolute setters used inNormaliseWrapsLargeYaw. Starting from the zero-angleEuler()default makes relative and absolute operations numerically identical, so this test cannot distinguish a broken relative implementation from a correct absolute one. Seeding with a non-zero initial Euler before the relative calls would make the relative semantics verifiable.💡 Stronger `RelativeYawPitchRoll` starting from a non-zero base
TEST(EulerTest, RelativeYawPitchRoll) { - Euler e; - e.yaw(Radian(0.1f)).pitch(Radian(0.2f)).roll(Radian(0.3f)); - EXPECT_NEAR(e.yaw().valueRadians(), 0.1f, 1e-5f); - EXPECT_NEAR(e.pitch().valueRadians(), 0.2f, 1e-5f); - EXPECT_NEAR(e.roll().valueRadians(), 0.3f, 1e-5f); + // Seed with known non-zero angles so relative adds are distinguishable from absolute sets + Euler e; + e.setYaw(Radian(0.5f)); + e.setPitch(Radian(0.4f)); + e.setRoll(Radian(0.3f)); + e.yaw(Radian(0.1f)).pitch(Radian(0.2f)).roll(Radian(0.3f)); + EXPECT_NEAR(e.yaw().valueRadians(), 0.6f, 1e-5f); + EXPECT_NEAR(e.pitch().valueRadians(), 0.6f, 1e-5f); + EXPECT_NEAR(e.roll().valueRadians(), 0.6f, 1e-5f); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Euler_test.cpp` around lines 7 - 55, Replace the use of positionEquals in the IdentityForwardRightUp test with the direction-oriented comparison and make RelativeYawPitchRoll actually verify relative (additive) behavior by seeding the Euler with a non-zero base before calling the relative mutators: use directionEquals (or direction-based angle checks) for Vector3 comparisons instead of positionEquals (referencing Euler::forward, Euler::right, Euler::up and Vector3::UNIT_*/NEGATIVE_UNIT_Z), and change the RelativeYawPitchRoll test to construct or set e to a non-zero initial Euler (via setYaw/setPitch/setRoll or Euler ctor), then call e.yaw(Radian(...)), e.pitch(Radian(...)), e.roll(Radian(...)) and assert the resulting yaw/pitch/roll equals initial + delta (compare values via yaw().valueRadians(), pitch().valueRadians(), roll().valueRadians()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/BevelGizmo_test.cpp`:
- Line 56: The test currently calls gizmo.setScale(0.0f) which produces a
singular (non-invertible) transform; change the call to use a small positive
near-zero value instead (e.g., replace gizmo.setScale(0.0f) with
gizmo.setScale(1e-6f) or a named constant like NEAR_ZERO_SCALE) so the transform
remains invertible; update any test-local constant or add a small constexpr
(e.g., NEAR_ZERO_SCALE or kEpsilon) and use that with setScale to preserve the
boundary condition without producing degenerate matrices.
- Line 56: The test currently calls gizmo.setScale(0.0f) which produces a
singular (non-invertible) transform; change the call in BevelGizmo_test.cpp to
use a small positive epsilon (e.g. 1e-6f or similar) instead of 0.0f so the
transform remains invertible and avoids Ogre warnings/assertions—update the
invocation of setScale on the gizmo object to use that tiny positive value.
- Around line 25-35: Add a TearDown() to the BevelGizmoTest fixture that mirrors
the cleanup performed in SetUp: call Manager::kill() and QThread::msleep(50) to
ensure Ogre is torn down after each test (so objects like the "BevelGizmoUT_Cam"
created in AxisVisibilityScaleAndPick are not left live); implement TearDown()
as an override in the BevelGizmoTest class to perform the same teardown steps
currently in SetUp.
- Around line 25-35: Add a TearDown() to the BevelGizmoTest fixture that
symmetrically cleans up the Ogre session created in SetUp: call Manager::kill()
(and any necessary waits like QThread::msleep(50)) and explicitly destroy or
release objects created in tests (e.g., the camera named "BevelGizmoUT_Cam"
created via sm->createCamera()) so no Ogre resources remain after the last test;
implement TearDown() in the BevelGizmoTest class to reverse SetUp() actions and
ensure the Ogre scene manager/camera/resources are properly removed before
returning.
In `@src/ScanEngine_test.cpp`:
- Around line 1695-1751: The tests write to a QTemporaryFile and call
ScanConfig::loadFromFile while the QTemporaryFile is still open, which can cause
Windows sharing violations; fix each test (LoadFromFile_ValidYaml,
LoadFromFile_ValidJson, LoadFromFile_InvalidJsonFallsBackToDefaults) by calling
file.close() immediately after file.flush() and before calling
ScanConfig::loadFromFile(file.fileName()) — keep setAutoRemove(true) as-is since
closing the QTemporaryFile does not remove the temporary file until the
QTemporaryFile object is destroyed.
---
Nitpick comments:
In `@src/Euler_test.cpp`:
- Around line 7-55: Replace the use of positionEquals in the
IdentityForwardRightUp test with the direction-oriented comparison and make
RelativeYawPitchRoll actually verify relative (additive) behavior by seeding the
Euler with a non-zero base before calling the relative mutators: use
directionEquals (or direction-based angle checks) for Vector3 comparisons
instead of positionEquals (referencing Euler::forward, Euler::right, Euler::up
and Vector3::UNIT_*/NEGATIVE_UNIT_Z), and change the RelativeYawPitchRoll test
to construct or set e to a non-zero initial Euler (via setYaw/setPitch/setRoll
or Euler ctor), then call e.yaw(Radian(...)), e.pitch(Radian(...)),
e.roll(Radian(...)) and assert the resulting yaw/pitch/roll equals initial +
delta (compare values via yaw().valueRadians(), pitch().valueRadians(),
roll().valueRadians()).
In `@src/ScanEngine_test.cpp`:
- Around line 1695-1751: The tests call ScanConfig::loadFromFile while the
QTemporaryFile is still open; close the temporary file explicitly after
file.flush() to avoid ambiguity on Windows and follow best practice. In each
test function (LoadFromFile_ValidYaml, LoadFromFile_ValidJson,
LoadFromFile_InvalidJsonFallsBackToDefaults) add file.close(); immediately after
the existing file.flush(); so the file is closed before calling
ScanConfig::loadFromFile(file.fileName()).
🪄 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: 5390d055-591a-43bc-bbdf-05139dad21fe
📒 Files selected for processing (4)
src/BevelGizmo_test.cppsrc/Euler_test.cppsrc/ScanEngine_test.cppsrc/commands/TransformCommands_test.cpp
Comment on lines
+25
to
+35
| class BevelGizmoTest : public ::testing::Test { | ||
| protected: | ||
| void SetUp() override | ||
| { | ||
| Manager::kill(); | ||
| QThread::msleep(50); | ||
| ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr); | ||
| ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; | ||
| createStandardOgreMaterials(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing TearDown() leaves Ogre state dirty after the last test.
The fixture resets Ogre at the start of each test via Manager::kill() in SetUp, but never after the last one completes. Concretely, the camera "BevelGizmoUT_Cam" allocated via sm->createCamera() in AxisVisibilityScaleAndPick (line 59) is never explicitly destroyed. Any test suite that runs after BevelGizmoTest in the same binary will inherit a live Ogre session with dangling scene objects.
Add a TearDown() that mirrors SetUp's teardown logic:
🔧 Proposed fix
class BevelGizmoTest : public ::testing::Test {
protected:
void SetUp() override
{
Manager::kill();
QThread::msleep(50);
ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr);
ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)";
createStandardOgreMaterials();
}
+
+ void TearDown() override
+ {
+ Manager::kill();
+ QThread::msleep(50);
+ }
};📝 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.
Suggested change
| class BevelGizmoTest : public ::testing::Test { | |
| protected: | |
| void SetUp() override | |
| { | |
| Manager::kill(); | |
| QThread::msleep(50); | |
| ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr); | |
| ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; | |
| createStandardOgreMaterials(); | |
| } | |
| }; | |
| class BevelGizmoTest : public ::testing::Test { | |
| protected: | |
| void SetUp() override | |
| { | |
| Manager::kill(); | |
| QThread::msleep(50); | |
| ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr); | |
| ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; | |
| createStandardOgreMaterials(); | |
| } | |
| void TearDown() override | |
| { | |
| Manager::kill(); | |
| QThread::msleep(50); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/BevelGizmo_test.cpp` around lines 25 - 35, Add a TearDown() to the
BevelGizmoTest fixture that mirrors the cleanup performed in SetUp: call
Manager::kill() and QThread::msleep(50) to ensure Ogre is torn down after each
test (so objects like the "BevelGizmoUT_Cam" created in
AxisVisibilityScaleAndPick are not left live); implement TearDown() as an
override in the BevelGizmoTest class to perform the same teardown steps
currently in SetUp.
Missing TearDown() leaves the Ogre session live after the last fixture test.
SetUp() calls Manager::kill() at the beginning of each test to reset state left by the previous one, but there is no TearDown(). After the last test in the fixture completes (currently DistanceAlongAxisParallelRayReturnsZero), a full Ogre session — including the camera "BevelGizmoUT_Cam" allocated via sm->createCamera() at line 59 and never destroyed — remains live. Any test suite that runs later in the same binary will inherit this dirty state.
Add a symmetric TearDown():
🔧 Proposed fix
class BevelGizmoTest : public ::testing::Test {
protected:
void SetUp() override
{
Manager::kill();
QThread::msleep(50);
ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr);
ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)";
createStandardOgreMaterials();
}
+
+ void TearDown() override
+ {
+ Manager::kill();
+ QThread::msleep(50);
+ }
};📝 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.
Suggested change
| class BevelGizmoTest : public ::testing::Test { | |
| protected: | |
| void SetUp() override | |
| { | |
| Manager::kill(); | |
| QThread::msleep(50); | |
| ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr); | |
| ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; | |
| createStandardOgreMaterials(); | |
| } | |
| }; | |
| class BevelGizmoTest : public ::testing::Test { | |
| protected: | |
| void SetUp() override | |
| { | |
| Manager::kill(); | |
| QThread::msleep(50); | |
| ASSERT_NE(qobject_cast<QApplication*>(QCoreApplication::instance()), nullptr); | |
| ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; | |
| createStandardOgreMaterials(); | |
| } | |
| void TearDown() override | |
| { | |
| Manager::kill(); | |
| QThread::msleep(50); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/BevelGizmo_test.cpp` around lines 25 - 35, Add a TearDown() to the
BevelGizmoTest fixture that symmetrically cleans up the Ogre session created in
SetUp: call Manager::kill() (and any necessary waits like QThread::msleep(50))
and explicitly destroy or release objects created in tests (e.g., the camera
named "BevelGizmoUT_Cam" created via sm->createCamera()) so no Ogre resources
remain after the last test; implement TearDown() in the BevelGizmoTest class to
reverse SetUp() actions and ensure the Ogre scene manager/camera/resources are
properly removed before returning.
| EXPECT_TRUE(gizmo.isHandle(handle)); | ||
|
|
||
| gizmo.setHandleOffset(0.2f); | ||
| gizmo.setScale(0.0f); |
There was a problem hiding this comment.
setScale(0.0f) produces a degenerate (singular) transform matrix.
A zero scale creates a non-invertible matrix; Ogre scene node operations that require matrix inversion (e.g., world-to-local transforms, normal re-scaling) may log errors, trigger assertions, or produce NaNs — generating noise in CI output and potentially masking real failures. Use a very small positive value or an explicit near-zero constant to exercise the same boundary while keeping the matrix invertible:
🔧 Proposed fix
- gizmo.setScale(0.0f);
+ gizmo.setScale(1e-4f);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/BevelGizmo_test.cpp` at line 56, The test currently calls
gizmo.setScale(0.0f) which produces a singular (non-invertible) transform;
change the call to use a small positive near-zero value instead (e.g., replace
gizmo.setScale(0.0f) with gizmo.setScale(1e-6f) or a named constant like
NEAR_ZERO_SCALE) so the transform remains invertible; update any test-local
constant or add a small constexpr (e.g., NEAR_ZERO_SCALE or kEpsilon) and use
that with setScale to preserve the boundary condition without producing
degenerate matrices.
setScale(0.0f) produces a singular transform matrix that may trigger Ogre warnings or assertions.
A zero scale creates a non-invertible matrix. Ogre scene node operations that require world-to-local transform inversion can log errors, fire debug assertions, or produce NaN propagation — generating noise in CI output and potentially masking real failures in adjacent test steps. Use a very small positive value to test the same near-zero boundary while keeping the matrix invertible:
🔧 Proposed fix
- gizmo.setScale(0.0f);
+ gizmo.setScale(1e-4f);📝 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.
Suggested change
| gizmo.setScale(0.0f); | |
| gizmo.setScale(1e-4f); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/BevelGizmo_test.cpp` at line 56, The test currently calls
gizmo.setScale(0.0f) which produces a singular (non-invertible) transform;
change the call in BevelGizmo_test.cpp to use a small positive epsilon (e.g.
1e-6f or similar) instead of 0.0f so the transform remains invertible and avoids
Ogre warnings/assertions—update the invocation of setScale on the gizmo object
to use that tiny positive value.
Comment on lines
+1695
to
+1751
| TEST(ScanConfigLoadTest, LoadFromFile_ValidYaml) | ||
| { | ||
| QTemporaryFile file(QStringLiteral("%1/scan_ut_XXXXXX.yml").arg(QDir::tempPath())); | ||
| file.setAutoRemove(true); | ||
| ASSERT_TRUE(file.open()); | ||
| const char* yaml = | ||
| "version: 2\n" | ||
| "scan:\n" | ||
| " roots:\n" | ||
| " - ./assets\n" | ||
| "rules:\n" | ||
| " max_file_size_mb: 100.5\n" | ||
| " require_skeleton: true\n"; | ||
| file.write(yaml); | ||
| file.flush(); | ||
|
|
||
| ScanConfig c = ScanConfig::loadFromFile(file.fileName()); | ||
| EXPECT_EQ(c.version, 2); | ||
| ASSERT_EQ(c.roots.size(), 1); | ||
| EXPECT_EQ(c.roots[0], QStringLiteral("./assets")); | ||
| EXPECT_DOUBLE_EQ(c.maxFileSizeMb, 100.5); | ||
| EXPECT_TRUE(c.requireSkeleton); | ||
| } | ||
|
|
||
| TEST(ScanConfigLoadTest, LoadFromFile_ValidJson) | ||
| { | ||
| QTemporaryFile file(QStringLiteral("%1/scan_ut_XXXXXX.json").arg(QDir::tempPath())); | ||
| file.setAutoRemove(true); | ||
| ASSERT_TRUE(file.open()); | ||
| const char* json = R"json({ | ||
| "version": 3, | ||
| "scan": { "roots": ["a", "b"] }, | ||
| "report": { "format": "json", "fail_on": "warning" } | ||
| })json"; | ||
| file.write(json); | ||
| file.flush(); | ||
|
|
||
| ScanConfig c = ScanConfig::loadFromFile(file.fileName()); | ||
| EXPECT_EQ(c.version, 3); | ||
| ASSERT_EQ(c.roots.size(), 2); | ||
| EXPECT_EQ(c.roots[0], QStringLiteral("a")); | ||
| EXPECT_EQ(c.reportFormat, QStringLiteral("json")); | ||
| EXPECT_EQ(c.failOn, QStringLiteral("warning")); | ||
| } | ||
|
|
||
| TEST(ScanConfigLoadTest, LoadFromFile_InvalidJsonFallsBackToDefaults) | ||
| { | ||
| QTemporaryFile file(QStringLiteral("%1/scan_ut_XXXXXX.json").arg(QDir::tempPath())); | ||
| file.setAutoRemove(true); | ||
| ASSERT_TRUE(file.open()); | ||
| file.write("{ not valid json"); | ||
| file.flush(); | ||
|
|
||
| ScanConfig c = ScanConfig::loadFromFile(file.fileName()); | ||
| ScanConfig d = ScanConfig::defaults(); | ||
| EXPECT_EQ(c.version, d.version); | ||
| } |
There was a problem hiding this comment.
QTemporaryFile kept open before loadFromFile — confirmed Windows sharing violation risk
All three write-then-read tests (LoadFromFile_ValidYaml, LoadFromFile_ValidJson, LoadFromFile_InvalidJsonFallsBackToDefaults) write to a QTemporaryFile, flush it, but then call ScanConfig::loadFromFile(file.fileName()) while the file is still open. On Windows, Qt's QTemporaryFile internal handle can carry FILE_FLAG_DELETE_ON_CLOSE; a secondary QFile::open(ReadOnly) inside loadFromFile does not carry this flag, creating the mismatch that Qt's own documentation warns about: "all processes attempting to open the file must agree on using this flag or not using it. A mismatch will likely cause a sharing violation and failure to open the file."
When this sharing violation is silently swallowed by loadFromFile and it falls back to defaults, the positive assertions in ValidYaml and ValidJson pass vacuously rather than actually exercising the file-loading code path.
Qt's documentation confirms that "reopening a QTemporaryFile after calling close() is safe. For as long as the QTemporaryFile object itself is not destroyed, the unique temporary file will exist" — so adding file.close() immediately after file.flush() is the correct fix and does not affect auto-removal.
🛡️ Proposed fix (apply to all three tests identically)
file.write(yaml); // (or the json / invalid-json content)
file.flush();
+ file.close();
ScanConfig c = ScanConfig::loadFromFile(file.fileName());As per coding guidelines: "All code must compile and run on Windows, Linux (Ubuntu), and macOS."
📝 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.
Suggested change
| TEST(ScanConfigLoadTest, LoadFromFile_ValidYaml) | |
| { | |
| QTemporaryFile file(QStringLiteral("%1/scan_ut_XXXXXX.yml").arg(QDir::tempPath())); | |
| file.setAutoRemove(true); | |
| ASSERT_TRUE(file.open()); | |
| const char* yaml = | |
| "version: 2\n" | |
| "scan:\n" | |
| " roots:\n" | |
| " - ./assets\n" | |
| "rules:\n" | |
| " max_file_size_mb: 100.5\n" | |
| " require_skeleton: true\n"; | |
| file.write(yaml); | |
| file.flush(); | |
| ScanConfig c = ScanConfig::loadFromFile(file.fileName()); | |
| EXPECT_EQ(c.version, 2); | |
| ASSERT_EQ(c.roots.size(), 1); | |
| EXPECT_EQ(c.roots[0], QStringLiteral("./assets")); | |
| EXPECT_DOUBLE_EQ(c.maxFileSizeMb, 100.5); | |
| EXPECT_TRUE(c.requireSkeleton); | |
| } | |
| TEST(ScanConfigLoadTest, LoadFromFile_ValidJson) | |
| { | |
| QTemporaryFile file(QStringLiteral("%1/scan_ut_XXXXXX.json").arg(QDir::tempPath())); | |
| file.setAutoRemove(true); | |
| ASSERT_TRUE(file.open()); | |
| const char* json = R"json({ | |
| "version": 3, | |
| "scan": { "roots": ["a", "b"] }, | |
| "report": { "format": "json", "fail_on": "warning" } | |
| })json"; | |
| file.write(json); | |
| file.flush(); | |
| ScanConfig c = ScanConfig::loadFromFile(file.fileName()); | |
| EXPECT_EQ(c.version, 3); | |
| ASSERT_EQ(c.roots.size(), 2); | |
| EXPECT_EQ(c.roots[0], QStringLiteral("a")); | |
| EXPECT_EQ(c.reportFormat, QStringLiteral("json")); | |
| EXPECT_EQ(c.failOn, QStringLiteral("warning")); | |
| } | |
| TEST(ScanConfigLoadTest, LoadFromFile_InvalidJsonFallsBackToDefaults) | |
| { | |
| QTemporaryFile file(QStringLiteral("%1/scan_ut_XXXXXX.json").arg(QDir::tempPath())); | |
| file.setAutoRemove(true); | |
| ASSERT_TRUE(file.open()); | |
| file.write("{ not valid json"); | |
| file.flush(); | |
| ScanConfig c = ScanConfig::loadFromFile(file.fileName()); | |
| ScanConfig d = ScanConfig::defaults(); | |
| EXPECT_EQ(c.version, d.version); | |
| } | |
| TEST(ScanConfigLoadTest, LoadFromFile_ValidYaml) | |
| { | |
| QTemporaryFile file(QStringLiteral("%1/scan_ut_XXXXXX.yml").arg(QDir::tempPath())); | |
| file.setAutoRemove(true); | |
| ASSERT_TRUE(file.open()); | |
| const char* yaml = | |
| "version: 2\n" | |
| "scan:\n" | |
| " roots:\n" | |
| " - ./assets\n" | |
| "rules:\n" | |
| " max_file_size_mb: 100.5\n" | |
| " require_skeleton: true\n"; | |
| file.write(yaml); | |
| file.flush(); | |
| file.close(); | |
| ScanConfig c = ScanConfig::loadFromFile(file.fileName()); | |
| EXPECT_EQ(c.version, 2); | |
| ASSERT_EQ(c.roots.size(), 1); | |
| EXPECT_EQ(c.roots[0], QStringLiteral("./assets")); | |
| EXPECT_DOUBLE_EQ(c.maxFileSizeMb, 100.5); | |
| EXPECT_TRUE(c.requireSkeleton); | |
| } | |
| TEST(ScanConfigLoadTest, LoadFromFile_ValidJson) | |
| { | |
| QTemporaryFile file(QStringLiteral("%1/scan_ut_XXXXXX.json").arg(QDir::tempPath())); | |
| file.setAutoRemove(true); | |
| ASSERT_TRUE(file.open()); | |
| const char* json = R"json({ | |
| "version": 3, | |
| "scan": { "roots": ["a", "b"] }, | |
| "report": { "format": "json", "fail_on": "warning" } | |
| })json"; | |
| file.write(json); | |
| file.flush(); | |
| file.close(); | |
| ScanConfig c = ScanConfig::loadFromFile(file.fileName()); | |
| EXPECT_EQ(c.version, 3); | |
| ASSERT_EQ(c.roots.size(), 2); | |
| EXPECT_EQ(c.roots[0], QStringLiteral("a")); | |
| EXPECT_EQ(c.reportFormat, QStringLiteral("json")); | |
| EXPECT_EQ(c.failOn, QStringLiteral("warning")); | |
| } | |
| TEST(ScanConfigLoadTest, LoadFromFile_InvalidJsonFallsBackToDefaults) | |
| { | |
| QTemporaryFile file(QStringLiteral("%1/scan_ut_XXXXXX.json").arg(QDir::tempPath())); | |
| file.setAutoRemove(true); | |
| ASSERT_TRUE(file.open()); | |
| file.write("{ not valid json"); | |
| file.flush(); | |
| file.close(); | |
| ScanConfig c = ScanConfig::loadFromFile(file.fileName()); | |
| ScanConfig d = ScanConfig::defaults(); | |
| EXPECT_EQ(c.version, d.version); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ScanEngine_test.cpp` around lines 1695 - 1751, The tests write to a
QTemporaryFile and call ScanConfig::loadFromFile while the QTemporaryFile is
still open, which can cause Windows sharing violations; fix each test
(LoadFromFile_ValidYaml, LoadFromFile_ValidJson,
LoadFromFile_InvalidJsonFallsBackToDefaults) by calling file.close() immediately
after file.flush() and before calling ScanConfig::loadFromFile(file.fileName())
— keep setAutoRemove(true) as-is since closing the QTemporaryFile does not
remove the temporary file until the QTemporaryFile object is destroyed.
CI Linux failed to compile BevelGizmo_test (Camera has no setPosition/lookAt) and Euler_test (Quaternion has Dot, not dotProduct). Co-authored-by: Cursor <cursoragent@cursor.com>
|
fernandotonon
added a commit
that referenced
this pull request
May 2, 2026
…n, breadcrumbs) CodeRabbit Major: - positionForSample now clamps to clip length when t == clipLen instead of fmod-wrapping to 0. Previously, the closing keyframe of an equal-length bake captured the start pose, producing a visible pop on weight=0/1 bakes. fmod still applies for the bake-length > clip-length looping case. CodeRabbit Major: - New deactivateIfInvalid() helper called from setAnimA/setAnimB. If the user clears one side or makes A == B while preview is active, the blender now restores the snapshot and flips off — previously it was left with a stale enabled/weight configuration that kept playing until the user manually toggled Active off. CodeRabbit Minor: - Sentry breadcrumbs for blend preview activate/deactivate (matches CLAUDE.md guidance and the slice-B precedent for bake). Tests: - ClearingAnimAWhileActiveDeactivates / MakingAEqualBWhileActiveDeactivates. Rebased onto master (#359 / 0351755). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
May 2, 2026
…B) (#361) * feat(animation): two-way blend preview + bake-to-clip (Phase 5 slice B) Adds the slice B work from #260 / #360. - New AnimationBlender singleton (QML-registered as AnimationControl.AnimationBlender). Holds animA/animB names, weight (0..1), and mode (Mix / Additive / Override). Tracks the entity currently selected in the Animation Control panel. - Mix: weights (1-w, w), both states enabled, ANIMBLEND_AVERAGE. - Additive: same weights, skeleton blend mode ANIMBLEND_CUMULATIVE. - Override: single state enabled (B if w >= 0.5, else A). - MainWindow::frameRenderingQueued routes the active entity through blender->apply(); inactive entities follow the slice-A path (per-state speed scaling + selected-clip loop wrap). - bake() samples the blended pose at 30 fps (configurable), captures each bone's local TRS via Skeleton::_updateTransforms(), and writes a new Ogre::Animation with one node track per bone. Live state is saved + restored so the preview isn't disturbed by the bake. An existing clip with the same name is replaced. QML - New "Blend" section in PropertiesPanel.qml's Animations group: active checkbox, two animation pickers, weight slider, mode combo, bake-name field, Bake button. Visible only when the active entity has at least two animations. Tests - Pure-data fixture (10 cases, no Ogre): defaults, weight clamp, mode validation, signal emission, no-op safety. - Ogre fixture (Linux CI): refresh exposes both clips, bake produces the expected length + keyframe count, weight=0 ⇒ pure A, weight=1 ⇒ pure B, repeat-bake replaces the existing clip. Issue: #360 Plan: #260 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(build): add AnimationBlender to MaterialEditorQML test target sources The MaterialEditorQML_{test,qml_test,perf_test} executables maintain their own duplicated source list in tests/CMakeLists.txt. Slice B added AnimationBlender to src/CMakeLists.txt but not here, which caused undefined-reference link errors on the QML test targets in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(animation): address slice B review feedback CodeRabbit (Critical): - Refuse to bake when clipName matches animA or animB. The state pointers sa/sb resolve before removeAnimation() runs, so reusing the source name would invalidate them mid-bake. Codex P1 + CodeRabbit Major (preview state restore): - Snapshot every animation state's enabled+weight (and skeleton blend mode) when the blender activates and restore on deactivate / entity switch. Before, only A and B were touched, so any auxiliary layers enabled on the entity stayed off after toggling Active or Bake. CodeRabbit Major (slice-A loop region): - apply() now routes the active clip's time advance through AnimationControlController::advanceTime(), so the slice-A loop region still wraps the selected animation while blend preview is on. Non-active clip uses speed-scaled dt directly. - mainwindow.cpp passes raw dt to apply() (advanceTime applies speed itself); the lambda inside apply() recomputes scaledDt for non-A/B. CodeRabbit Major (QML visibility): - Drop AnimationControlController.hasAnimation from the blend panel's visible binding — that property is a "is a clip selected for KF edit", not "does the active entity have animations". Now gated only on AnimationBlender.animations.length >= 2. CodeRabbit Major (bake drops layers): - bake() now snapshots+restores every state in the set (not just A/B) so the live preview is fully preserved across a bake. SonarCloud cleanup: - Cast fps to float for the sample-count math (S5276). - Extract positionForSample() and writeAllBoneKeyframes() helpers to bring bake()'s cognitive complexity below the threshold (S3776). - Mark singleton new/delete with NOSONAR — pattern is shared across the project's controllers and changing it would be a separate refactor. Did NOT address (intentional): - CodeRabbit's GTEST_SKIP suggestion in AnimationBlender_test.cpp: the project's own convention (PR #355) is ASSERT_TRUE(tryInitOgre) to fail fast in CI. Switching to skip would mask CI regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(animation): reduce cognitive complexity + flatten nesting (Sonar) - Extract findEntityByName, configureBlend, muteOtherLayers, advanceState, captureAllStates, restoreAllStates, disableNonAB, createBoneTracks helpers in AnimationBlender.cpp. apply() drops from CC=31 → ~15, bake() from CC=35 → ~12 (S3776). - Extract advanceEntityStates() in mainwindow.cpp; restructure frameRenderingQueued with an early return so the inner loop is ≤ 3 levels deep (S134). - Const-correct refreshFromSelection's entity pointer (S5350) and use init-in-if for activeEntity (S6004). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(animation): blend UX polish + tests + sentry breadcrumb UX - Blend section is now a collapsible subgroup in the Animations panel, starts collapsed; header shows "(active)" hint when on. - Bake now deactivates the blender so the per-frame apply() stops re-imposing weights on top of the restored pre-bake state. - After bake, both AnimationControlController (Animation Control panel) and PropertiesPanelController (Inspector) refresh so the new clip appears in their lists without needing a re-select. - Activating the blender disables every per-animation Enable flag on the active entity (deactivation restores them via the snapshot). Inspector's per-anim Enable/Loop checkboxes show as 40 % opacity and ignore clicks while the blender is active for that entity, so the panel and the blender no longer fight over setEnabled() each frame. - New "Active" toggle in the Blend group uses the same 14×14 Rectangle + ✓ pattern as the per-anim Enable/Loop boxes (was a stock CheckBox). - New PropertiesPanelController.controlBgColor — a lightened Button shade — used as the unchecked background for all custom checkboxes (was "transparent", which disappeared on dark mode). Sentry - bake() emits a "ui.action" breadcrumb with clip name, mode, weight, fps, length, and sample count (per CLAUDE.md guidance). Tests - AnimationBlender_test pure-data: refuses bake on empty A/B, refuses bake over source clip names without an entity, default activeEntityName. - AnimationBlender_test Ogre fixture: bake refuses to overwrite source clip; activate disables every state; deactivate restores enabled flags via snapshot; bake auto-deactivates the blender; clipBaked signal emits with the new clip name; activeEntityName tracks the controller's selected entity. - PropertiesPanelController_test: controlBgColor matches button.lighter(115) and differs from panelColor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(animation): guard A/B and clean up Sonar findings CodeRabbit Major: - setActive() and apply()/bake() now refuse to operate when animA or animB is empty, or when they're equal. Previously, hitting Active with no clips picked would disable every state on the entity but apply() would bail out — leaving the rig frozen. Same problem for A == B (mix/additive would advance the same state twice per frame). SonarCloud: - S134 (critical): extract disableAllStates() helper from setActive() so the inner loop is no longer 4 levels deep. - S3358 (major): replace nested ternary in the bake breadcrumb with a small modeName() switch helper. - S5817 (major): apply() mutates skeleton+state pointers indirectly, so it can't be const. Mark with NOSONAR + rationale. Tests: - ActivateRefusedWhen{AnimAEmpty, AnimBEmpty, AEqualsB}: setActive(true) is rejected and active() stays false. - BakeRefusedWhenAEqualsB: bake returns empty when both clips match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: bump version to 2.34.0 Slice B (animation blend preview + bake-to-clip) is a feature addition since 2.33.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): set A/B before ActiveTogglesEmitSignal expects activation The new A/B guard in setActive() rejects activation when animA or animB is empty. The pre-existing ActiveTogglesEmitSignal test didn't set them, so setActive(true) was a silent no-op and the signal never fired. Set A/B in the test before toggling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(animation): address slice B review (end-pose, mid-run invalidation, breadcrumbs) CodeRabbit Major: - positionForSample now clamps to clip length when t == clipLen instead of fmod-wrapping to 0. Previously, the closing keyframe of an equal-length bake captured the start pose, producing a visible pop on weight=0/1 bakes. fmod still applies for the bake-length > clip-length looping case. CodeRabbit Major: - New deactivateIfInvalid() helper called from setAnimA/setAnimB. If the user clears one side or makes A == B while preview is active, the blender now restores the snapshot and flips off — previously it was left with a stale enabled/weight configuration that kept playing until the user manually toggled Active off. CodeRabbit Minor: - Sentry breadcrumbs for blend preview activate/deactivate (matches CLAUDE.md guidance and the slice-B precedent for bake). Tests: - ClearingAnimAWhileActiveDeactivates / MakingAEqualBWhileActiveDeactivates. Rebased onto master (#359 / 0351755). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: NOSONAR S1448 on AnimationBlender (Q_PROPERTY boilerplate) Sonar counts Q_PROPERTY getters/setters/signals + QML singleton boilerplate (instance/qmlInstance/kill) as separate methods, putting the class at 36 vs the 35 threshold. The class is cohesive — live preview and bake share the same selection/snapshot state — so splitting it would just fragment the wiring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds unit tests to raise Sonar coverage for BevelGizmo, Euler, ScanConfig::loadFromFile, and SubMeshTransform edge cases. Follow-up commit fixes Linux CI compile errors against Ogre 14 (camera must live on a
SceneNode; quaternions useDot).Technical details
Features (tests)
SceneManagersafety; axis, visibility, handle offset, scale,updateScreenSpaceScale,isHandle,distanceAlongAxis.direction, relative yaw/pitch/roll,normalise.loadFromFilefor missing path, valid YAML/JSON, invalid JSON fallback.Bugfixes
SceneNode, set clip distances and aspect,lookAton the node (Ogre 14CameraAPI).dotProductwithQuaternion::Dot.