fix(scan): stabilize scan --fix + better output + FSAA default - #325
Conversation
|
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 (13)
📝 WalkthroughWalkthroughThis PR introduces skeleton-only FBX export functionality, expands the scan engine with redundant keyframe detection and auto-fixing, enhances the CLI animation pipeline with analysis and export capabilities, and hardens import/material processing with robustness improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Pipeline
participant FB as FBXExporter
participant FBD as FBXDocumentBuilder
participant FBX as FBX File
CLI->>FB: exportSkeletonOnlyFBX(skeleton, path)
FB->>FBD: buildSkeletonOnly(skeleton)
FBD->>FBD: Create bone models & bind pose
FBD->>FBD: Optimize animation curves<br/>(per-channel time arrays)
FBD->>FBD: Skip geometry/materials/textures
FBD->>FB: Return optimized document
FB->>FBX: Write FBX with skeleton+animations
FBX-->>CLI: Success
sequenceDiagram
participant CLI as CLI/Scan
participant SE as ScanEngine
participant AM as AnimationMerger
participant FBD as FBXDocumentBuilder
participant FBX as FBX Export
CLI->>SE: applyFixes(config, scanRoot, asset, findings)
SE->>SE: Detect redundant keyframes
alt Redundant Keys Found & Fixable
SE->>AM: simplifyAnimation(animation)
AM->>AM: Remove consecutive duplicates
AM->>AM: Skip tracks with zero keys
SE->>FBD: buildSkeletonOnly + simplify
FBD->>FBX: Re-export smaller FBX
SE->>SE: Record bytesSaved, keysRemoved
SE-->>CLI: finding.skipped=false, marked FIXED
else Dry-Run or Size Growth
SE-->>CLI: finding.skipped=true
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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. 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: f349021ac2
ℹ️ 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".
| return 0; | ||
| } | ||
|
|
||
| if (analyzeMode) { |
There was a problem hiding this comment.
This new early analyzeMode return bypasses the existing if (analyzeMode || simplifyMode) block that computes redundant keyframes and emits the detailed analysis JSON/text. As a result, qtmesh anim <file> --analyze now only prints skeleton metadata, and options like --preset, --tolerance, --rotation-tolerance-deg, and --animation are effectively ignored despite still being documented in usage, which is a behavioral regression for CLI users and automation that depend on the previous analysis output.
Useful? React with 👍 / 👎.
| if (!skel && !animOnlySkeletons.isEmpty()) | ||
| skel = animOnlySkeletons.first(); |
There was a problem hiding this comment.
These lines allow cmdAnim to proceed with a valid skeleton even when no mesh entity was imported (animation-only input), but the simplify branch later still unconditionally dereferences entity (refreshAvailableAnimationState() / getParentSceneNode()). That means qtmesh anim <anim-only-file> --simplify can now crash with a null-pointer dereference; simplify needs the same isAnimOnlyInput export handling already added for resample/decimate/rename.
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline.cpp (1)
1829-1837:⚠️ Potential issue | 🔴 CriticalNull pointer dereference when
--simplifyis used on animation-only files.If the input file is animation-only (no mesh geometry),
entitywill benullptrat line 1448, but the simplify code path at lines 1829-1837 unconditionally dereferences it. This will cause a crash.The resample and decimate modes correctly guard against this with
if (isAnimOnlyInput)checks (lines 1602, 1663), but simplify mode lacks this protection.🐛 Proposed fix to handle animation-only inputs in simplify mode
// simplifyMode int totalRemoved = 0; int animsProcessed = 0; for (const auto& name : animNames) { if (!animationFilter.isEmpty() && animationFilter.toStdString() != name) continue; int removed = AnimationMerger::simplifyAnimation(skel.get(), name, tol); totalRemoved += removed; ++animsProcessed; } - entity->refreshAvailableAnimationState(); - QFileInfo outFi(outputPath); - auto* node = entity->getParentSceneNode(); - int result = MeshImporterExporter::exporter(node, outFi.absoluteFilePath(), formatForExtension(outputPath)); - if (result != 0) { - SentryReporter::captureMessage(QString("CLI anim: simplify export failed (.%1)").arg(outFi.suffix()), "error"); - err() << "Error: Export failed." << Qt::endl; - return 1; + if (isAnimOnlyInput) { + QString exportErr; + if (!exportAnimOnly(skel, outFi.absoluteFilePath(), &exportErr)) { + SentryReporter::captureMessage(QString("CLI anim: simplify export failed (anim-only)"), "error"); + err() << "Error: Export failed: " << exportErr << Qt::endl; + return 1; + } + } else { + entity->refreshAvailableAnimationState(); + auto* node = entity->getParentSceneNode(); + int result = MeshImporterExporter::exporter(node, outFi.absoluteFilePath(), formatForExtension(outputPath)); + if (result != 0) { + SentryReporter::captureMessage(QString("CLI anim: simplify export failed (.%1)").arg(outFi.suffix()), "error"); + err() << "Error: Export failed." << Qt::endl; + return 1; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 1829 - 1837, The simplify branch unconditionally dereferences entity (calling entity->getParentSceneNode() and refreshAvailableAnimationState()) which crashes for animation-only inputs; add the same guard used in resample/decimate: check isAnimOnlyInput before touching entity — if isAnimOnlyInput, avoid dereferencing entity (either call MeshImporterExporter::exporter with a nullptr node if supported or route to the existing animation-only export path), otherwise proceed to call entity->refreshAvailableAnimationState() and node = entity->getParentSceneNode(); ensure MeshImporterExporter::exporter is invoked with the appropriate node value and keep the existing error reporting (SentryReporter::captureMessage / err()) on non-zero result.
🧹 Nitpick comments (4)
src/Assimp/MaterialProcessor.cpp (1)
59-67: Add Sentry breadcrumbs for these material import branches.These changed paths perform significant import-time operations (normal-map load/apply attempts and skip/early-return conditions) but only write Ogre logs. Please add
SentryReporter::addBreadcrumb(...)events for observability consistency.As per coding guidelines "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'file.import'/'file.export' for I/O operations."
Also applies to: 73-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Assimp/MaterialProcessor.cpp` around lines 59 - 67, Add Sentry breadcrumbs for the normal-map load/apply and skip branches by calling SentryReporter::addBreadcrumb("file.import", ...) at each significant point: when the normal map fails to load (the branch that logs "Failed to load normal map" using normalFilename and materialName), when a normalTexPtr exists and you are about to apply it (before the log "Applying RTSS normal map" using normalFilename and materialName), and also mirror this for the later branch around lines 73-76; place these calls near the existing Ogre::LogManager::getSingleton().logMessage invocations, include concise messages referencing the materialName and normalFilename, and do not change behavior of ensureFirstPass(existingMaterial) or applyRTSSNormalMap(existingMaterial, normalTexPtr->getName()).src/FBX/FBXExporter.cpp (1)
2119-2145: Add afile.exportbreadcrumb for the new skeleton-only path.This is a new user-visible export flow, so failures here will be much harder to correlate without a breadcrumb.
Suggested change
bool FBXExporter::exportSkeletonOnlyFBX(const Ogre::Skeleton* skeleton, const QString& filePath) { if (!skeleton || filePath.isEmpty()) return false; + + SentryReporter::addBreadcrumb( + "file.export", + QString("Export skeleton-only FBX: %1").arg(filePath)); std::ofstream out(filePath.toStdString(), std::ios::binary);As per coding guidelines "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/FBX/FBXExporter.cpp` around lines 2119 - 2145, The exportSkeletonOnlyFBX function lacks a Sentry breadcrumb; add SentryReporter::addBreadcrumb("file.export", ...) calls to record this new user-visible export flow: one breadcrumb at function start (include the filePath and tag it as "skeleton-only" and maybe "start") and another on failure before each early return/error log (e.g., when !skeleton, filePath.isEmpty(), out not open, and when builder.buildSkeletonOnly fails) so failures can be correlated; use the function name exportSkeletonOnlyFBX and the filePath in the breadcrumb messages to make them unique and actionable.src/FBX/FBXExporter_test.cpp (1)
2303-2342: Tighten this skeleton-only test a bit more.Right now it only proves that bones/animations exist. It won't catch regressions that accidentally start exporting mesh-only objects again, and the fixture never freezes an explicit bind pose before export.
Suggested test hardening
auto* child = skel->createBone("Child", 1); child->setPosition(0, 1, 0); root->addChild(child); + skel->setBindingPose(); auto* anim = skel->createAnimation("wave", 1.0f); auto* track = anim->createNodeTrack(0, child); @@ auto animStacks = objects->findAll("AnimationStack"); auto animCurves = objects->findAll("AnimationCurve"); EXPECT_GE(animStacks.size(), 1); EXPECT_GE(animCurves.size(), 1); + EXPECT_TRUE(objects->findAll("Geometry").empty()); + EXPECT_TRUE(objects->findAll("Material").empty()); + EXPECT_TRUE(objects->findAll("Deformer").empty());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FBX/FBXExporter_test.cpp` around lines 2303 - 2342, The test currently only checks that bones and animations exist after calling FBXExporter::exportSkeletonOnlyFBX(skel.get(), outPath) and will not catch regressions that export mesh data or fail to capture the skeleton bind pose; before calling exportSkeletonOnlyFBX, freeze the skeleton's bind pose on the skel instance (use the appropriate Ogre Skeleton API to save/apply the binding pose for the bones created with createBone and the animation created with createAnimation) and after parseFBX(outPath.toStdString()) assert that the parsed Objects block (found via findTopLevel) contains no "Model" or "Geometry"/mesh-related entries and that only bone/node and animation objects exist (tighten EXPECT_GE checks to exact counts or explicit absence assertions) so the test fails if mesh data is exported or the bind pose was not preserved.src/CLIPipeline.cpp (1)
1316-1316: Duplicate--analyzeflag parsing.The
--analyzeflag is parsed twice: once at line 1316 and again at line 1344. This redundancy won't cause incorrect behavior but adds confusion to the code.♻️ Proposed fix to remove duplicate
if (arg == "--list") { listMode = true; continue; } - if (arg == "--analyze") { analyzeMode = true; continue; } + if (arg == "--analyze") { analyzeMode = true; continue; } if (arg == "--json") { jsonOutput = true; continue; } ... if (arg == "--simplify") { simplifyMode = true; continue; } - if (arg == "--analyze") { analyzeMode = true; continue; } if (arg == "--preset" && i + 1 < argc) {Remove the second occurrence at line 1344.
Also applies to: 1344-1344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` at line 1316, The command-line parsing contains a duplicate branch that checks if (arg == "--analyze") { analyzeMode = true; continue; } — keep the first occurrence (the one at the top of the argument-parsing block) and remove the second duplicate to avoid redundancy; search for the repeated check in the argument parsing loop (look for the variable arg and analyzeMode in CLIPipeline.cpp) and delete the later if-block so analyzeMode is only set once.
🤖 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/Assimp/Importer.cpp`:
- Around line 88-103: The retry path currently ORs in additionalFlags
(lightFlags |= additionalFlags) which can re-enable the post-process bits that
caused the initial failure; change this to only merge a curated safe subset:
define a SAFE_FALLBACK_FLAGS mask containing only known-safe aiProcess_* bits
(e.g., aiProcess_Triangulate, aiProcess_ValidateDataStructure,
aiProcess_LimitBoneWeights, aiProcess_PopulateArmatureData,
aiProcess_GlobalScale and optionally aiProcess_ConvertToLeftHanded) and replace
the OR with lightFlags |= (additionalFlags & SAFE_FALLBACK_FLAGS) so
importer.ReadFile(path, lightFlags) uses only the permitted fallback flags
(referencing lightFlags, additionalFlags, SAFE_FALLBACK_FLAGS,
convertToLeftHanded, and importer.ReadFile).
In `@src/Assimp/MaterialProcessor.cpp`:
- Around line 64-67: ensure the code checks the result of
ensureFirstPass(existingMaterial) before calling applyRTSSNormalMap: capture the
return value from ensureFirstPass (or the pass pointer it returns) and only call
applyRTSSNormalMap(existingMaterial, normalTexPtr->getName()) when
ensureFirstPass indicates success; if ensureFirstPass fails, skip the RTSS
normal-map application (and optionally log or handle the failure) to avoid
dereferencing a missing pass.
In `@src/FBX/FBXExporter.cpp`:
- Around line 477-487: buildSkeletonOnly currently casts away const and calls
reset() on the caller's Ogre::Skeleton, mutating external state; instead
preserve the const contract by not modifying the input: set m_skeletonOnly and
m_hasSkeleton as before, but do not const_cast the incoming pointer — either
store it as a const pointer (e.g. keep a const Ogre::Skeleton* m_skeletonConst)
or make an internal copy/clone of the skeleton and call reset() on that copy,
then assign m_skeleton to the internal copy; update references to m_skeleton
accordingly (or add/use m_skeletonConst) so export logic uses the
non-mutating/internal copy while leaving the caller's skeleton unchanged.
In `@src/OgreWidget_test.cpp`:
- Around line 251-260: This test modifies global QSettings by removing
ViewportSettingsKeys::fsaaSamples() without restoring it; update
TEST_F(OgreWidgetTest, FsaaDefaultsToZeroWhenUnset) to save the original
presence and value of that key (using QSettings::contains and value), perform
the remove and assertions (including widget->rebuildRenderWindow()), then
restore the original state at the end of the test (re-set the key if it existed
or remove it if it did not) so QSettings state is isolated and order-independent
for other tests.
In `@src/ScanConfig.h`:
- Around line 54-57: The comment for redundantKeyframesPctThreshold in
ScanConfig.h is out of sync with the default value; update the comment to
accurately state that the check is enabled by default at 40% (or change the
default to 0 to keep it disabled). Locate the redundantKeyframesPctThreshold
declaration and either edit the comment to read that redundant-keyframe warnings
are enabled by default and trigger at >=40%, or set
redundantKeyframesPctThreshold to 0.0 if you prefer to keep the check disabled
by default; ensure the comment and the variable default stay consistent.
---
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 1829-1837: The simplify branch unconditionally dereferences entity
(calling entity->getParentSceneNode() and refreshAvailableAnimationState())
which crashes for animation-only inputs; add the same guard used in
resample/decimate: check isAnimOnlyInput before touching entity — if
isAnimOnlyInput, avoid dereferencing entity (either call
MeshImporterExporter::exporter with a nullptr node if supported or route to the
existing animation-only export path), otherwise proceed to call
entity->refreshAvailableAnimationState() and node =
entity->getParentSceneNode(); ensure MeshImporterExporter::exporter is invoked
with the appropriate node value and keep the existing error reporting
(SentryReporter::captureMessage / err()) on non-zero result.
---
Nitpick comments:
In `@src/Assimp/MaterialProcessor.cpp`:
- Around line 59-67: Add Sentry breadcrumbs for the normal-map load/apply and
skip branches by calling SentryReporter::addBreadcrumb("file.import", ...) at
each significant point: when the normal map fails to load (the branch that logs
"Failed to load normal map" using normalFilename and materialName), when a
normalTexPtr exists and you are about to apply it (before the log "Applying RTSS
normal map" using normalFilename and materialName), and also mirror this for the
later branch around lines 73-76; place these calls near the existing
Ogre::LogManager::getSingleton().logMessage invocations, include concise
messages referencing the materialName and normalFilename, and do not change
behavior of ensureFirstPass(existingMaterial) or
applyRTSSNormalMap(existingMaterial, normalTexPtr->getName()).
In `@src/CLIPipeline.cpp`:
- Line 1316: The command-line parsing contains a duplicate branch that checks if
(arg == "--analyze") { analyzeMode = true; continue; } — keep the first
occurrence (the one at the top of the argument-parsing block) and remove the
second duplicate to avoid redundancy; search for the repeated check in the
argument parsing loop (look for the variable arg and analyzeMode in
CLIPipeline.cpp) and delete the later if-block so analyzeMode is only set once.
In `@src/FBX/FBXExporter_test.cpp`:
- Around line 2303-2342: The test currently only checks that bones and
animations exist after calling FBXExporter::exportSkeletonOnlyFBX(skel.get(),
outPath) and will not catch regressions that export mesh data or fail to capture
the skeleton bind pose; before calling exportSkeletonOnlyFBX, freeze the
skeleton's bind pose on the skel instance (use the appropriate Ogre Skeleton API
to save/apply the binding pose for the bones created with createBone and the
animation created with createAnimation) and after
parseFBX(outPath.toStdString()) assert that the parsed Objects block (found via
findTopLevel) contains no "Model" or "Geometry"/mesh-related entries and that
only bone/node and animation objects exist (tighten EXPECT_GE checks to exact
counts or explicit absence assertions) so the test fails if mesh data is
exported or the bind pose was not preserved.
In `@src/FBX/FBXExporter.cpp`:
- Around line 2119-2145: The exportSkeletonOnlyFBX function lacks a Sentry
breadcrumb; add SentryReporter::addBreadcrumb("file.export", ...) calls to
record this new user-visible export flow: one breadcrumb at function start
(include the filePath and tag it as "skeleton-only" and maybe "start") and
another on failure before each early return/error log (e.g., when !skeleton,
filePath.isEmpty(), out not open, and when builder.buildSkeletonOnly fails) so
failures can be correlated; use the function name exportSkeletonOnlyFBX and the
filePath in the breadcrumb messages to make them unique and actionable.
🪄 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: a3ab770b-7dad-46fc-a398-9cb1096b1b57
📒 Files selected for processing (13)
src/AnimationMerger.cppsrc/Assimp/Importer.cppsrc/Assimp/MaterialProcessor.cppsrc/CLIPipeline.cppsrc/FBX/FBXExporter.cppsrc/FBX/FBXExporter.hsrc/FBX/FBXExporter_test.cppsrc/OgreWidget.cppsrc/OgreWidget_test.cppsrc/ScanConfig.hsrc/ScanEngine.cppsrc/ScanEngine.hsrc/ScanEngine_test.cpp
| if ((!scene || !scene->mRootNode) && | ||
| (pathEndsWithInsensitive(path, ".fbx") || pathEndsWithInsensitive(path, ".fbxa"))) { | ||
| unsigned int lightFlags = aiProcess_Triangulate | | ||
| aiProcess_ValidateDataStructure | | ||
| aiProcess_LimitBoneWeights | | ||
| aiProcess_PopulateArmatureData | | ||
| aiProcess_GlobalScale; | ||
| if (convertToLeftHanded) | ||
| lightFlags |= aiProcess_ConvertToLeftHanded; | ||
| lightFlags |= additionalFlags; | ||
| importer.SetPropertyBool(AI_CONFIG_IMPORT_FBX_PRESERVE_PIVOTS, false); | ||
| scene = importer.ReadFile(path, lightFlags); | ||
| m_sceneUpAxis = 1; | ||
| if (scene && scene->mMetaData) | ||
| scene->mMetaData->Get("UpAxis", m_sceneUpAxis); | ||
| } |
There was a problem hiding this comment.
Keep the retry path genuinely “light.”
lightFlags |= additionalFlags can immediately re-enable the same post-process bits that caused the first import to fail, so the fallback stops being a real fallback as soon as a caller supplies extra Assimp flags. This should carry through only a known-safe subset, not the whole caller mask.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Assimp/Importer.cpp` around lines 88 - 103, The retry path currently ORs
in additionalFlags (lightFlags |= additionalFlags) which can re-enable the
post-process bits that caused the initial failure; change this to only merge a
curated safe subset: define a SAFE_FALLBACK_FLAGS mask containing only
known-safe aiProcess_* bits (e.g., aiProcess_Triangulate,
aiProcess_ValidateDataStructure, aiProcess_LimitBoneWeights,
aiProcess_PopulateArmatureData, aiProcess_GlobalScale and optionally
aiProcess_ConvertToLeftHanded) and replace the OR with lightFlags |=
(additionalFlags & SAFE_FALLBACK_FLAGS) so importer.ReadFile(path, lightFlags)
uses only the permitted fallback flags (referencing lightFlags, additionalFlags,
SAFE_FALLBACK_FLAGS, convertToLeftHanded, and importer.ReadFile).
| // Some materials can exist without any techniques/passes (e.g. partially loaded | ||
| // script materials). Ensure a valid pass exists before RTSS touches it. | ||
| (void)ensureFirstPass(existingMaterial); | ||
| applyRTSSNormalMap(existingMaterial, normalTexPtr->getName()); |
There was a problem hiding this comment.
Guard RTSS application when pass creation fails.
At Line 66 the return from ensureFirstPass(existingMaterial) is ignored, but Line 67 still calls applyRTSSNormalMap. If pass creation/retrieval fails, this can reintroduce the crash path you’re trying to eliminate.
Suggested fix
- (void)ensureFirstPass(existingMaterial);
- applyRTSSNormalMap(existingMaterial, normalTexPtr->getName());
+ Ogre::Pass* existingPass = ensureFirstPass(existingMaterial);
+ if (!existingPass) {
+ Ogre::LogManager::getSingleton().logMessage(
+ "MaterialProcessor: Skipping RTSS normal map for existing material '" + materialName +
+ "' because no valid pass could be ensured");
+ } else {
+ applyRTSSNormalMap(existingMaterial, normalTexPtr->getName());
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Assimp/MaterialProcessor.cpp` around lines 64 - 67, ensure the code
checks the result of ensureFirstPass(existingMaterial) before calling
applyRTSSNormalMap: capture the return value from ensureFirstPass (or the pass
pointer it returns) and only call applyRTSSNormalMap(existingMaterial,
normalTexPtr->getName()) when ensureFirstPass indicates success; if
ensureFirstPass fails, skip the RTSS normal-map application (and optionally log
or handle the failure) to avoid dereferencing a missing pass.
| bool buildSkeletonOnly(const Ogre::Skeleton* skeleton) | ||
| { | ||
| if (!skeleton) return false; | ||
| m_skeletonOnly = true; | ||
| m_hasSkeleton = true; | ||
| m_skeleton = const_cast<Ogre::Skeleton*>(skeleton); | ||
| m_entity = nullptr; | ||
| m_mesh = nullptr; | ||
|
|
||
| m_skeleton->reset(); | ||
|
|
There was a problem hiding this comment.
Avoid mutating caller state in a const export API.
buildSkeletonOnly() takes a const Ogre::Skeleton*, then casts away constness and calls reset(). That means exporting can snap the caller’s skeleton back to bind pose as a side effect, which is surprising and leaks state through a const API.
Minimal fix
m_skeletonOnly = true;
m_hasSkeleton = true;
m_skeleton = const_cast<Ogre::Skeleton*>(skeleton);
m_entity = nullptr;
m_mesh = nullptr;
-
- m_skeleton->reset();
m_w.writeHeader();📝 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.
| bool buildSkeletonOnly(const Ogre::Skeleton* skeleton) | |
| { | |
| if (!skeleton) return false; | |
| m_skeletonOnly = true; | |
| m_hasSkeleton = true; | |
| m_skeleton = const_cast<Ogre::Skeleton*>(skeleton); | |
| m_entity = nullptr; | |
| m_mesh = nullptr; | |
| m_skeleton->reset(); | |
| bool buildSkeletonOnly(const Ogre::Skeleton* skeleton) | |
| { | |
| if (!skeleton) return false; | |
| m_skeletonOnly = true; | |
| m_hasSkeleton = true; | |
| m_skeleton = const_cast<Ogre::Skeleton*>(skeleton); | |
| m_entity = nullptr; | |
| m_mesh = nullptr; | |
| m_w.writeHeader(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FBX/FBXExporter.cpp` around lines 477 - 487, buildSkeletonOnly currently
casts away const and calls reset() on the caller's Ogre::Skeleton, mutating
external state; instead preserve the const contract by not modifying the input:
set m_skeletonOnly and m_hasSkeleton as before, but do not const_cast the
incoming pointer — either store it as a const pointer (e.g. keep a const
Ogre::Skeleton* m_skeletonConst) or make an internal copy/clone of the skeleton
and call reset() on that copy, then assign m_skeleton to the internal copy;
update references to m_skeleton accordingly (or add/use m_skeletonConst) so
export logic uses the non-mutating/internal copy while leaving the caller's
skeleton unchanged.
| TEST_F(OgreWidgetTest, FsaaDefaultsToZeroWhenUnset) | ||
| { | ||
| QSettings settings; | ||
| settings.remove(ViewportSettingsKeys::fsaaSamples()); | ||
|
|
||
| EXPECT_NO_THROW(widget->rebuildRenderWindow()); | ||
| app->processEvents(); | ||
|
|
||
| EXPECT_EQ(widget->fsaaSamples(), 0u); | ||
| } |
There was a problem hiding this comment.
Keep QSettings state isolated in this test.
At Line 254, the key is removed but never restored. This can leak config across tests and create order-dependent failures.
💡 Suggested test-isolation fix
TEST_F(OgreWidgetTest, FsaaDefaultsToZeroWhenUnset)
{
QSettings settings;
- settings.remove(ViewportSettingsKeys::fsaaSamples());
+ const QString key = ViewportSettingsKeys::fsaaSamples();
+ const bool hadOriginal = settings.contains(key);
+ const QVariant originalValue = settings.value(key);
+ settings.remove(key);
+ settings.sync();
EXPECT_NO_THROW(widget->rebuildRenderWindow());
app->processEvents();
EXPECT_EQ(widget->fsaaSamples(), 0u);
+
+ if (hadOriginal)
+ settings.setValue(key, originalValue);
+ else
+ settings.remove(key);
+ settings.sync();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OgreWidget_test.cpp` around lines 251 - 260, This test modifies global
QSettings by removing ViewportSettingsKeys::fsaaSamples() without restoring it;
update TEST_F(OgreWidgetTest, FsaaDefaultsToZeroWhenUnset) to save the original
presence and value of that key (using QSettings::contains and value), perform
the remove and assertions (including widget->rebuildRenderWindow()), then
restore the original state at the end of the test (re-set the key if it existed
or remove it if it did not) so QSettings state is isolated and order-independent
for other tests.
Fix scan --fix crashes in Ogre/Assimp paths, prevent FBX simplify from bloating files, and enrich scan output with [fixed]/[skipped] tags plus saved-bytes and keys-removed summaries. Default viewport FSAA to 0 when unset and add a regression test. Made-with: Cursor
FBX animation curve channels may be emitted with a single key when the channel is constant. Relax the AnimationCurves test accordingly. Also replace a `strlen`-based suffix length with `std::string_view` sizing for Sonar. Made-with: Cursor
f349021 to
2166e59
Compare
|
1 participant
Summary
qtmesh scan --fixcrashes in the redundant-keyframe FBX fix path (Ogre headless init + safer material handling + empty-track guard).[skipped]).[fixed]/[skipped]tags, bytes saved + keys removed totals, and treat fix-skips as passed while still counted inSkipped.Test plan
UnitTests --gtest_filter=OgreWidgetTest.FsaaDefaultsToZeroWhenUnset(added)UnitTests --gtest_filter=ScanEngine*(existing coverage)QT_QPA_PLATFORM=offscreen QtMeshEditor scan . --fixon EidolaEngine assets (verified during dev)Made with Cursor
Summary by CodeRabbit
Release Notes
New Features
--analyzeflag for skeleton metadata and duration listsBug Fixes
Improvements