test: stabilize CLI mesh import and headless Ogre tests - #355
Merged
Conversation
- TestHelpers: treat CLIHidden like TestHidden for GL context checks - MeshImporterExporter: absolute paths for resource groups; evict cached mesh before reload so disk updates are visible - CLIPipeline tests: export generated meshes to unique temp dirs; fix LOD auto test paths; add removeCliExportTree cleanup - EditableMesh_test: vertex color brush falloff regression test - AnimationMerger_test: skip mesh tests without hardware buffers - EditorViewport_test: safer teardown; processEvents before widget delete - MainWindow: conditional singleton teardown when Manager already gone Made-with: Cursor
|
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 (3)
📝 WalkthroughWalkthroughTests and helpers now fail-fast on missing Ogre/GL (replacing many GTEST_SKIP() paths with ASSERT_*), MeshImporterExporter forcibly normalizes/unloads and reloads mesh resources, TestHelpers gains CLI/headless handling and robot mesh path lookup, CLIPipeline export uses UUID temp dirs with recursive cleanup, and MainWindow teardown guards Manager pointer. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 8 minutes and 35 seconds.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc373b0eee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1182
to
+1183
| if (auto existing = Ogre::MeshManager::getSingleton().getByName(meshResName, meshGroup)) | ||
| Ogre::MeshManager::getSingleton().remove(existing); |
There was a problem hiding this comment.
Removing an existing mesh resource unconditionally before reloading (MeshManager::remove(existing)) can invalidate live scene objects when the same .mesh is imported again while older entities still reference it. In Ogre, removing a referenced resource can leave dependent objects with stale internal pointers, which can surface as crashes or undefined rendering in the “re-import same file/path” workflow this change targets. Gate removal to unreferenced meshes (or use an explicit reload path) instead of always deleting by name/group.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/CLIPipeline_test.cpp (1)
794-801: ⚡ Quick winAdd a safety guard before recursive directory deletion.
removeCliExportTreecurrently removes whatever parent directory is derived frommeshFilePath. A lightweight guard ensures only test-ownedqtmesh_cli_exp_*trees are removed.Proposed hardening
static void removeCliExportTree(const QString& meshFilePath) { if (meshFilePath.isEmpty()) return; QFileInfo fi(meshFilePath); - QDir(fi.absolutePath()).removeRecursively(); + const QDir dir(fi.absolutePath()); + if (!dir.dirName().startsWith("qtmesh_cli_exp_")) + return; + dir.removeRecursively(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 794 - 801, The function removeCliExportTree currently removes the parent directory of meshFilePath unconditionally; add a safety guard so it only removes directories matching the test-owned prefix (e.g., "qtmesh_cli_exp_"). In removeCliExportTree, after creating QFileInfo fi and before calling QDir(fi.absolutePath()).removeRecursively(), check fi.fileName() or fi.absolutePath() path components to verify the directory name startsWith "qtmesh_cli_exp_" (or use a stricter regex), and if the check fails simply return (or log a warning) to avoid deleting unrelated directories.
🤖 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/EditorViewport_test.cpp`:
- Around line 70-72: The test teardown must not call mainWindow->close() because
MainWindow::closeEvent invokes platform-specific _exit(0) on macOS; replace the
call to mainWindow->close() with a safe teardown that does not trigger
closeEvent, e.g. call mainWindow->hide() and schedule destruction via
mainWindow->deleteLater() (or delete the object directly) and set mainWindow to
nullptr so the window is removed without invoking MainWindow::closeEvent.
In `@src/MeshImporterExporter.cpp`:
- Around line 1181-1185: Add Sentry breadcrumbs around the
cache-evict-and-reload sequence so the forced cache-bust is observable: before
calling Ogre::MeshManager::getSingleton().remove(existing) and again
before/after Ogre::MeshManager::getSingleton().load(meshResName, meshGroup) call
SentryReporter::addBreadcrumb("file.import", "<describe: evicting cached mesh: "
+ meshResName + ">") and SentryReporter::addBreadcrumb("file.import",
"<describe: loading mesh: " + meshResName + ">") respectively, using meshResName
and meshGroup to make the messages unique and informative.
---
Nitpick comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 794-801: The function removeCliExportTree currently removes the
parent directory of meshFilePath unconditionally; add a safety guard so it only
removes directories matching the test-owned prefix (e.g., "qtmesh_cli_exp_"). In
removeCliExportTree, after creating QFileInfo fi and before calling
QDir(fi.absolutePath()).removeRecursively(), check fi.fileName() or
fi.absolutePath() path components to verify the directory name startsWith
"qtmesh_cli_exp_" (or use a stricter regex), and if the check fails simply
return (or log a warning) to avoid deleting unrelated directories.
🪄 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: 3eb9f6a0-7e94-4fd6-9c7d-aa81021f8816
📒 Files selected for processing (7)
src/AnimationMerger_test.cppsrc/CLIPipeline_test.cppsrc/EditableMesh_test.cppsrc/EditorViewport_test.cppsrc/MeshImporterExporter.cppsrc/TestHelpers.hsrc/mainwindow.cpp
Comment on lines
+70
to
72
| if (mainWindow) { | ||
| mainWindow->close(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify EditorViewport teardown calls close():"
rg -n -C2 'TearDownTestSuite|mainWindow->close\(' src/EditorViewport_test.cpp
echo
echo "Verify MainWindow::closeEvent side effects:"
rg -n -C3 'void MainWindow::closeEvent|QApplication::quit\(|_exit\(0\)' src/mainwindow.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1214
Avoid mainWindow->close() in teardown; it terminates the test process on macOS via _exit(0).
Line 71 calls close(), which triggers MainWindow::closeEvent. In src/mainwindow.cpp (line 1734), that path calls _exit(0) on macOS, aborting the test suite before completion.
🔧 Proposed safe change
- if (mainWindow) {
- mainWindow->close();
- }
+ if (mainWindow) {
+ // Avoid MainWindow::closeEvent side effects in tests (quit/_exit on macOS).
+ mainWindow->hide();
+ }Per coding guidelines, all code must compile and run on Windows, Linux, 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
| if (mainWindow) { | |
| mainWindow->close(); | |
| } | |
| if (mainWindow) { | |
| // Avoid MainWindow::closeEvent side effects in tests (quit/_exit on macOS). | |
| mainWindow->hide(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditorViewport_test.cpp` around lines 70 - 72, The test teardown must not
call mainWindow->close() because MainWindow::closeEvent invokes
platform-specific _exit(0) on macOS; replace the call to mainWindow->close()
with a safe teardown that does not trigger closeEvent, e.g. call
mainWindow->hide() and schedule destruction via mainWindow->deleteLater() (or
delete the object directly) and set mainWindow to nullptr so the window is
removed without invoking MainWindow::closeEvent.
Comment on lines
+1181
to
+1185
| // Drop any cached mesh so a replaced file on disk is re-read (tests/CLI reuse paths). | ||
| if (auto existing = Ogre::MeshManager::getSingleton().getByName(meshResName, meshGroup)) | ||
| Ogre::MeshManager::getSingleton().remove(existing); | ||
| Ogre::MeshPtr mesh = Ogre::MeshManager::getSingleton().load( | ||
| file.fileName().toStdString().data(), | ||
| file.path().toStdString().data()); | ||
| meshResName, meshGroup); |
There was a problem hiding this comment.
Add import breadcrumbs around cache eviction and mesh reload.
Line 1182/1184 introduces a significant I/O path change (forced cache-bust + reload) but doesn’t emit file.import breadcrumbs, which makes this behavior hard to trace in Sentry during regressions.
🧭 Suggested instrumentation
const Ogre::String meshResName = file.fileName().toStdString();
const Ogre::String meshGroup = file.absolutePath().toStdString();
// Drop any cached mesh so a replaced file on disk is re-read (tests/CLI reuse paths).
- if (auto existing = Ogre::MeshManager::getSingleton().getByName(meshResName, meshGroup))
+ if (auto existing = Ogre::MeshManager::getSingleton().getByName(meshResName, meshGroup)) {
+ SentryReporter::addBreadcrumb("file.import",
+ QStringLiteral("Evict cached mesh before reload: %1").arg(file.filePath()));
Ogre::MeshManager::getSingleton().remove(existing);
+ }
+ SentryReporter::addBreadcrumb("file.import",
+ QStringLiteral("Load mesh from disk: %1").arg(file.filePath()));
Ogre::MeshPtr mesh = Ogre::MeshManager::getSingleton().load(
meshResName, meshGroup);As per coding guidelines, **/*.cpp: “All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). … use 'file.import'/'file.export' for I/O operations.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 1181 - 1185, Add Sentry
breadcrumbs around the cache-evict-and-reload sequence so the forced cache-bust
is observable: before calling Ogre::MeshManager::getSingleton().remove(existing)
and again before/after Ogre::MeshManager::getSingleton().load(meshResName,
meshGroup) call SentryReporter::addBreadcrumb("file.import", "<describe:
evicting cached mesh: " + meshResName + ">") and
SentryReporter::addBreadcrumb("file.import", "<describe: loading mesh: " +
meshResName + ">") respectively, using meshResName and meshGroup to make the
messages unique and informative.
- Use C++17 if init for haveLocation (cpp:S6004) - Catch Ogre::Exception and std::exception; log instead of bare catch (S2738/S2486) Made-with: Cursor
Made-with: Cursor
Owner
Author
|
Sonar / CI follow-up: Pushed two small commits on CodeRabbit: The “docstring coverage” item is a CodeRabbit org check tuned for Python-style docstrings; this C++ PR does not add public APIs that need Doxygen blocks. No code change required unless you want to raise that threshold in CodeRabbit settings. |
…e/GL - Replace headless/GL skips with ASSERT_* / ASSERT_NO_THROW across suites - TestHelpers: tryInitOgre/createTestRenderWindow and canLoadMeshFiles fixes - test_main: exit non-zero if Ogre init fails before RUN_ALL_TESTS - RTShaderHelper_test: resolve media via QTMESH_UT_SOURCE_ROOT (CMake) - MaterialEditorQML: unload LLM model before no-model error-path test - MainWindow/SpaceCamera/OgreWidget: FAIL instead of skip on init failure Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/MCPServer_test.cpp (1)
201-215: 🏗️ Heavy liftSplit the non-Ogre tool tests out of this fixture.
MCPServerTest::SetUp()now hard-fails on Ogre initialization, so any Xvfb/EGL regression prevents unrelated MCP coverage in this fixture (list_files,search_files,read_file, simple argument-validation paths, etc.) from running at all. Moving the Ogre assertion into a GL-only subfixture would keep the new fail-fast behavior for rendering tests without masking regressions in the pure JSON/filesystem tool paths.As per coding guidelines "Linux: Tests must work under Xvfb (headless X11) — avoid assumptions about a real display."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer_test.cpp` around lines 201 - 215, Currently MCPServerTest::SetUp() calls ASSERT_TRUE(tryInitOgre()) and createStandardOgreMaterials(), which hard-fails unrelated non-GL tests; remove the Ogre initialization from MCPServerTest::SetUp() so the fixture only constructs server (server = std::make_unique<MCPServer>()) and leaves GL-related setup out; create a new derived fixture (e.g., MCPServerGLTest) whose SetUp() calls tryInitOgre() and createStandardOgreMaterials() before constructing server and uses ASSERT_TRUE on tryInitOgre() so only GL tests fail fast; update GL-dependent tests to use MCPServerGLTest while leaving list_files/search_files/read_file and other pure JSON/filesystem tests on the original MCPServerTest.src/SkeletonTransform_test.cpp (1)
571-571: ⚡ Quick winDon’t require disk-mesh loading for the no-file-I/O skeleton fixtures.
Both of these paths build their mesh/skeleton state entirely in memory, so
canLoadMeshFiles()is stricter than the behavior under test and can fail before anySkeletonTransformlogic runs. A lighter Ogre/entity readiness check would keep these tests useful in headless runs.As per coding guidelines,
src/**/*_test.cpp: “Linux: Tests must work under Xvfb (headless X11) — avoid assumptions about a real display.”Also applies to: 642-643
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SkeletonTransform_test.cpp` at line 571, The test currently asserts ASSERT_TRUE(canLoadMeshFiles()) which unnecessarily requires disk/GL mesh loading for in-memory skeleton fixtures; update the two occurrences (around ASSERT_TRUE(canLoadMeshFiles()) at line ~571 and the similar check at ~642-643) to either remove the canLoadMeshFiles() assertion or replace it with a lightweight headless Ogre readiness check (e.g., assert that the Ogre subsystem/root or a headless-initialization helper is available) so SkeletonTransform tests that build meshes/skeletons in memory can run under Xvfb; ensure you update both spots referencing canLoadMeshFiles().src/MeshImporterExporter_test.cpp (1)
500-500: ⚡ Quick winUse a narrower readiness check for these in-memory exporter tests.
These cases never load meshes from disk; they only build in-memory geometry and call
sceneExporter(). Gating them oncanLoadMeshFiles()broadens the failure surface to unrelated mesh-import setup and drops valid headless coverage.As per coding guidelines,
src/**/*_test.cpp: “Linux: Tests must work under Xvfb (headless X11) — avoid assumptions about a real display.”Also applies to: 524-524
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` at line 500, Replace the broad display/mesh-import readiness check canLoadMeshFiles() used in the test (ASSERT_TRUE(canLoadMeshFiles())) with a narrow in-memory-exporter readiness check because these tests only build geometry and call sceneExporter(); add or use a helper like canRunInMemoryExporterTests() (or similar) and assert that instead, or remove the assertion entirely if no runtime dependency exists; update the assertion in the test to ASSERT_TRUE(canRunInMemoryExporterTests()) (referencing canLoadMeshFiles() and sceneExporter() to locate the test) and implement the helper to only verify minimal headless X11/CI requirements rather than full mesh-file loading.src/mainwindow_test.cpp (1)
66-72: ⚡ Quick winDeduplicate repeated
MainWindowconstruction try/catch blocks.The same exception-to-
FAIL()pattern appears three times; a small fixture helper would reduce repetition and keep error text uniform.♻️ Suggested refactor
class MainWindowTest : public ::testing::Test { protected: @@ + void constructWindowOrFail(const char* context) + { + try { + window = new MainWindow(); + } catch (const std::exception& e) { + FAIL() << context << ": " << e.what(); + } catch (...) { + FAIL() << context << ": unknown exception"; + } + ASSERT_NE(window, nullptr); + } + void SetUp() override { @@ - try { - window = new MainWindow(); - } catch (const std::exception& e) { - FAIL() << "MainWindow construction failed: " << e.what(); - } catch (...) { - FAIL() << "MainWindow construction failed with unknown exception"; - } - - ASSERT_NE(window, nullptr); + constructWindowOrFail("MainWindow construction failed"); } @@ - try { - window = new MainWindow(); - } catch (const std::exception& e) { - FAIL() << "MainWindow reconstruction failed: " << e.what(); - } catch (...) { - FAIL() << "MainWindow reconstruction failed with unknown exception"; - } + constructWindowOrFail("MainWindow reconstruction failed");Also applies to: 527-533, 951-957
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow_test.cpp` around lines 66 - 72, Create a small fixture/helper function (e.g., makeMainWindowOrFail or constructMainWindowSafely) that encapsulates the try/catch pattern used around new MainWindow(): it should attempt to new MainWindow, catch std::exception and non-standard exceptions and call FAIL() with the uniform message including e.what() when available, then return the constructed MainWindow* (or abort via GTest FAIL). Replace each duplicated try/catch block around MainWindow construction (the three occurrences and the other similar blocks) with a single call to this helper to remove repetition and keep error text consistent.src/AnimationWidget_test.cpp (1)
537-1123: ⚡ Quick winExtract repeated
canLoadMeshFiles()precondition into a fixture helper.There are many identical guard lines; centralizing them will keep messages consistent and simplify future edits.
♻️ Suggested refactor
class AnimationWidgetTest : public ::testing::Test { protected: QApplication* app = nullptr; + void requireMeshLoadingSupport() { + ASSERT_TRUE(canLoadMeshFiles()) << "mesh loading requires GL (Xvfb in CI)"; + } void SetUp() override { Manager::kill(); QThread::msleep(50); @@ TEST_F(AnimationWidgetTest, TriangleMeshEntityShowsNoAnimations) { - ASSERT_TRUE(canLoadMeshFiles()) << "mesh loading requires GL (Xvfb in CI)"; + requireMeshLoadingSupport(); @@ TEST_F(AnimationWidgetTest, AnimatedEntityShowsAnimationRow) { - ASSERT_TRUE(canLoadMeshFiles()) << "mesh loading requires GL (Xvfb in CI)"; + requireMeshLoadingSupport();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationWidget_test.cpp` around lines 537 - 1123, Extract the repeated ASSERT_TRUE(canLoadMeshFiles()) guard into a single fixture helper: add a protected helper method (e.g. ensureCanLoadMeshFiles() or requireGL()) on the AnimationWidgetTest fixture (and any other test fixtures seen: AnimationWidgetToggleBoneWeightsTest, AnimationWidgetSkeletonTableBoneWeightsClickTest, AnimationWidgetSceneNodeDestroyedCleanupTest, AnimationWidgetSceneClearingCleanupTest) that calls ASSERT_TRUE(canLoadMeshFiles()) << "mesh loading requires GL (Xvfb in CI)"; then replace each direct ASSERT_TRUE(canLoadMeshFiles()) invocation in the tests (e.g. in tests like AnimTableClicked_EnableColumn, SkeletonMeshEntityWithoutAnimations, PollAnimationStateUpdatesCheckbox, MultipleWidgetsSyncOnSelectionChange, SkeletonTableWeightsColumnDisabledForNoSkeleton, etc.) with a call to the new helper to centralize the check and message.
🤖 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/BoneWeightOverlay_test.cpp`:
- Around line 38-39: The test currently passes a relative path
("./media/models/robot.mesh") into MeshImporterExporter::importer(uris) making
the fixture dependent on the current working directory; change the test to
resolve an absolute/application-relative test-data path (using the same
test-data lookup helper used elsewhere in the test-suite) and assign that
resolved path to the uris variable before calling
MeshImporterExporter::importer(uris), then keep the assertions on
Manager::getSingleton()->getEntities().isEmpty() as-is so the import is
validated reliably regardless of working directory.
In `@src/EditModeController_test.cpp`:
- Around line 295-296: Before performing the hard precondition checks, reset any
leftover Ogre state by clearing/destroying the Manager singleton and removing
any existing render-windows so prior tests cannot leak GL state into these
checks; i.e., add a call to clear/destroy the Manager (e.g., invoke
Manager::getSingletonPtr()->clear() or the project's
Manager::destroy/destroySingleton equivalent) and ensure all render windows are
closed just before the ASSERT_TRUE(tryInitOgre()) and
ASSERT_TRUE(canLoadMeshFiles()) lines, and also add `#include` <QThread> with the
other Qt headers.
In `@src/MaterialEditorQML_test.cpp`:
- Around line 1955-1963: The test races with LLMManager's delayed auto-load
(tryAutoLoadModel) because unloadModel() is queued; update the test around
LLMManager::getSingleton().unloadModel() to either disable auto-load for the
duration of the test (call whatever flag/method the manager exposes to turn off
auto-loading) or wait deterministically for the manager's unload completion
signal rather than polling llmModelLoaded(); specifically, locate calls to
LLMManager::getSingleton().unloadModel(), replace the polling loop that checks
editor->llmModelLoaded() with a connection/wait on the manager's
unload-completed signal (or set the manager's auto-load disabled before calling
unloadModel()) so the final ASSERT_FALSE(editor->llmModelLoaded()) is stable.
In `@src/MaterialPresetLibrary_test.cpp`:
- Around line 97-98: The test currently gates the "no-selection" early-return
path on tryInitOgre() and calls createStandardOgreMaterials(), which makes it
fail when GL/X11 isn't available; modify the test in
MaterialPresetLibrary_test.cpp to exercise the no-selection branch without
initializing Ogre or calling createStandardOgreMaterials() (remove or
conditionalize the tryInitOgre() ASSERT_TRUE and the
createStandardOgreMaterials() call for this specific test), ensuring the code
path under test runs in a pure CPU/headless environment—if necessary, use
dependency injection or a lightweight mock in place of Ogre initialization so
the test asserts the early-return behavior deterministically.
In `@src/MeshImporterExporter.cpp`:
- Around line 1177-1184: The mesh reload evicts the cached mesh but not the
sidecar's script-declared materials, so update the import routine to purge or
reload materials for the same resource group before calling
Ogre::MeshManager::getSingleton().load: after tryLoadSidecarMaterialScript(file)
and before loading the mesh, iterate the materials declared in meshGroup (via
Ogre::MaterialManager::getSingleton()) and remove or unload them (or
clear/recreate script declarations for meshGroup) so that
tryLoadSidecarMaterialScript() will reparse and register fresh materials for
meshGroup rather than skipping due to stale declarations.
In `@src/test_main.cpp`:
- Around line 81-87: The global Ogre preflight fail-fast block in test_main.cpp
should be skipped when GoogleTest is re-executing death-test subprocesses;
modify the code around the tryInitOgre() call so it first checks argv for the
internal flag "--gtest_internal_run_death_test" (or uses GoogleTest's internal
flag detection) and only calls tryInitOgre() when that flag is NOT present, thus
avoiding the createTestRenderWindow() failure in death-test child processes;
update the conditional that currently runs tryInitOgre() so it gates on absence
of "--gtest_internal_run_death_test" before logging the fatal message and
returning 1.
---
Nitpick comments:
In `@src/AnimationWidget_test.cpp`:
- Around line 537-1123: Extract the repeated ASSERT_TRUE(canLoadMeshFiles())
guard into a single fixture helper: add a protected helper method (e.g.
ensureCanLoadMeshFiles() or requireGL()) on the AnimationWidgetTest fixture (and
any other test fixtures seen: AnimationWidgetToggleBoneWeightsTest,
AnimationWidgetSkeletonTableBoneWeightsClickTest,
AnimationWidgetSceneNodeDestroyedCleanupTest,
AnimationWidgetSceneClearingCleanupTest) that calls
ASSERT_TRUE(canLoadMeshFiles()) << "mesh loading requires GL (Xvfb in CI)"; then
replace each direct ASSERT_TRUE(canLoadMeshFiles()) invocation in the tests
(e.g. in tests like AnimTableClicked_EnableColumn,
SkeletonMeshEntityWithoutAnimations, PollAnimationStateUpdatesCheckbox,
MultipleWidgetsSyncOnSelectionChange,
SkeletonTableWeightsColumnDisabledForNoSkeleton, etc.) with a call to the new
helper to centralize the check and message.
In `@src/mainwindow_test.cpp`:
- Around line 66-72: Create a small fixture/helper function (e.g.,
makeMainWindowOrFail or constructMainWindowSafely) that encapsulates the
try/catch pattern used around new MainWindow(): it should attempt to new
MainWindow, catch std::exception and non-standard exceptions and call FAIL()
with the uniform message including e.what() when available, then return the
constructed MainWindow* (or abort via GTest FAIL). Replace each duplicated
try/catch block around MainWindow construction (the three occurrences and the
other similar blocks) with a single call to this helper to remove repetition and
keep error text consistent.
In `@src/MCPServer_test.cpp`:
- Around line 201-215: Currently MCPServerTest::SetUp() calls
ASSERT_TRUE(tryInitOgre()) and createStandardOgreMaterials(), which hard-fails
unrelated non-GL tests; remove the Ogre initialization from
MCPServerTest::SetUp() so the fixture only constructs server (server =
std::make_unique<MCPServer>()) and leaves GL-related setup out; create a new
derived fixture (e.g., MCPServerGLTest) whose SetUp() calls tryInitOgre() and
createStandardOgreMaterials() before constructing server and uses ASSERT_TRUE on
tryInitOgre() so only GL tests fail fast; update GL-dependent tests to use
MCPServerGLTest while leaving list_files/search_files/read_file and other pure
JSON/filesystem tests on the original MCPServerTest.
In `@src/MeshImporterExporter_test.cpp`:
- Line 500: Replace the broad display/mesh-import readiness check
canLoadMeshFiles() used in the test (ASSERT_TRUE(canLoadMeshFiles())) with a
narrow in-memory-exporter readiness check because these tests only build
geometry and call sceneExporter(); add or use a helper like
canRunInMemoryExporterTests() (or similar) and assert that instead, or remove
the assertion entirely if no runtime dependency exists; update the assertion in
the test to ASSERT_TRUE(canRunInMemoryExporterTests()) (referencing
canLoadMeshFiles() and sceneExporter() to locate the test) and implement the
helper to only verify minimal headless X11/CI requirements rather than full
mesh-file loading.
In `@src/SkeletonTransform_test.cpp`:
- Line 571: The test currently asserts ASSERT_TRUE(canLoadMeshFiles()) which
unnecessarily requires disk/GL mesh loading for in-memory skeleton fixtures;
update the two occurrences (around ASSERT_TRUE(canLoadMeshFiles()) at line ~571
and the similar check at ~642-643) to either remove the canLoadMeshFiles()
assertion or replace it with a lightweight headless Ogre readiness check (e.g.,
assert that the Ogre subsystem/root or a headless-initialization helper is
available) so SkeletonTransform tests that build meshes/skeletons in memory can
run under Xvfb; ensure you update both spots referencing canLoadMeshFiles().
🪄 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: b79ecfe5-462e-4fa1-985a-d6ab5cbab30f
📒 Files selected for processing (49)
src/AnimationControlController_test.cppsrc/AnimationMerger_test.cppsrc/AnimationWidget_test.cppsrc/BoneWeightOverlay_test.cppsrc/CLIPipeline_test.cppsrc/CMakeLists.txtsrc/EditModeController_test.cppsrc/EditableMesh_test.cppsrc/FBX/FBXExporter_test.cppsrc/GizmoAxisHelpers_test.cppsrc/HalfEdgeMesh_test.cppsrc/MCPServer_test.cppsrc/Manager_test.cppsrc/MaterialComboDelegate_test.cppsrc/MaterialEditorQML_test.cppsrc/MaterialPresetLibrary_test.cppsrc/MaterialPreviewRenderer_test.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cppsrc/MeshInfoOverlay_test.cppsrc/MeshLodController_test.cppsrc/MeshTransform_test.cppsrc/MeshValidator_test.cppsrc/NodeGrouping_test.cppsrc/NormalVisualizer_test.cppsrc/ObjectItemModel_test.cppsrc/OgreWidget_test.cppsrc/PrimitiveObject_test.cppsrc/PrimitivesWidget_test.cppsrc/PropertiesPanelController_test.cppsrc/RTShaderHelper_test.cppsrc/RotationGizmo_test.cppsrc/ScaleGizmo_test.cppsrc/ScanEngine_test.cppsrc/SceneTreeModel_test.cppsrc/SelectionBoxObject_test.cppsrc/SelectionSet_test.cppsrc/SkeletonDebug_test.cppsrc/SkeletonTransform_test.cppsrc/SpaceCamera_test.cppsrc/SubEntityHighlight_test.cppsrc/TestHelpers.hsrc/TransformOperator_test.cppsrc/TranslationGizmo_test.cppsrc/ViewportGrid_test.cppsrc/commands/TransformCommands_test.cppsrc/mainwindow_test.cppsrc/material_test.cppsrc/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/EditableMesh_test.cpp
- src/TestHelpers.h
Comment on lines
1177
to
+1184
| tryLoadSidecarMaterialScript(file); | ||
| const Ogre::String meshResName = file.fileName().toStdString(); | ||
| const Ogre::String meshGroup = file.absolutePath().toStdString(); | ||
| // Drop any cached mesh so a replaced file on disk is re-read (tests/CLI reuse paths). | ||
| if (auto existing = Ogre::MeshManager::getSingleton().getByName(meshResName, meshGroup)) | ||
| Ogre::MeshManager::getSingleton().remove(existing); | ||
| Ogre::MeshPtr mesh = Ogre::MeshManager::getSingleton().load( | ||
| file.fileName().toStdString().data(), | ||
| file.path().toStdString().data()); | ||
| meshResName, meshGroup); |
There was a problem hiding this comment.
Reload the sidecar material cache when forcing a .mesh refresh.
This now evicts the cached mesh, but not the .material resources loaded from the same group. On a second import of the same path after the sidecar changed, tryLoadSidecarMaterialScript() can see the old material as already declared and skip reparsing, so you end up with fresh geometry and stale material definitions.
Please evict or recreate the script-declared materials for meshGroup before loading the mesh.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 1177 - 1184, The mesh reload
evicts the cached mesh but not the sidecar's script-declared materials, so
update the import routine to purge or reload materials for the same resource
group before calling Ogre::MeshManager::getSingleton().load: after
tryLoadSidecarMaterialScript(file) and before loading the mesh, iterate the
materials declared in meshGroup (via Ogre::MaterialManager::getSingleton()) and
remove or unload them (or clear/recreate script declarations for meshGroup) so
that tryLoadSidecarMaterialScript() will reparse and register fresh materials
for meshGroup rather than skipping due to stale declarations.
Comment on lines
+81
to
+87
| // All GPU-backed tests require a headless GL context (Xvfb on Linux CI). | ||
| // Fail fast instead of silently skipping hundreds of tests. | ||
| if (!tryInitOgre()) { | ||
| fprintf(stderr, | ||
| "UnitTests FATAL: tryInitOgre() failed — need working DISPLAY / Xvfb for GL.\n"); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -type f -name "*.cpp" -o -name "*.h" | grep -E "(test_main|CLIPipeline_test|TestHelpers)" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 126
🏁 Script executed:
cd /repo && wc -l src/test_main.cpp src/CLIPipeline_test.cpp src/TestHelpers.h 2>/dev/nullRepository: fernandotonon/QtMeshEditor
Length of output: 126
🏁 Script executed:
cd /repo && cat -n src/test_main.cpp | head -100Repository: fernandotonon/QtMeshEditor
Length of output: 126
🏁 Script executed:
cd /repo && sed -n '60,100p' src/test_main.cppRepository: fernandotonon/QtMeshEditor
Length of output: 126
🏁 Script executed:
cd /repo && cat -n src/TestHelpers.h | head -80Repository: fernandotonon/QtMeshEditor
Length of output: 126
🏁 Script executed:
cd /repo && rg "CLIPipelineRun" -A 10 -B 2 src/CLIPipeline_test.cpp | head -100Repository: fernandotonon/QtMeshEditor
Length of output: 126
🏁 Script executed:
cd /repo && rg "QT_QPA_PLATFORM" src/CLIPipeline_test.cpp -B 3 -A 3Repository: fernandotonon/QtMeshEditor
Length of output: 126
🏁 Script executed:
cd /repo && rg "EXPECT_EXIT\|EXPECT_DEATH\|threadsafe" src/CLIPipeline_test.cpp -B 2 -A 5Repository: fernandotonon/QtMeshEditor
Length of output: 126
🏁 Script executed:
cd /repo && git log --oneline -n 1 -- src/test_main.cppRepository: fernandotonon/QtMeshEditor
Length of output: 126
🏁 Script executed:
git ls-files | grep -E "(test_main|CLIPipeline_test|TestHelpers)" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 131
🏁 Script executed:
wc -l src/test_main.cpp src/CLIPipeline_test.cpp src/TestHelpers.h 2>/dev/nullRepository: fernandotonon/QtMeshEditor
Length of output: 165
🏁 Script executed:
cat -n src/test_main.cpp | sed -n '1,110p'Repository: fernandotonon/QtMeshEditor
Length of output: 4052
🏁 Script executed:
cat -n src/TestHelpers.hRepository: fernandotonon/QtMeshEditor
Length of output: 24970
🏁 Script executed:
rg "CLIPipelineRun" -A 15 -B 2 src/CLIPipeline_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2481
🏁 Script executed:
rg "QT_QPA_PLATFORM\|EXPECT_EXIT\|EXPECT_DEATH" src/CLIPipeline_test.cpp -B 3 -A 5Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
rg "gtest_internal_run_death_test" src/ 2>/dev/null | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
Skip the global Ogre preflight in death-test subprocesses.
GoogleTest threadsafe death tests re-exec the test binary with the internal flag --gtest_internal_run_death_test before the test body runs. The CLIPipelineRun death tests run under QT_QPA_PLATFORM=offscreen, which causes createTestRenderWindow() to fail when tryInitOgre() attempts to create a hidden widget and obtain a valid window handle. This causes the reëxec'd process to return 1 before CLIPipeline::run() executes, so the death test fails expecting the intended exit code.
Gate the fail-fast block by detecting the internal flag.
Suggested fix
+ bool isDeathTestChild = false;
+ for (int i = 1; i < argc; ++i) {
+ if (QByteArray(argv[i]).startsWith("--gtest_internal_run_death_test")) {
+ isDeathTestChild = true;
+ break;
+ }
+ }
+
// All GPU-backed tests require a headless GL context (Xvfb on Linux CI).
// Fail fast instead of silently skipping hundreds of tests.
- if (!tryInitOgre()) {
+ if (!isDeathTestChild && !tryInitOgre()) {
fprintf(stderr,
"UnitTests FATAL: tryInitOgre() failed — need working DISPLAY / Xvfb for GL.\n");
return 1;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test_main.cpp` around lines 81 - 87, The global Ogre preflight fail-fast
block in test_main.cpp should be skipped when GoogleTest is re-executing
death-test subprocesses; modify the code around the tryInitOgre() call so it
first checks argv for the internal flag "--gtest_internal_run_death_test" (or
uses GoogleTest's internal flag detection) and only calls tryInitOgre() when
that flag is NOT present, thus avoiding the createTestRenderWindow() failure in
death-test child processes; update the conditional that currently runs
tryInitOgre() so it gates on absence of "--gtest_internal_run_death_test" before
logging the fatal message and returning 1.
Made-with: Cursor
- Add testRobotMeshPath() in TestHelpers; use in BoneWeightOverlay and SkeletonTransform - MaterialPresetLibrary: ApplyPresetWithoutSelection needs no Ogre (early return) - EditModeController bevel E2E: Manager::kill + settle before Ogre init - MaterialEditorQML: disable LLM auto-load before unload to avoid race Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/SkeletonTransform_test.cpp`:
- Around line 33-36: The test assumes
Manager::getSingleton()->getEntities().last() is the imported skeletal Entity
which is order-dependent and unsafe; update the setup to iterate
Manager::getSingleton()->getEntities(), check each object's getMovableType() ==
"Entity" before casting to Ogre::Entity*, then select the one where
hasSkeleton() is true (or ASSERT/FAIL if none found) instead of using .last();
use the symbols getEntities(), getMovableType(), Ogre::Entity, and hasSkeleton()
to locate and implement this filter and safe cast.
In `@src/TestHelpers.h`:
- Around line 134-137: The current try-catch around the compound expression
root->getRenderTarget("TestHidden") || root->getRenderTarget("CLIHidden") lets
an exception from the first getRenderTarget call skip the second check; change
the logic to call getRenderTarget for "TestHidden" and "CLIHidden" in separate
try blocks (or check each in its own try/catch) so an exception when querying
"TestHidden" doesn't prevent checking "CLIHidden"; update both occurrences (the
block around the earlier return and the similar code at the later line ~213) to
individually catch exceptions from root->getRenderTarget while returning true if
either lookup succeeds.
🪄 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: 416333a7-85ce-47ec-b183-a56af5da811e
📒 Files selected for processing (6)
src/BoneWeightOverlay_test.cppsrc/EditModeController_test.cppsrc/MaterialEditorQML_test.cppsrc/MaterialPresetLibrary_test.cppsrc/SkeletonTransform_test.cppsrc/TestHelpers.h
✅ Files skipped from review due to trivial changes (1)
- src/MaterialEditorQML_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/BoneWeightOverlay_test.cpp
Comment on lines
+33
to
+36
| ASSERT_FALSE(Manager::getSingleton()->getEntities().isEmpty()); | ||
| entity = Manager::getSingleton()->getEntities().last(); | ||
| ASSERT_NE(entity, nullptr); | ||
| ASSERT_TRUE(entity->hasSkeleton()); |
There was a problem hiding this comment.
Don't assume .last() is the imported skeletal entity.
This setup is order-dependent, and Manager::getEntities() is documented as returning all attached objects. Filter for a movable of type "Entity" and then pick the one with hasSkeleton() instead of binding the fixture to the container's last entry.
As per coding guidelines, "Manager::getEntities() returns all attached objects — check obj->getMovableType() == 'Entity' before casting to Ogre::Entity* (ManualObjects will crash otherwise)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SkeletonTransform_test.cpp` around lines 33 - 36, The test assumes
Manager::getSingleton()->getEntities().last() is the imported skeletal Entity
which is order-dependent and unsafe; update the setup to iterate
Manager::getSingleton()->getEntities(), check each object's getMovableType() ==
"Entity" before casting to Ogre::Entity*, then select the one where
hasSkeleton() is true (or ASSERT/FAIL if none found) instead of using .last();
use the symbols getEntities(), getMovableType(), Ogre::Entity, and hasSkeleton()
to locate and implement this filter and safe cast.
Comment on lines
+134
to
137
| // Already have a render window (tests use TestHidden; CLIPipeline::initOgreHeadless uses CLIHidden) | ||
| try { | ||
| if (root->getRenderTarget("TestHidden")) | ||
| if (root->getRenderTarget("TestHidden") || root->getRenderTarget("CLIHidden")) | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code at the mentioned lines
cat -n src/TestHelpers.h | sed -n '130,155p'Repository: fernandotonon/QtMeshEditor
Length of output: 1289
🏁 Script executed:
# Also check the second location
cat -n src/TestHelpers.h | sed -n '205,230p'Repository: fernandotonon/QtMeshEditor
Length of output: 1087
🏁 Script executed:
# Search for other getRenderTarget usage in the codebase to understand patterns
rg "getRenderTarget" --context 3Repository: fernandotonon/QtMeshEditor
Length of output: 2904
Fix short-circuit evaluation preventing CLIHidden detection when TestHidden throws.
Ogre::Root::getRenderTarget() throws when a render target is not found in some Ogre versions. With the current A || B form wrapped in a single try-catch, if "TestHidden" throws, the exception exits the try block entirely, and the "CLIHidden" lookup never runs. This breaks detection of CLI-initialized headless windows, breaking headless/Xvfb test support.
Suggested fix
+ auto hasRenderTarget = [&](const char* name) {
+ try {
+ return root->getRenderTarget(name) != nullptr;
+ } catch (const Ogre::Exception&) {
+ return false;
+ }
+ };
+
// Already have a render window (tests use TestHidden; CLIPipeline::initOgreHeadless uses CLIHidden)
- try {
- if (root->getRenderTarget("TestHidden") || root->getRenderTarget("CLIHidden"))
- return true;
- } catch (...) {
- // getRenderTarget throws if not found in some Ogre versions
- }
+ if (hasRenderTarget("TestHidden") || hasRenderTarget("CLIHidden"))
+ return true;Also applies to: line 213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TestHelpers.h` around lines 134 - 137, The current try-catch around the
compound expression root->getRenderTarget("TestHidden") ||
root->getRenderTarget("CLIHidden") lets an exception from the first
getRenderTarget call skip the second check; change the logic to call
getRenderTarget for "TestHidden" and "CLIHidden" in separate try blocks (or
check each in its own try/catch) so an exception when querying "TestHidden"
doesn't prevent checking "CLIHidden"; update both occurrences (the block around
the earlier return and the similar code at the later line ~213) to individually
catch exceptions from root->getRenderTarget while returning true if either
lookup succeeds.
Assimp processor suites instantiate their own Ogre::Root; leaving Manager alive after the headless GL probe broke those tests and EXPECT_EXIT cases. Made-with: Cursor
threadsafe forks inherit main's QApplication; CLIPipeline::run then creates a second QApplication and exits 1 instead of _exit(2). Made-with: Cursor
EditModeController connects to SelectionSet in its constructor. Manager::kill() destroys SelectionSet while the singleton controller still holds stale connections; recycle the controller first so tryInitOgre() rebuilds a clean graph and avoids SIGSEGV in CI. Made-with: Cursor
Re-initing Manager every SetUp crashed the second test (SIGSEGV on Linux CI). Keep one Ogre instance for the suite, clear SelectionSet before node teardown, and EditModeController::kill() in TearDown so BevelGizmo is not reused stale. Made-with: Cursor
|
fernandotonon
added a commit
that referenced
this pull request
May 2, 2026
- mainwindow.cpp (Sonar S5350): animCtrl is now const auto*. - mainwindow.cpp (Sonar S5827 ×2): use auto for static_cast results where the type is already on the RHS. - AnimationControlController.cpp (Sonar S2681 ×2 / CodeRabbit nitpick): brace single-line ifs in setLoopStart / setLoopEnd, and clamp before the qFuzzyCompare bail-out so we don't emit loopRegionChanged when the request collapses to the existing value. - AnimationControlPanel.qml (CodeRabbit major): drop the hard-coded pad=13 / avail=width-26 in the timeline canvas and loop-handles layer; bind to timeSlider.leftPadding and timeSlider.availableWidth so the loop shading and drag handles track the slider groove across Qt styles, DPI settings, and platforms. - PropertiesPanel.qml (CodeRabbit minor): speed combobox now picks the nearest preset rather than silently showing 1× when the controller's value isn't an exact match. - AnimationControlController_test.cpp: align with master PR #355's new pattern — ASSERT_TRUE(canLoadMeshFiles()) instead of GTEST_SKIP() (CI now requires headless GL to work). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
May 2, 2026
… A) (#356) * feat(animation): playback speed, loop region, auto-key (Phase 5 slice A) Adds the timeline polish from #260: - playbackSpeed (0.25–4×) global multiplier; ComboBox sits next to the Play button in the Inspector's Animations section. In-app only — scales dt before setTimePosition; keyframes/length untouched. - loopStart/loopEnd/loopRegionActive scoped to the entity+animation selected in the Animation Control panel. Other entities advance at native timing. Wraps via fmod so large overshoots fold back inside the region. Resets to [0, length] on each new selection. - autoKey toggle: end-of-drag in TransformOperator pushes a keyframe on the active bone via AnimationControlController::addKeyframe(). - QML: timeline canvas shades the loop region and renders blue in/out markers; two transparent drag handles let you reposition them. Tests: 14 new pure-data tests in a separate fixture (no Ogre needed) covering speed scaling, signal emission, loop wrap (basic, large overshoot, degenerate, inactive passthrough, clamping), and auto-key safety. 2 Ogre-fixture tests for the auto-key add-keyframe path and loop-region reset on selection (run on Linux CI). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(animation): preserve loop-handle bindings during drag Codex flagged that drag.target: parent on the loop-marker rectangles would mutate their x property directly, breaking the declarative bind to loopStart/loopEnd. After the first drag, controller-driven updates (new clip selected, length changed, external value change) could leave the handles visually out of sync. Drop drag.target and compute the new time from mouseX inside the MouseArea instead. The handle's x binding stays intact; the controller remains the single source of truth. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(animation): drop auto-key toggle from slice A Auto-key only makes sense once bones are directly manipulable in the viewport (click a bone → drag the gizmo). The original wiring fired on scene-node end-of-drag and sampled an interpolated pose, which is not what the toggle's name implies. Without a bone-gizmo there is no honest behavior to ship. Splitting it out into #358 (bone manipulation gizmo + auto-keyframe). Slice A keeps speed scaling + per-entity loop region, which work on their own. Removes: - autoKey property/setter/signal + autoKeyOnTransform() from controller - "Auto Key" toggle from AnimationControlPanel.qml - TransformOperator end-of-drag hook (and its include) - 5 auto-key tests (4 pure-data + 1 Ogre-fixture) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(animation): address SonarCloud + CodeRabbit review - mainwindow.cpp (Sonar S5350): animCtrl is now const auto*. - mainwindow.cpp (Sonar S5827 ×2): use auto for static_cast results where the type is already on the RHS. - AnimationControlController.cpp (Sonar S2681 ×2 / CodeRabbit nitpick): brace single-line ifs in setLoopStart / setLoopEnd, and clamp before the qFuzzyCompare bail-out so we don't emit loopRegionChanged when the request collapses to the existing value. - AnimationControlPanel.qml (CodeRabbit major): drop the hard-coded pad=13 / avail=width-26 in the timeline canvas and loop-handles layer; bind to timeSlider.leftPadding and timeSlider.availableWidth so the loop shading and drag handles track the slider groove across Qt styles, DPI settings, and platforms. - PropertiesPanel.qml (CodeRabbit minor): speed combobox now picks the nearest preset rather than silently showing 1× when the controller's value isn't an exact match. - AnimationControlController_test.cpp: align with master PR #355's new pattern — ASSERT_TRUE(canLoadMeshFiles()) instead of GTEST_SKIP() (CI now requires headless GL to work). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
May 2, 2026
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>
fernandotonon
added a commit
that referenced
this pull request
May 2, 2026
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>
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
Improves reliability of the unit test suite and CLI-related mesh loading paths.
Changes
canLoadMeshFiles/createTestRenderWindowrecognizeCLIHidden(fromCLIPipeline::initOgreHeadless) as well asTestHidden, avoiding false "no GL" state after CLI tests.QFileInfo; remove a cached mesh in the same (name, group) beforeMeshManager::loadso updated files on disk are not masked..meshfiles under unique/tmp/qtmesh_cli_exp_<uuid>/directories; fix LOD auto-mode test to look for outputs next to the source; centralize cleanup withremoveCliExportTree.kill()whenManageris already gone (test ordering).Verification
xvfb-run -a ./bin/UnitTests --gtest_shuffleMade with Cursor
Summary by CodeRabbit
Bug Fixes
Tests