Add CLI pipeline (qtmesh) for headless mesh operations#178
Open
fernandotonon wants to merge 5 commits intomasterfrom
Open
Add CLI pipeline (qtmesh) for headless mesh operations#178fernandotonon wants to merge 5 commits intomasterfrom
fernandotonon wants to merge 5 commits intomasterfrom
Conversation
Simplify FixOptions to only expose --remove-degenerates and --merge-materials as extra flags on top of the standard import pipeline (which already applies join-vertices, smooth-normals, optimize, etc.). Thread additionalFlags through MeshImporterExporter::importer() and AssimpToOgreImporter::loadModel() so they get OR'd into the standard Assimp post-processing flags. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire CLIPipeline into main.cpp with automatic detection via qtmesh symlink, --cli flag, or recognized subcommand. Create qtmesh symlink at build time (macOS/Linux) and batch wrapper (Windows). Expose AnimationMerger::renameAnimation as public API for CLI anim --rename. Suppress Ogre/Qt debug noise in test output. Document all CLI commands in CLAUDE.md and README.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a headless CLI pipeline (qtmesh) with subcommands Changes
Sequence Diagram(s)sequenceDiagram
participant User as User CLI
participant Main as main.cpp
participant Pipeline as CLIPipeline
participant QApp as hidden QApplication
participant Ogre as Ogre Engine
participant Importer as MeshImporterExporter/Assimp
participant FS as File I/O
participant Out as stdout/stderr
User->>Main: launch executable with args
Main->>Main: detect CLI mode (name / --cli / subcommand)
alt CLI Mode
Main->>Pipeline: CLIPipeline::run(argc, argv)
Pipeline->>Pipeline: parse globals (--help,--version,--verbose)
Pipeline->>QApp: create hidden QApplication
Pipeline->>Out: redirect stdout/stderr (suppress engine noise)
Pipeline->>Ogre: init headless Ogre context
Pipeline->>Importer: load(path, convertLH, additionalFlags)
Importer->>FS: ReadFile via Assimp
Importer->>Ogre: produce Ogre::Mesh / Entity
Pipeline->>Pipeline: perform subcommand (info/convert/fix/anim)
Pipeline->>Out: emit text or JSON results
Pipeline->>Main: return exit code
else GUI Mode
Main->>QApp: normal GUI startup
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test_main.cpp (1)
54-62: Minor: Clarify comment about createLog parameters.The inline comment
// suppressDebugOut, suppressFileOutputdoesn't match Ogre'screateLogsignature which is(name, defaultLog, debuggerOutput, suppressFileOutput). The actual effect is correct (suppresses debugger output and file output), but the comment is misleading.📝 Suggested fix
// Suppress Ogre log output (debug spam from Root, RenderSystem, plugins). // Must be done before any Manager::getSingleton() call creates Root. if (!Ogre::LogManager::getSingletonPtr()) { auto* logMgr = new Ogre::LogManager(); - logMgr->createLog("ogre.log", true, false, true); // suppressDebugOut, suppressFileOutput + logMgr->createLog("ogre.log", true, false, true); // defaultLog, debuggerOutput=false, suppressFileOutput }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test_main.cpp` around lines 54 - 62, The comment next to the Ogre::LogManager::createLog call is misleading about its parameters; update the inline comment near Ogre::LogManager::getSingletonPtr()/createLog("ogre.log", true, false, true) to reflect the actual signature (name, defaultLog, debuggerOutput, suppressFileOutput) and state that the call sets defaultLog=true, disables debuggerOutput (false) and enables suppressFileOutput (true), leaving qInstallMessageHandler(testMessageHandler) unchanged.src/CMakeLists.txt (1)
279-299: Windows wrapper usesfile(WRITE)at configure-time instead ofPOST_BUILD.The Windows
.cmdwrapper is created viafile(WRITE)which executes at CMake configure time, unlike macOS/Linux which useadd_custom_command(POST_BUILD). This creates an inconsistency:
- The wrapper is created before the build starts
- If
CMAKE_PROJECT_NAMEorCMAKE_RUNTIME_OUTPUT_DIRECTORYchanges, the wrapper won't update until the next CMake reconfigurationThis is functionally acceptable since the wrapper content is static, but for consistency, consider using a
POST_BUILDcommand on Windows as well.♻️ Optional: Use POST_BUILD on Windows for consistency
elseif(WIN32) - file(WRITE "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/qtmesh.cmd" - "@echo off\n\"%~dp0${CMAKE_PROJECT_NAME}.exe\" --cli %*\n") + add_custom_command(TARGET ${CMAKE_PROJECT_NAME} POST_BUILD + COMMAND ${CMAKE_COMMAND} -E echo "@echo off" > "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/qtmesh.cmd" + COMMAND ${CMAKE_COMMAND} -E echo "\"%%~dp0${CMAKE_PROJECT_NAME}.exe\" --cli %%*" >> "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/qtmesh.cmd" + COMMENT "Creating qtmesh.cmd wrapper" + )Alternatively, use
file(GENERATE)which evaluates generator expressions at generation time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CMakeLists.txt` around lines 279 - 299, The Windows wrapper is being created with file(WRITE) at configure-time causing inconsistency with the macOS/Linux add_custom_command(... POST_BUILD) flow; replace the configure-time file(WRITE) for "qtmesh.cmd" with a POST_BUILD step (use add_custom_command(TARGET ${CMAKE_PROJECT_NAME} POST_BUILD COMMAND ${CMAKE_COMMAND} -E echo_append / file generation) that writes the same "@echo off\n\"%~dp0${CMAKE_PROJECT_NAME}.exe\" --cli %*\n" into "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/qtmesh.cmd"), or alternatively switch to file(GENERATE) so generator expressions like ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} and ${CMAKE_PROJECT_NAME} are evaluated at generation time; target symbols to edit: the existing file(WRITE) invocation, add_custom_command, CMAKE_PROJECT_NAME, CMAKE_RUNTIME_OUTPUT_DIRECTORY, and qtmesh.cmd.src/CLIPipeline.cpp (1)
555-568: Consider extracting shared Assimp flags constant.These flags duplicate the ones in
src/Assimp/Importer.cpp(lines 41-54). If the base import flags change, this copy could drift out of sync.💡 Suggestion: Extract shared constant
Consider defining a shared constant (e.g., in
Importer.hor a common header):// In a shared location constexpr unsigned int kStandardAssimpFlags = aiProcess_CalcTangentSpace | aiProcess_JoinIdenticalVertices | ...;Then both
Importer.cpp::loadModelandCLIPipeline.cpp::cmdFixcan reference it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 555 - 568, The aiProcess_* flag set assigned to stdFlags in CLIPipeline.cpp is duplicated from Assimp/Importer.cpp; extract a single shared constexpr (e.g., kStandardAssimpFlags) into a common header (for example Importer.h or a new AssimpFlags.h) and replace the local stdFlags usage in CLIPipeline.cpp::cmdFix with that constant; likewise update Importer.cpp::loadModel to use the same kStandardAssimpFlags so both locations (stdFlags and loadModel) reference the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/CLIPipeline.cpp`:
- Around line 555-568: The aiProcess_* flag set assigned to stdFlags in
CLIPipeline.cpp is duplicated from Assimp/Importer.cpp; extract a single shared
constexpr (e.g., kStandardAssimpFlags) into a common header (for example
Importer.h or a new AssimpFlags.h) and replace the local stdFlags usage in
CLIPipeline.cpp::cmdFix with that constant; likewise update
Importer.cpp::loadModel to use the same kStandardAssimpFlags so both locations
(stdFlags and loadModel) reference the single source of truth.
In `@src/CMakeLists.txt`:
- Around line 279-299: The Windows wrapper is being created with file(WRITE) at
configure-time causing inconsistency with the macOS/Linux add_custom_command(...
POST_BUILD) flow; replace the configure-time file(WRITE) for "qtmesh.cmd" with a
POST_BUILD step (use add_custom_command(TARGET ${CMAKE_PROJECT_NAME} POST_BUILD
COMMAND ${CMAKE_COMMAND} -E echo_append / file generation) that writes the same
"@echo off\n\"%~dp0${CMAKE_PROJECT_NAME}.exe\" --cli %*\n" into
"${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/qtmesh.cmd"), or alternatively switch to
file(GENERATE) so generator expressions like ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
and ${CMAKE_PROJECT_NAME} are evaluated at generation time; target symbols to
edit: the existing file(WRITE) invocation, add_custom_command,
CMAKE_PROJECT_NAME, CMAKE_RUNTIME_OUTPUT_DIRECTORY, and qtmesh.cmd.
In `@src/test_main.cpp`:
- Around line 54-62: The comment next to the Ogre::LogManager::createLog call is
misleading about its parameters; update the inline comment near
Ogre::LogManager::getSingletonPtr()/createLog("ogre.log", true, false, true) to
reflect the actual signature (name, defaultLog, debuggerOutput,
suppressFileOutput) and state that the call sets defaultLog=true, disables
debuggerOutput (false) and enables suppressFileOutput (true), leaving
qInstallMessageHandler(testMessageHandler) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c6fc995-fbc8-44a4-b1b2-97ef9c6bbc16
📒 Files selected for processing (14)
CLAUDE.mdREADME.mdsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/Assimp/Importer.cppsrc/Assimp/Importer.hsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_test.cppsrc/CMakeLists.txtsrc/MeshImporterExporter.cppsrc/MeshImporterExporter.hsrc/main.cppsrc/test_main.cpp
![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: 483758ef68
ℹ️ 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 1; | ||
| } | ||
|
|
||
| AnimationMerger::renameAnimation(skel.get(), oldName.toStdString(), newName.toStdString()); |
There was a problem hiding this comment.
The anim --rename path checks that oldName exists but never checks whether newName already exists before calling AnimationMerger::renameAnimation. In that case, the rename path attempts to create a duplicate animation name on the skeleton and can raise an Ogre exception/abort instead of returning a controlled CLI error, so a user typo can terminate the command unexpectedly.
Useful? React with 👍 / 👎.
src/CLIPipeline.cpp
Outdated
|
|
||
| // Load animation files | ||
| for (const auto& f : mergeAnimFiles) | ||
| MeshImporterExporter::importer({f}); |
There was a problem hiding this comment.
cmdMergeAnimations imports each file in mergeAnimFiles but does not verify that each import actually succeeded. If one path is wrong (or unreadable) while others load, the command can still succeed and write output missing expected animations, which is a silent data-loss behavior for batch pipelines.
Useful? React with 👍 / 👎.
Document all qtmesh commands (info, convert, fix, anim, merge-animations) in the project landing page. Update existing CLI references to use the qtmesh command instead of ./QtMeshEditor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/index.html`:
- Around line 650-653: The multiline shell examples using backslash
continuations (e.g., the qtmesh merge-animations example and the other
occurrences around the other examples) assume a POSIX shell and will break when
pasted into Windows cmd.exe; update the docs to either collapse each example
into a single-line command or add a short note stating these snippets use POSIX
`\` line continuations and need to be adapted for Windows (e.g., join into one
line or use PowerShell/cmd alternatives), and apply the same change to the other
examples mentioned in the comment.
Consolidate the standalone `merge-animations` command into the existing `anim` subcommand as `--merge`, matching the pattern of `--list` and `--rename`. Remove legacy merge handling from main.cpp. Address CodeRabbit review feedback: - P1: Guard anim --rename against duplicate target animation names - P2: Verify each merge animation file loaded successfully - Add POSIX shell note to docs for Windows users Add 30+ process-based CLI tests covering all subcommands, error paths, and flag combinations for near-complete CLIPipeline coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
docs/index.html (1)
650-652:⚠️ Potential issue | 🟡 MinorMake this merge example Windows-safe too.
This is still a POSIX multiline snippet, but the shell note now lives much later in the page. If someone copies from this section directly,
cmd.exeusers will still hit a broken command. Either collapse this one to a single line or add a local note beside it.💡 Low-friction fix
-qtmesh anim character.fbx \ - --merge walk.fbx run.fbx jump.fbx idle.fbx \ - -o character_merged.mesh +qtmesh anim character.fbx --merge walk.fbx run.fbx jump.fbx idle.fbx -o character_merged.mesh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/index.html` around lines 650 - 652, The POSIX-style multiline example "qtmesh anim character.fbx --merge walk.fbx run.fbx jump.fbx idle.fbx -o character_merged.mesh" will break for cmd.exe users; update the snippet where that command appears to be Windows-safe by either collapsing it to a single line ("qtmesh anim character.fbx --merge ... -o character_merged.mesh") or adding a short local note showing the Windows cmd.exe equivalent (use ^ line-continuation or recommend pasting the single-line command) so Windows users can run the "qtmesh anim" merge example without errors.
🧹 Nitpick comments (1)
src/CLIPipeline_test.cpp (1)
267-285: These tests never exercise theqtmeshentrypoint.Lines 267-285 always return
QtMeshEditor(.exe), so this suite validates subcommand auto-detection but not the new symlink/cmd wrapper path. A brokenqtmeshlauncher or executable-name detection insrc/main.cppwould still ship green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 267 - 285, The test helper findAppBinary currently always returns the QtMeshEditor binary, so tests never exercise the new 'qtmesh' entrypoint; update findAppBinary to first look for a 'qtmesh' launcher/symlink (e.g., testBinDir + "/qtmesh" and on Windows "/qtmesh.exe", and the macOS equivalent inside the .app bundle if applicable) and return it when present, falling back to QtMeshEditor only if no 'qtmesh' is found; additionally adjust or add tests to invoke both executable names so subcommand auto-detection and the new wrapper path are validated (references: function findAppBinary).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/index.html`:
- Around line 679-739: Add a short platform note clarifying that on Windows the
shipped wrapper is named qtmesh.cmd (so examples using "qtmesh" should be run as
"qtmesh.cmd" on Windows); update the explanatory paragraph near the command
examples (the block that references qtmesh, multiline continuations, and flags
like --verbose/--help) to include a parenthetical or sentence: "On Windows use
qtmesh.cmd" and, if helpful, a brief note to join multiline examples into one
line for PowerShell—leave all existing examples using "qtmesh" unchanged for
POSIX.
In `@README.md`:
- Around line 48-72: Update the README's CLI section to note that on Windows the
executable is shipped as the wrapper "qtmesh.cmd" (instead of "qtmesh"), and
adjust examples or add a short sentence: mention both names (qtmesh /
qtmesh.cmd), suggest using "qtmesh.cmd" on Windows or adding the app folder to
PATH, and recommend using "--help" or "--verbose" the same way; reference the
command examples (qtmesh info, qtmesh convert, qtmesh fix, qtmesh anim) so
readers know to substitute qtmesh.cmd on Windows when copying those commands.
In `@src/CLIPipeline_test.cpp`:
- Around line 400-411: Tests like TEST(CLIPipelineCLI, InfoNonexistentFile) (and
the other tests at the listed ranges) hard-code POSIX /tmp paths; update them to
use QDir::tempPath() or create a QTemporaryDir and build file paths with
QDir::separator() (or QDir::toNativeSeparators) so the tests run on Windows too.
Replace occurrences of "/tmp/nonexistent_file_12345.fbx" (and other "/tmp/..."
strings in the other tests) by constructing the path from QDir::tempPath() +
QDir::separator() + "nonexistent_file_12345.fbx" or by creating a QTemporaryDir
and using its path()/filePath("nonexistent_file_12345.fbx"); apply the same
change to the tests in the other ranges noted in the comment.
In `@src/CLIPipeline.cpp`:
- Around line 428-435: The loop over entities writes one top-level JSON object
per entity (using formatMeshInfoJson) which produces invalid JSON when multiple
entities exist; change the logic in the entities loop so that when jsonOutput is
true you collect each MeshInfo (from extractMeshInfo) and produce a single JSON
array (or top-level object with an array) before calling cliWrite once, rather
than calling cliWrite per entity; keep the existing per-entity text path
(formatMeshInfoText) unchanged and only change the jsonOutput branch to
aggregate results and serialize once.
- Around line 341-360: Remove the unnecessary QApplication creations in the CLI
help/version/no-command paths so the binary won't try to initialize X11 before
deciding CLI-only behavior: when detecting "--help", "-h", "--version", "-v", or
no subcommand, call printUsage() or printVersion() directly (they use
cliWrite()) and return without instantiating QApplication; leave the main
QApplication instantiation used for rendering commands intact (the later
QApplication created for commands that require Ogre should remain), and ensure
no other code in these early-exit branches triggers GUI platform initialization
(this avoids the forceX11PlatformIfNeeded() side-effect causing X11 connection
attempts).
In `@src/main.cpp`:
- Around line 61-69: The CLI auto-detection in main.cpp sets cliMode by checking
argv[1] only, so when a flag like --verbose precedes the subcommand (e.g.,
"--verbose info") the app falls back to GUI; update the detection in main() (the
cliMode logic) to scan argv[] for either the explicit "--cli" or any supported
subcommand token ("info","fix","convert","anim") while ignoring known option
flags such as "--verbose" (i.e., treat "-" or "--" prefixed tokens as options to
skip) so that CLIPipeline::run() is invoked when a supported subcommand appears
anywhere in the argument list even if preceded by options.
---
Duplicate comments:
In `@docs/index.html`:
- Around line 650-652: The POSIX-style multiline example "qtmesh anim
character.fbx --merge walk.fbx run.fbx jump.fbx idle.fbx -o
character_merged.mesh" will break for cmd.exe users; update the snippet where
that command appears to be Windows-safe by either collapsing it to a single line
("qtmesh anim character.fbx --merge ... -o character_merged.mesh") or adding a
short local note showing the Windows cmd.exe equivalent (use ^ line-continuation
or recommend pasting the single-line command) so Windows users can run the
"qtmesh anim" merge example without errors.
---
Nitpick comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 267-285: The test helper findAppBinary currently always returns
the QtMeshEditor binary, so tests never exercise the new 'qtmesh' entrypoint;
update findAppBinary to first look for a 'qtmesh' launcher/symlink (e.g.,
testBinDir + "/qtmesh" and on Windows "/qtmesh.exe", and the macOS equivalent
inside the .app bundle if applicable) and return it when present, falling back
to QtMeshEditor only if no 'qtmesh' is found; additionally adjust or add tests
to invoke both executable names so subcommand auto-detection and the new wrapper
path are validated (references: function findAppBinary).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23cfef57-8e51-482d-8b83-2a79b4b20885
📒 Files selected for processing (9)
CLAUDE.mdREADME.mddocs/index.htmlsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_test.cppsrc/MergeAnimationsCLI_test.cppsrc/TestHelpers.hsrc/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
- Remove unnecessary QApplication from --help/--version/no-command paths (fixes crash on headless X11 systems, speeds up help/version output) - Fix info --json for multi-entity imports: emit single object for one entity, JSON array for multiple entities - Fix CLI auto-detection to handle --verbose before subcommand (e.g., "QtMeshEditor --verbose info foo.fbx" now routes to CLI) - Replace hard-coded /tmp/ paths in tests with QDir::tempPath() for Windows portability - Add Windows qtmesh.cmd note to docs/index.html and README.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/CLIPipeline.cpp (2)
678-686: Minor: Single-dash flags may be collected as merge files.The loop at lines 681-682 stops collecting merge files when it sees
--prefixed flags or-o, but single-dash flags (if any were added later) would be incorrectly collected as filenames. Consider usingarg.startsWith("-")for consistency:💡 Suggested fix
// Collect files until next --flag or end - while (i + 1 < argc && QString(argv[i + 1]).left(2) != "--" - && QString(argv[i + 1]) != "-o") { + while (i + 1 < argc) { + QString next(argv[i + 1]); + if (next.startsWith("-")) break; mergeFiles.append(QString(argv[++i])); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 678 - 686, The merge-file collection loop in the "--merge" branch (variables: arg, mergeMode, mergeFiles) currently stops only for args beginning with "--" or for "-o", which allows single-dash flags to be mistaken for filenames; update the while-condition to stop when the next token starts with "-" (e.g., use QString(argv[i+1]).startsWith("-")) so any flag (single- or double-dash) halts collection, then continue appending files as before.
540-542: In-place overwrite may corrupt file if export fails.When no
-ois specified,outputPathdefaults toinputPath(line 541). If the export at line 613 fails midway, the original file may be corrupted or truncated. CLI tools typically use atomic writes (write to temp file, then rename on success) to prevent data loss.This is a minor concern since the user is explicitly choosing to overwrite, but worth considering for robustness.
💡 Optional: Atomic write pattern
+ // For in-place operations, write to temp file first + QString actualOutput = outputPath; + bool inPlace = (outputPath == inputPath); + if (inPlace) { + actualOutput = outputPath + ".tmp"; + } + QFileInfo outFi(outputPath); - int result = MeshImporterExporter::exporter(node, outFi.absoluteFilePath(), fmt); + QFileInfo actualOutFi(actualOutput); + int result = MeshImporterExporter::exporter(node, actualOutFi.absoluteFilePath(), fmt); if (result != 0) { err() << "Error: Export failed." << Qt::endl; + if (inPlace) QFile::remove(actualOutput); return 1; } + if (inPlace) { + QFile::remove(outputPath); + QFile::rename(actualOutput, outputPath); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 540 - 542, The code currently sets outputPath = inputPath which risks corrupting the original if the export fails; change the export to perform an atomic write by creating a temporary file in the same directory (e.g., outputPath + ".tmp" or unique suffix), write the export output to that temp file (ensure data is flushed/synced), and only on successful completion replace/rename the temp file to outputPath (atomic rename/replace), cleaning up the temp file on error; reference the variables outputPath and inputPath and the existing export/write routine so you write to a temp path first and then rename/replace the original.
🤖 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/CLIPipeline.cpp`:
- Around line 151-160: The conversion of hiddenWidget->winId() to string must
follow OgreWidget.cpp's platform-specific pattern: when building for Windows
cast hiddenWidget->winId() to unsigned long before calling
Ogre::StringConverter::toString, but on non-Windows platforms call
Ogre::StringConverter::toString with the WId directly so the correct overload is
used; update the conversion used in the CLIPipeline.cpp block that prepares
params["externalWindowHandle"] (the hiddenWidget usage before
Manager::getSingleton()->getRoot()->createRenderWindow) to implement this
conditional cast, and apply the same change to the analogous code in
TestHelpers.h to keep behavior consistent across both places.
---
Nitpick comments:
In `@src/CLIPipeline.cpp`:
- Around line 678-686: The merge-file collection loop in the "--merge" branch
(variables: arg, mergeMode, mergeFiles) currently stops only for args beginning
with "--" or for "-o", which allows single-dash flags to be mistaken for
filenames; update the while-condition to stop when the next token starts with
"-" (e.g., use QString(argv[i+1]).startsWith("-")) so any flag (single- or
double-dash) halts collection, then continue appending files as before.
- Around line 540-542: The code currently sets outputPath = inputPath which
risks corrupting the original if the export fails; change the export to perform
an atomic write by creating a temporary file in the same directory (e.g.,
outputPath + ".tmp" or unique suffix), write the export output to that temp file
(ensure data is flushed/synced), and only on successful completion
replace/rename the temp file to outputPath (atomic rename/replace), cleaning up
the temp file on error; reference the variables outputPath and inputPath and the
existing export/write routine so you write to a temp path first and then
rename/replace the original.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee73b0bc-9bab-469d-88d4-50d06ecb1cd5
📒 Files selected for processing (5)
README.mddocs/index.htmlsrc/CLIPipeline.cppsrc/CLIPipeline_test.cppsrc/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CLIPipeline_test.cpp
Comment on lines
+151
to
+160
| try { | ||
| Ogre::NameValuePairList params; | ||
| params["externalWindowHandle"] = Ogre::StringConverter::toString( | ||
| static_cast<unsigned long>(hiddenWidget->winId())); | ||
| #ifdef Q_OS_MACOS | ||
| params["macAPI"] = "cocoa"; | ||
| params["macAPICocoaUseNSView"] = "true"; | ||
| #endif | ||
| Manager::getSingleton()->getRoot()->createRenderWindow( | ||
| "CLIHidden", 1, 1, false, ¶ms); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other parts of the codebase handle winId() to externalWindowHandle conversion
rg -n "externalWindowHandle" src/ -A 2 -B 2Repository: fernandotonon/QtMeshEditor
Length of output: 1273
🏁 Script executed:
#!/bin/bash
# Check how OgreWidget.cpp constructs winHandle
sed -n '140,180p' src/OgreWidget.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1164
🏁 Script executed:
#!/bin/bash
# Look for any other references to winId() and platform-specific handling
rg -n "winId()" src/ -B 2 -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 1469
🏁 Script executed:
#!/bin/bash
# Check if there are any compilation issues or comments about WId casting
rg -n "WId|unsigned long.*winId" src/ -B 2 -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 1079
🏁 Script executed:
#!/bin/bash
# Look at the full context of TestHelpers.h initOgreHeadless to compare
sed -n '110,130p' src/TestHelpers.hRepository: fernandotonon/QtMeshEditor
Length of output: 748
🏁 Script executed:
#!/bin/bash
# Check if StringConverter has overloads or documentation for different types
rg -n "StringConverter::toString" src/ | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 2847
🏁 Script executed:
#!/bin/bash
# Look for any Ogre-related documentation or comments about WId handling
rg -n "StringConverter" src/ -B 1 -A 1 --context=2Repository: fernandotonon/QtMeshEditor
Length of output: 50382
Align with OgreWidget.cpp's platform-specific pattern for window handle conversion.
The code lacks platform-specific handling like OgreWidget.cpp uses. OgreWidget.cpp demonstrates the proper approach: cast to unsigned long on Windows, but pass winId() directly to StringConverter::toString() on other platforms, since the function has overloads for the WId type. Apply this same pattern here for consistency:
💡 Suggested fix
Ogre::NameValuePairList params;
+#ifdef WIN32
+ params["externalWindowHandle"] = Ogre::StringConverter::toString(
+ (unsigned long)(hiddenWidget->winId()));
+#else
params["externalWindowHandle"] = Ogre::StringConverter::toString(
- static_cast<unsigned long>(hiddenWidget->winId()));
+ hiddenWidget->winId());
+#endifApply the same fix to TestHelpers.h (line 119–120) for consistency across the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CLIPipeline.cpp` around lines 151 - 160, The conversion of
hiddenWidget->winId() to string must follow OgreWidget.cpp's platform-specific
pattern: when building for Windows cast hiddenWidget->winId() to unsigned long
before calling Ogre::StringConverter::toString, but on non-Windows platforms
call Ogre::StringConverter::toString with the WId directly so the correct
overload is used; update the conversion used in the CLIPipeline.cpp block that
prepares params["externalWindowHandle"] (the hiddenWidget usage before
Manager::getSingleton()->getRoot()->createRenderWindow) to implement this
conditional cast, and apply the same change to the analogous code in
TestHelpers.h to keep behavior consistent across both places.
|
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
qtmeshCLI tool — headless command-line interface for 3D mesh operations, built alongside the GUI binaryinfo,fix,convert,anim(list/rename),merge-animationsqtmeshsymlink,--cliflag, or recognized subcommand as first argumentqtmeshsymlink (macOS/Linux) orqtmesh.cmdwrapper (Windows) created automatically at build timeAnimationMerger::renameAnimation()— exposed for CLIanim --rename(was previously a private free function)CLI Usage
Files Changed
src/CLIPipeline.cpp/hsrc/CLIPipeline_test.cppsrc/main.cppsrc/CMakeLists.txtsrc/AnimationMerger.cpp/hrenameAnimationpublic staticsrc/Assimp/Importer.cpp/hsrc/MeshImporterExporter.cpp/hsrc/test_main.cppCLAUDE.mdREADME.mdTest plan
qtmesh --helpprints usageqtmesh --versionprints versionqtmesh info <file>shows mesh info (text and JSON)qtmesh convert <input> -o <output>converts between formatsqtmesh fix <input> -o <output>re-imports/exports with optimizationsqtmesh anim <file> --listlists animationsqtmesh anim <file> --rename <old> <new>renames animationqtmesh merge-animationsmerges animation filesCLIPipeline_test.cpp)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores