feat(materials): texture channel packing (Phase 5 slice G) - #477
Merged
Conversation
Pack 1-4 grayscale source images into the RGBA channels of a single
output texture. Common indie game-dev pattern — Unity ORM
(Occlusion R / Roughness G / Metallic B), Unreal MR (Metallic R /
Roughness G / unused B), per-engine PBR conventions.
Pure-data packer (src/TextureChannelPacker.{h,cpp}):
- PackingSpec with per-channel `path` (sampled as Rec.601 luminance) /
`constantValue` / `invert`, plus overall outputWidth/Height +
includeAlpha.
- pack() returns a QImage; packToFile() writes PNG/TGA/JPG/BMP via
QImageWriter.
- Smaller sources are bilinear-scaled up to the largest source;
all-constants → 256x256 default.
Tests (src/TextureChannelPacker_test.cpp): all-constants default,
ORM 3-channel pack, invert (rough→gloss), mismatched-size scaling,
missing-file error, explicit output size, RGB888 vs RGBA8888, file
write round-trip, empty path, unsupported extension.
CLI subcommand `qtmesh pack-textures`:
qtmesh pack-textures --r ao.png --g rough.png --b metal.png -o orm.png
qtmesh pack-textures --r metal.png --g rough.png --bc 0 --no-alpha -o mr.png
qtmesh pack-textures --r rough.png --invert-r -o gloss.png
MCP tool `pack_textures` with full schema (paths, constants, inverts,
width/height, include_alpha, output) registered alongside the existing
material tools.
Material Mode Inspector hook: "Pack Texture Channels…" button in the
Mode Tools tab right after "Open Material Editor". Opens a top-level
modal Window styled with the same Inspector primitives (Rectangle +
Text + MouseArea over PropertiesPanelController.* colours) used
throughout the rest of the inspector — InspectorButton, InspectorLabel,
InspectorReadOnlyField, InspectorTextField, InspectorPercentField with
up/down arrows (TransformField idiom), InspectorCheckBox.
Documentation: CLAUDE.md CLI examples, recognised-subcommand list,
architecture entry for TextureChannelPacker.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a comprehensive texture channel packing feature for QtMeshEditor. It adds a new ChangesTexture Channel Packing Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 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: c313f1c60e
ℹ️ 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".
| // [--rc <0..1>] [--gc <0..1>] [--bc <0..1>] [--ac <0..1>] | ||
| // [--invert-r] [--invert-g] [--invert-b] [--invert-a] | ||
| // [--width N] [--height N] [--no-alpha] -o out.png | ||
| TextureChannelPacker::PackingSpec spec; |
There was a problem hiding this comment.
The new pack-textures CLI path builds a default PackingSpec but never sets spec.alpha.constantValue to 1.0, so common invocations like --r/--g/--b -o out.png produce RGBA output with A=0 everywhere unless users remember --ac 1 or --no-alpha. That makes the generated texture fully transparent by default, which is an unexpected regression for the documented ORM workflow and can cause downstream tools/pipelines to mishandle RGB data on transparent texels.
Useful? React with 👍 / 👎.
| { | ||
| SentryReporter::addBreadcrumb("ai.tool_call", "pack_textures"); | ||
|
|
||
| TextureChannelPacker::PackingSpec spec; |
There was a problem hiding this comment.
The MCP pack_textures handler has the same default-alpha issue as the CLI: it constructs a fresh PackingSpec and only overrides alpha when alpha/alpha_constant are provided, so calls that omit alpha emit RGBA images with zero alpha. For 3-channel ORM/MR packing through MCP, this silently creates fully transparent outputs unless callers know to set alpha_constant: 1 or disable alpha explicitly.
Useful? React with 👍 / 👎.
Five tests in each of the three integration entry points so the new slice G surfaces are verified end-to-end on Linux CI: CLIPipeline_test.cpp: - MissingOutputFails (usage error 2) - AllConstantsWritesPng (32x32 default-format png) - MissingSourceFileReturnsRuntimeError (exit 1) - NoAlphaProducesRgbPng (--no-alpha branch) - InvertFlagFlipsConstantSource (constant 1.0 + --invert-r → 0) MCPServer_test.cpp: - MissingOutputReturnsError - AllConstantsWritesPng (asserts width/height in result JSON) - MissingSourceReportsError - InvertFlagAppliesToConstant - AppearsInToolList (regression guard for tools/list registration) MaterialEditorQML_test.cpp: - AllConstantsWritesPng (Q_INVOKABLE wrapper happy path) - MissingPathReturnsError - EmptyOutputPathReturnsError - InvertFlagFlipsConstant Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…est sources Linux CI MaterialEditorQML_test (and the parallel _perf, _qml, _runner targets) duplicate the src/ source list at tests/CMakeLists.txt:13–95, and that list was missing TextureChannelPacker.cpp. The link failed with undefined references to TextureChannelPacker::packToFile from both MCPServer.cpp::toolPackTextures and CLIPipeline.cpp::cmdPackTextures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
tests/CMakeLists.txt (1)
88-88: Run the full OS matrix after widening shared test linkage.This line affects all test binaries that consume
TEST_SRC_FILES, so please gate merge on Windows (MinGW), macOS, and Linux (Xvfb) CI for confidence.Based on learnings: "Compile code and run CI build on Windows (MinGW), macOS, and Linux before merging to ensure cross-platform functionality".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/CMakeLists.txt` at line 88, You've expanded shared test linkage by adding ${CMAKE_CURRENT_SOURCE_DIR}/../src/TextureChannelPacker.cpp into TEST_SRC_FILES which impacts all test binaries; before merging, run the complete OS CI matrix (Windows/MinGW, macOS, Linux with Xvfb) to ensure cross-platform build and test compatibility. Trigger CI runs for those platforms, verify failures are fixed (toolchain, include paths, symbol visibility), and only merge once all three OSes pass to avoid platform-specific breakage.src/MCPServer.cpp (1)
4212-4258: 💤 Low valueLGTM: Comprehensive tool schema
The
pack_texturestool schema is well-documented and accurately describes:
- Per-channel source options (image path vs. constant)
- Inversion flags for roughness↔glossiness workflows
- Optional output sizing and alpha inclusion
- Real-world PBR use cases (Unity ORM, Unreal MR)
The required fields correctly list only
output, matching the implementation.Optional: Consider adding to heavy tools for observability
If texture packing proves slow with large images, consider adding
"pack_textures"to theheavyToolsset (lines 454-467) to enable Sentry transaction tracking. This is optional and can be deferred until performance profiling shows it's warranted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MCPServer.cpp` around lines 4212 - 4258, The schema for "pack_textures" is good; optionally add the string "pack_textures" to the heavyTools set (variable heavyTools) so the tool is treated as a heavy operation for observability (Sentry transaction tracking) — locate the heavyTools declaration and include "pack_textures" in that initializer/insert set, e.g. heavyTools.insert("pack_textures") or add it to the initializer list where other heavy tool names are listed.qml/PropertiesPanel.qml (1)
1741-1755: ⚡ Quick winAdd explicit Loader error handling to avoid silent no-op failures.
If the qrc target fails to load, clicking the button currently has no user-visible/diagnostic signal.
♻️ Suggested hardening
Loader { id: textureChannelPackerLoader active: false anchors.centerIn: parent source: "qrc:/MaterialEditorQML/TextureChannelPackerDialog.qml" onLoaded: if (item && item.open) item.open() + onStatusChanged: { + if (status === Loader.Error) + console.error("Failed to load TextureChannelPackerDialog:", source) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qml/PropertiesPanel.qml` around lines 1741 - 1755, The Loader textureChannelPackerLoader currently assumes the QML resource loads successfully and openTextureChannelPackerDialog() is a no-op on failure; add explicit error handling by checking Loader.status and responding to Loader.onStatusChanged/onError: when opening, if status is Loader.Ready call item.open(); if status is Loader.Error or becomes Error, present a user-visible fallback (e.g., show a MessageDialog or call a processLogger/error handler) and set active = false to avoid repeated silent attempts; also update onLoaded to verify item exists before calling open. This touches the Loader with id textureChannelPackerLoader and the function openTextureChannelPackerDialog.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 165: Update the documentation line describing TextureChannelPacker to
include BMP as a supported output format; modify the sentence that currently
lists "PNG/TGA/JPG" to read "PNG/TGA/JPG/BMP" (or otherwise include BMP) so the
feature scope matches the implementation for TextureChannelPacker
(src/TextureChannelPacker.h/cpp) and the related interfaces (qtmesh
pack-textures CLI, pack_textures MCP tool, and the "Pack Channels…" Material
Editor button).
In `@qml/TextureChannelPackerDialog.qml`:
- Around line 49-76: InspectorButton is mouse-only; make it keyboard-operable by
enabling tab focus on the Rectangle (id: btn) and handling Enter/Space in the
MouseArea (id: btnMa): add activeFocusOnTab: true to btn so it can be reached
via Tab, allow the MouseArea to accept focus, and add a Keys.onPressed (or
onKeyPressed) handler in btnMa that calls btn.clicked() when the key is
Qt.Key_Return/Qt.Key_Enter or Qt.Key_Space; apply the same changes to the other
custom controls (the similar block around lines 262-314).
In `@src/CLIPipeline.cpp`:
- Around line 962-963: The help text is missing the "pack-textures" command even
though it's dispatched by cmdPackTextures; update the printUsage() function to
add an entry for "pack-textures" (matching the wording and formatting of other
commands) so users see it in --help output and can discover cmdPackTextures.
Ensure the new help line follows the same alignment and style as the surrounding
usage entries.
- Around line 2653-2657: The breadcrumb currently emitted before packing uses
the generic category "cli.pack-textures"; change breadcrumb usage around
SentryReporter::addBreadcrumb and TextureChannelPacker::packToFile so source
texture inputs emit SentryReporter::addBreadcrumb("file.import", QString("Import
texture -> %1").arg(QFileInfo(inputPath).fileName())) for each source/spec path
and the final output write uses SentryReporter::addBreadcrumb("file.export",
QString("Export texture -> %1").arg(QFileInfo(outputPath).fileName())); keep the
existing message structure but replace the category strings and add per-source
imports before calling TextureChannelPacker::packToFile(spec, outputPath) so I/O
operations follow repository telemetry conventions.
- Around line 2615-2636: The lambda setConst and CLI parsing currently call
QString::toFloat()/toInt() which silently produce 0 on invalid input; change
parsing to use the overloads that provide success flags (QString::toFloat(bool*
ok) and QString::toInt(bool* ok)) when handling the --rc/--gc/--bc/--ac and
--width/--height options, validate the bool ok, enforce channel constants are
within [0.0,1.0] and output dimensions are >0, and on invalid values report a
clear error (including the offending arg and option) and exit non‑zero instead
of silently accepting the value; update references to setConst,
spec.red/green/blue/alpha, and spec.outputWidth/spec.outputHeight accordingly.
In `@src/MaterialEditorQML.cpp`:
- Around line 2811-2833: Add LCOV_EXCL markers around the modal dialog method
MaterialEditorQML::savePackedTextureDialog so it is excluded from coverage;
specifically, wrap the entire function body (or the function definition) with
comment markers // LCOV_EXCL_START and // LCOV_EXCL_STOP like other modal
helpers (e.g., openFileDialog, openMaterialImportDialog) to prevent headless CI
from attempting to execute the QFileDialog call.
In `@src/MCPServer_test.cpp`:
- Around line 5865-5874: The test PackTextures_MissingSourceReportsError uses a
hardcoded absolute path that may exist on some systems; change it to use a
temp-dir-scoped path via the QTemporaryDir tmp created in the test (use
tmp.filePath("nonexistent.png") or similar) and ensure the chosen path does not
exist (e.g., assert !QFile::exists(path)) before calling
server->callTool("pack_textures", args) with args["red"] set to that temp-local
missing path so the test is stable cross-platform; reference the test name
PackTextures_MissingSourceReportsError, the QTemporaryDir tmp, args["red"], and
server->callTool("pack_textures") when applying the change.
In `@src/MCPServer.h`:
- Around line 183-185: The MCP surface gained a new RPC/tool method
(toolPackTextures / "pack_textures") but the SERVER_VERSION constant was not
updated; locate the SERVER_VERSION symbol and bump the semantic version
(increment the minor version, e.g. 1.6.0 -> 1.7.0) wherever it is
defined/exported (headers, build metadata or version source) so clients can
detect the new capability; ensure the updated SERVER_VERSION is the one exposed
to clients.
In `@src/TextureChannelPacker.cpp`:
- Around line 21-39: The loadSource function performs a file read but lacks
Sentry breadcrumbs; add calls to SentryReporter::addBreadcrumb("file.import",
QStringLiteral("reading %1").arg(src.path)) at the start of the read attempt,
addBreadcrumb("file.import", QStringLiteral("read failed %1: %2").arg(src.path,
reader.errorString())) when reader.read() yields a null image (before
returning), and addBreadcrumb("file.import", QStringLiteral("read success
%1").arg(src.path)) after successful conversion (before setting out.present) so
all import attempts, failures, and successes are recorded; reference the
loadSource function, LoadedSource out, and QImageReader reader when inserting
these calls.
- Around line 173-176: The unsupported-write error currently logs only the file
suffix; update the error assignment where writer.canWrite() is checked (the
block setting r.ok and r.error) to include the full output path (outPath) in the
QStringLiteral message instead of or alongside QFileInfo(outPath).suffix(), so
r.error contains a clear message like "cannot write '<outPath>': format
unsupported" referencing outPath and leave r.ok = false unchanged.
- Around line 67-72: The code currently ignores a caller-provided
single-dimension override in resolveOutputSize; instead validate and reject
partial overrides: in resolveOutputSize (or at the start of pack()), check if
exactly one of PackingSpec::outputWidth or ::outputHeight is > 0 and treat that
as an error (e.g., throw std::invalid_argument or return an error status),
surfacing a clear message that both width and height must be provided or
neither; keep the existing auto-size behavior only when both are zero. Include
the check near resolveOutputSize and in pack() where spec is consumed so callers
get immediate, non-silent feedback.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 1741-1755: The Loader textureChannelPackerLoader currently assumes
the QML resource loads successfully and openTextureChannelPackerDialog() is a
no-op on failure; add explicit error handling by checking Loader.status and
responding to Loader.onStatusChanged/onError: when opening, if status is
Loader.Ready call item.open(); if status is Loader.Error or becomes Error,
present a user-visible fallback (e.g., show a MessageDialog or call a
processLogger/error handler) and set active = false to avoid repeated silent
attempts; also update onLoaded to verify item exists before calling open. This
touches the Loader with id textureChannelPackerLoader and the function
openTextureChannelPackerDialog.
In `@src/MCPServer.cpp`:
- Around line 4212-4258: The schema for "pack_textures" is good; optionally add
the string "pack_textures" to the heavyTools set (variable heavyTools) so the
tool is treated as a heavy operation for observability (Sentry transaction
tracking) — locate the heavyTools declaration and include "pack_textures" in
that initializer/insert set, e.g. heavyTools.insert("pack_textures") or add it
to the initializer list where other heavy tool names are listed.
In `@tests/CMakeLists.txt`:
- Line 88: You've expanded shared test linkage by adding
${CMAKE_CURRENT_SOURCE_DIR}/../src/TextureChannelPacker.cpp into TEST_SRC_FILES
which impacts all test binaries; before merging, run the complete OS CI matrix
(Windows/MinGW, macOS, Linux with Xvfb) to ensure cross-platform build and test
compatibility. Trigger CI runs for those platforms, verify failures are fixed
(toolchain, include paths, symbol visibility), and only merge once all three
OSes pass to avoid platform-specific breakage.
🪄 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: 5a27a679-349b-4001-846e-cf958cdae89c
📒 Files selected for processing (20)
CLAUDE.mdqml/PropertiesPanel.qmlqml/TextureChannelPackerDialog.qmlqml/qmldirsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_test.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MCPServer_test.cppsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/MaterialEditorQML_test.cppsrc/TextureChannelPacker.cppsrc/TextureChannelPacker.hsrc/TextureChannelPacker_test.cppsrc/main.cppsrc/qml_resources.qrctests/CMakeLists.txt
|
|
||
| - **BatchExporter** (`src/BatchExporter.h/cpp`): Multi-file conversion wrapping CLIPipeline. Supports progress reporting. | ||
| - **MaterialPresetLibrary** (`src/MaterialPresetLibrary.h/cpp`): QML_SINGLETON providing one-click material presets (Plastic, Metal, Wood, Glass, Unlit, Wireframe). | ||
| - **TextureChannelPacker** (`src/TextureChannelPacker.h/cpp`, slice G): pure-data packer that takes 1-4 grayscale source images (or constants) and writes a single packed RGBA texture (PNG/TGA/JPG). Each output channel is sampled via Rec.601 luminance from its source image, with an optional invert flag (useful for roughness↔glossiness). Smaller sources are bilinear-scaled up to match the largest input. Surfaced via the `qtmesh pack-textures` CLI subcommand, the `pack_textures` MCP tool, and the "Pack Channels…" button in the Material Editor. |
There was a problem hiding this comment.
Update format list to include BMP for the texture packer docs.
This line documents only PNG/TGA/JPG, but the feature scope includes BMP output as well. Keeping this list complete avoids user confusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` at line 165, Update the documentation line describing
TextureChannelPacker to include BMP as a supported output format; modify the
sentence that currently lists "PNG/TGA/JPG" to read "PNG/TGA/JPG/BMP" (or
otherwise include BMP) so the feature scope matches the implementation for
TextureChannelPacker (src/TextureChannelPacker.h/cpp) and the related interfaces
(qtmesh pack-textures CLI, pack_textures MCP tool, and the "Pack Channels…"
Material Editor button).
Comment on lines
+49
to
+76
| component InspectorButton: Rectangle { | ||
| id: btn | ||
| property string label: "" | ||
| property bool buttonEnabled: true | ||
| signal clicked() | ||
| height: 24 | ||
| radius: 3 | ||
| color: btnMa.containsMouse && buttonEnabled | ||
| ? PropertiesPanelController.highlightColor | ||
| : PropertiesPanelController.headerColor | ||
| border.color: PropertiesPanelController.borderColor | ||
| border.width: 1 | ||
| opacity: buttonEnabled ? 1.0 : 0.45 | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: btn.label | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 11 | ||
| } | ||
| MouseArea { | ||
| id: btnMa | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| enabled: btn.buttonEnabled | ||
| cursorShape: btn.buttonEnabled ? Qt.PointingHandCursor : Qt.ForbiddenCursor | ||
| onClicked: btn.clicked() | ||
| } | ||
| } |
There was a problem hiding this comment.
Add keyboard operability for custom button/checkbox controls
These controls are currently mouse-driven only. Please add tab-focus + Enter/Space activation so keyboard-only users can complete the dialog flow.
Suggested patch
component InspectorButton: Rectangle {
id: btn
@@
+ activeFocusOnTab: buttonEnabled
+ Keys.onReturnPressed: if (buttonEnabled) btn.clicked()
+ Keys.onEnterPressed: if (buttonEnabled) btn.clicked()
+ Keys.onSpacePressed: if (buttonEnabled) btn.clicked()
@@
}
component InspectorCheckBox: Rectangle {
id: cbRoot
@@
+ activeFocusOnTab: boxEnabled
+ Keys.onReturnPressed: if (boxEnabled) cbRoot.toggled(!cbRoot.checked)
+ Keys.onEnterPressed: if (boxEnabled) cbRoot.toggled(!cbRoot.checked)
+ Keys.onSpacePressed: if (boxEnabled) cbRoot.toggled(!cbRoot.checked)
@@
}Also applies to: 262-314
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@qml/TextureChannelPackerDialog.qml` around lines 49 - 76, InspectorButton is
mouse-only; make it keyboard-operable by enabling tab focus on the Rectangle
(id: btn) and handling Enter/Space in the MouseArea (id: btnMa): add
activeFocusOnTab: true to btn so it can be reached via Tab, allow the MouseArea
to accept focus, and add a Keys.onPressed (or onKeyPressed) handler in btnMa
that calls btn.clicked() when the key is Qt.Key_Return/Qt.Key_Enter or
Qt.Key_Space; apply the same changes to the other custom controls (the similar
block around lines 262-314).
Comment on lines
+962
to
963
| else if (cmd == "pack-textures") rc = cmdPackTextures(argc, argv); | ||
|
|
There was a problem hiding this comment.
Expose pack-textures in --help output.
The command is dispatched, but it is not listed in printUsage(), so users cannot discover it from help text.
Small usage update
@@ void CLIPipeline::printUsage()
" material --list-presets List the built-in preset names\n"
+ " pack-textures [flags] -o <out> Pack grayscale inputs into RGBA channels\n"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/CLIPipeline.cpp` around lines 962 - 963, The help text is missing the
"pack-textures" command even though it's dispatched by cmdPackTextures; update
the printUsage() function to add an entry for "pack-textures" (matching the
wording and formatting of other commands) so users see it in --help output and
can discover cmdPackTextures. Ensure the new help line follows the same
alignment and style as the surrounding usage entries.
Comment on lines
+2615
to
+2636
| auto setConst = [](TextureChannelPacker::ChannelSource& dst, const QString& v) { | ||
| dst.constantValue = v.toFloat(); | ||
| }; | ||
|
|
||
| for (int i = 1; i < argc; ++i) { | ||
| QString arg(argv[i]); | ||
| if (arg == "pack-textures" || arg == "--cli") continue; | ||
| if ((arg == "--r" || arg == "--red") && i + 1 < argc) { setPath(spec.red, QString(argv[++i])); continue; } | ||
| if ((arg == "--g" || arg == "--green") && i + 1 < argc) { setPath(spec.green, QString(argv[++i])); continue; } | ||
| if ((arg == "--b" || arg == "--blue") && i + 1 < argc) { setPath(spec.blue, QString(argv[++i])); continue; } | ||
| if ((arg == "--a" || arg == "--alpha") && i + 1 < argc) { setPath(spec.alpha, QString(argv[++i])); continue; } | ||
| if (arg == "--rc" && i + 1 < argc) { setConst(spec.red, QString(argv[++i])); continue; } | ||
| if (arg == "--gc" && i + 1 < argc) { setConst(spec.green, QString(argv[++i])); continue; } | ||
| if (arg == "--bc" && i + 1 < argc) { setConst(spec.blue, QString(argv[++i])); continue; } | ||
| if (arg == "--ac" && i + 1 < argc) { setConst(spec.alpha, QString(argv[++i])); continue; } | ||
| if (arg == "--invert-r") { spec.red.invert = true; continue; } | ||
| if (arg == "--invert-g") { spec.green.invert = true; continue; } | ||
| if (arg == "--invert-b") { spec.blue.invert = true; continue; } | ||
| if (arg == "--invert-a") { spec.alpha.invert = true; continue; } | ||
| if (arg == "--width" && i + 1 < argc) { spec.outputWidth = QString(argv[++i]).toInt(); continue; } | ||
| if (arg == "--height" && i + 1 < argc) { spec.outputHeight = QString(argv[++i]).toInt(); continue; } | ||
| if (arg == "--no-alpha") { spec.includeAlpha = false; continue; } |
There was a problem hiding this comment.
Validate numeric CLI inputs instead of silently coercing bad values.
toFloat()/toInt() here accept invalid strings as 0, so typos can silently produce wrong packed outputs. Please validate parse success and enforce --rc/--gc/--bc/--ac in [0,1], and positive --width/--height.
Suggested hardening
- auto setConst = [](TextureChannelPacker::ChannelSource& dst, const QString& v) {
- dst.constantValue = v.toFloat();
- };
+ auto setConst = [&](TextureChannelPacker::ChannelSource& dst, const QString& v, const char* opt) -> bool {
+ bool ok = false;
+ const float parsed = v.toFloat(&ok);
+ if (!ok || parsed < 0.0f || parsed > 1.0f) {
+ err() << "Error: " << opt << " must be a number in [0,1]." << Qt::endl;
+ return false;
+ }
+ dst.constantValue = parsed;
+ return true;
+ };
@@
- if (arg == "--rc" && i + 1 < argc) { setConst(spec.red, QString(argv[++i])); continue; }
- if (arg == "--gc" && i + 1 < argc) { setConst(spec.green, QString(argv[++i])); continue; }
- if (arg == "--bc" && i + 1 < argc) { setConst(spec.blue, QString(argv[++i])); continue; }
- if (arg == "--ac" && i + 1 < argc) { setConst(spec.alpha, QString(argv[++i])); continue; }
+ if (arg == "--rc" && i + 1 < argc) { if (!setConst(spec.red, QString(argv[++i]), "--rc")) return 2; continue; }
+ if (arg == "--gc" && i + 1 < argc) { if (!setConst(spec.green, QString(argv[++i]), "--gc")) return 2; continue; }
+ if (arg == "--bc" && i + 1 < argc) { if (!setConst(spec.blue, QString(argv[++i]), "--bc")) return 2; continue; }
+ if (arg == "--ac" && i + 1 < argc) { if (!setConst(spec.alpha, QString(argv[++i]), "--ac")) return 2; continue; }
@@
- if (arg == "--width" && i + 1 < argc) { spec.outputWidth = QString(argv[++i]).toInt(); continue; }
- if (arg == "--height" && i + 1 < argc) { spec.outputHeight = QString(argv[++i]).toInt(); continue; }
+ if (arg == "--width" && i + 1 < argc) {
+ bool ok = false; const int w = QString(argv[++i]).toInt(&ok);
+ if (!ok || w <= 0) { err() << "Error: --width must be > 0." << Qt::endl; return 2; }
+ spec.outputWidth = w; continue;
+ }
+ if (arg == "--height" && i + 1 < argc) {
+ bool ok = false; const int h = QString(argv[++i]).toInt(&ok);
+ if (!ok || h <= 0) { err() << "Error: --height must be > 0." << Qt::endl; return 2; }
+ spec.outputHeight = h; continue;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/CLIPipeline.cpp` around lines 2615 - 2636, The lambda setConst and CLI
parsing currently call QString::toFloat()/toInt() which silently produce 0 on
invalid input; change parsing to use the overloads that provide success flags
(QString::toFloat(bool* ok) and QString::toInt(bool* ok)) when handling the
--rc/--gc/--bc/--ac and --width/--height options, validate the bool ok, enforce
channel constants are within [0.0,1.0] and output dimensions are >0, and on
invalid values report a clear error (including the offending arg and option) and
exit non‑zero instead of silently accepting the value; update references to
setConst, spec.red/green/blue/alpha, and spec.outputWidth/spec.outputHeight
accordingly.
Comment on lines
+2653
to
+2657
| SentryReporter::addBreadcrumb("cli.pack-textures", | ||
| QString("Pack textures -> %1").arg(QFileInfo(outputPath).fileName())); | ||
|
|
||
| auto r = TextureChannelPacker::packToFile(spec, outputPath); | ||
| if (!r.ok) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use repository-standard breadcrumb categories for texture packing I/O.
This new flow performs file I/O but only emits cli.pack-textures. Please emit file.import for source textures and file.export for the output write path.
Telemetry category alignment
- SentryReporter::addBreadcrumb("cli.pack-textures",
- QString("Pack textures -> %1").arg(QFileInfo(outputPath).fileName()));
+ if (!spec.red.path.isEmpty()) SentryReporter::addBreadcrumb("file.import", QString("Reading %1").arg(spec.red.path));
+ if (!spec.green.path.isEmpty()) SentryReporter::addBreadcrumb("file.import", QString("Reading %1").arg(spec.green.path));
+ if (!spec.blue.path.isEmpty()) SentryReporter::addBreadcrumb("file.import", QString("Reading %1").arg(spec.blue.path));
+ if (!spec.alpha.path.isEmpty()) SentryReporter::addBreadcrumb("file.import", QString("Reading %1").arg(spec.alpha.path));
+ SentryReporter::addBreadcrumb("file.export",
+ QString("Packing texture channels -> %1").arg(QFileInfo(outputPath).absoluteFilePath()));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/CLIPipeline.cpp` around lines 2653 - 2657, The breadcrumb currently
emitted before packing uses the generic category "cli.pack-textures"; change
breadcrumb usage around SentryReporter::addBreadcrumb and
TextureChannelPacker::packToFile so source texture inputs emit
SentryReporter::addBreadcrumb("file.import", QString("Import texture ->
%1").arg(QFileInfo(inputPath).fileName())) for each source/spec path and the
final output write uses SentryReporter::addBreadcrumb("file.export",
QString("Export texture -> %1").arg(QFileInfo(outputPath).fileName())); keep the
existing message structure but replace the category strings and add per-source
imports before calling TextureChannelPacker::packToFile(spec, outputPath) so I/O
operations follow repository telemetry conventions.
Comment on lines
+5865
to
+5874
| TEST_F(MCPServerTest, PackTextures_MissingSourceReportsError) | ||
| { | ||
| QTemporaryDir tmp; | ||
| ASSERT_TRUE(tmp.isValid()); | ||
| QJsonObject args; | ||
| args["red"] = "/nonexistent/should_not_resolve_for_test.png"; | ||
| args["output"] = tmp.filePath("nope.png"); | ||
| QJsonObject result = server->callTool("pack_textures", args); | ||
| EXPECT_TRUE(isError(result)); | ||
| } |
There was a problem hiding this comment.
Use a guaranteed-missing path scoped to the temp directory
The hardcoded /nonexistent/... path can be environment-dependent. Prefer a temp-dir-local path you explicitly ensure does not exist for stable cross-platform behavior.
Suggested patch
TEST_F(MCPServerTest, PackTextures_MissingSourceReportsError)
{
QTemporaryDir tmp;
ASSERT_TRUE(tmp.isValid());
QJsonObject args;
- args["red"] = "/nonexistent/should_not_resolve_for_test.png";
+ const QString missingPath = tmp.filePath("missing_source.png");
+ QFile::remove(missingPath); // Ensure path is absent
+ args["red"] = missingPath;
args["output"] = tmp.filePath("nope.png");
QJsonObject result = server->callTool("pack_textures", args);
EXPECT_TRUE(isError(result));
}Based on learnings: Compile code and run CI build on Windows (MinGW), macOS, and Linux before merging to ensure cross-platform functionality.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TEST_F(MCPServerTest, PackTextures_MissingSourceReportsError) | |
| { | |
| QTemporaryDir tmp; | |
| ASSERT_TRUE(tmp.isValid()); | |
| QJsonObject args; | |
| args["red"] = "/nonexistent/should_not_resolve_for_test.png"; | |
| args["output"] = tmp.filePath("nope.png"); | |
| QJsonObject result = server->callTool("pack_textures", args); | |
| EXPECT_TRUE(isError(result)); | |
| } | |
| TEST_F(MCPServerTest, PackTextures_MissingSourceReportsError) | |
| { | |
| QTemporaryDir tmp; | |
| ASSERT_TRUE(tmp.isValid()); | |
| QJsonObject args; | |
| const QString missingPath = tmp.filePath("missing_source.png"); | |
| QFile::remove(missingPath); // Ensure path is absent | |
| args["red"] = missingPath; | |
| args["output"] = tmp.filePath("nope.png"); | |
| QJsonObject result = server->callTool("pack_textures", args); | |
| EXPECT_TRUE(isError(result)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MCPServer_test.cpp` around lines 5865 - 5874, The test
PackTextures_MissingSourceReportsError uses a hardcoded absolute path that may
exist on some systems; change it to use a temp-dir-scoped path via the
QTemporaryDir tmp created in the test (use tmp.filePath("nonexistent.png") or
similar) and ensure the chosen path does not exist (e.g., assert
!QFile::exists(path)) before calling server->callTool("pack_textures", args)
with args["red"] set to that temp-local missing path so the test is stable
cross-platform; reference the test name PackTextures_MissingSourceReportsError,
the QTemporaryDir tmp, args["red"], and server->callTool("pack_textures") when
applying the change.
Comment on lines
+183
to
+185
| /// Slice G: pack 1-4 grayscale source images into a single RGBA | ||
| /// output texture (e.g. ORM = AO+Roughness+Metallic). | ||
| QJsonObject toolPackTextures(const QJsonObject &args); |
There was a problem hiding this comment.
Bump SERVER_VERSION when adding pack_textures to the MCP surface.
Adding toolPackTextures changes the MCP interface, but SERVER_VERSION is still 1.6.0. That can break capability gating on clients relying on version checks.
Suggested update
- // 1.6.0 — added list_material_presets / apply_material_preset (incl. PBR templates)
- static constexpr const char* SERVER_VERSION = "1.6.0";
+ // 1.6.0 — added list_material_presets / apply_material_preset (incl. PBR templates)
+ // 1.7.0 — added pack_textures
+ static constexpr const char* SERVER_VERSION = "1.7.0";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MCPServer.h` around lines 183 - 185, The MCP surface gained a new
RPC/tool method (toolPackTextures / "pack_textures") but the SERVER_VERSION
constant was not updated; locate the SERVER_VERSION symbol and bump the semantic
version (increment the minor version, e.g. 1.6.0 -> 1.7.0) wherever it is
defined/exported (headers, build metadata or version source) so clients can
detect the new capability; ensure the updated SERVER_VERSION is the one exposed
to clients.
Comment on lines
+21
to
+39
| LoadedSource loadSource(const ChannelSource& src, QString* errOut) | ||
| { | ||
| LoadedSource out; | ||
| out.constantValue = std::clamp(src.constantValue, 0.0f, 1.0f); | ||
| out.invert = src.invert; | ||
| if (src.path.isEmpty()) return out; | ||
| QImageReader reader(src.path); | ||
| QImage img = reader.read(); | ||
| if (img.isNull()) { | ||
| if (errOut) | ||
| *errOut = QStringLiteral("failed to read '%1': %2") | ||
| .arg(src.path, reader.errorString()); | ||
| return out; | ||
| } | ||
| // Convert to RGBA8 once so per-pixel sampling is uniform. | ||
| out.img = img.convertToFormat(QImage::Format_RGBA8888); | ||
| out.present = true; | ||
| return out; | ||
| } |
There was a problem hiding this comment.
Add Sentry breadcrumbs for texture reads/writes in this new I/O path.
This implementation performs significant file import/export operations but does not emit breadcrumbs, which weakens telemetry/debug traces for this feature.
Suggested instrumentation points
+#include "SentryReporter.h"
...
QImage img = reader.read();
if (img.isNull()) {
+ SentryReporter::addBreadcrumb("file.import",
+ QStringLiteral("pack_textures read failed: %1").arg(src.path));
...
}
+SentryReporter::addBreadcrumb("file.import",
+ QStringLiteral("pack_textures read: %1").arg(src.path));
...
if (!writer.write(r.image)) {
+ SentryReporter::addBreadcrumb("file.export",
+ QStringLiteral("pack_textures write failed: %1").arg(outPath));
...
}
+SentryReporter::addBreadcrumb("file.export",
+ QStringLiteral("pack_textures wrote: %1").arg(outPath));As per coding guidelines "**/*.cpp: Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message) ... use 'file.import'/'file.export' for I/O operations".
Also applies to: 161-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TextureChannelPacker.cpp` around lines 21 - 39, The loadSource function
performs a file read but lacks Sentry breadcrumbs; add calls to
SentryReporter::addBreadcrumb("file.import", QStringLiteral("reading
%1").arg(src.path)) at the start of the read attempt,
addBreadcrumb("file.import", QStringLiteral("read failed %1: %2").arg(src.path,
reader.errorString())) when reader.read() yields a null image (before
returning), and addBreadcrumb("file.import", QStringLiteral("read success
%1").arg(src.path)) after successful conversion (before setting out.present) so
all import attempts, failures, and successes are recorded; reference the
loadSource function, LoadedSource out, and QImageReader reader when inserting
these calls.
Comment on lines
+67
to
+72
| QSize resolveOutputSize(const PackingSpec& spec, | ||
| const std::array<LoadedSource, 4>& srcs) | ||
| { | ||
| if (spec.outputWidth > 0 && spec.outputHeight > 0) | ||
| return {spec.outputWidth, spec.outputHeight}; | ||
|
|
There was a problem hiding this comment.
Reject partial output-size overrides instead of silently falling back to auto-size.
If only one dimension is provided, this path ignores the override and proceeds with inferred/default size. That hides bad caller input and makes CLI/MCP behavior harder to reason about.
Suggested guard in pack()
PackResult pack(const PackingSpec& spec)
{
PackResult res;
+
+ const bool widthSet = spec.outputWidth > 0;
+ const bool heightSet = spec.outputHeight > 0;
+ if (spec.outputWidth < 0 || spec.outputHeight < 0 || (widthSet != heightSet)) {
+ res.error = QStringLiteral("outputWidth/outputHeight must both be > 0, or both be 0");
+ return res;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TextureChannelPacker.cpp` around lines 67 - 72, The code currently
ignores a caller-provided single-dimension override in resolveOutputSize;
instead validate and reject partial overrides: in resolveOutputSize (or at the
start of pack()), check if exactly one of PackingSpec::outputWidth or
::outputHeight is > 0 and treat that as an error (e.g., throw
std::invalid_argument or return an error status), surfacing a clear message that
both width and height must be provided or neither; keep the existing auto-size
behavior only when both are zero. Include the check near resolveOutputSize and
in pack() where spec is consumed so callers get immediate, non-silent feedback.
Comment on lines
+173
to
+176
| if (!writer.canWrite()) { | ||
| r.ok = false; | ||
| r.error = QStringLiteral("cannot write '%1': format unsupported") | ||
| .arg(QFileInfo(outPath).suffix()); |
There was a problem hiding this comment.
Use the output path in the unsupported-write error message.
Current message inserts only the file suffix, not the actual target path, which makes troubleshooting failed writes harder.
Suggested message fix
- r.error = QStringLiteral("cannot write '%1': format unsupported")
- .arg(QFileInfo(outPath).suffix());
+ r.error = QStringLiteral("cannot write '%1': format unsupported")
+ .arg(outPath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TextureChannelPacker.cpp` around lines 173 - 176, The unsupported-write
error currently logs only the file suffix; update the error assignment where
writer.canWrite() is checked (the block setting r.ok and r.error) to include the
full output path (outPath) in the QStringLiteral message instead of or alongside
QFileInfo(outPath).suffix(), so r.error contains a clear message like "cannot
write '<outPath>': format unsupported" referencing outPath and leave r.ok =
false unchanged.
|
6 tasks
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
Phase 5 slice G — texture channel packing. Indie game-dev workflow: pack 1–4 grayscale source images into the RGBA channels of a single output texture. Common conventions covered:
What ships
pack(),packToFile()src/TextureChannelPacker.{h,cpp}qtmesh pack-textures --r --g --b --a --rc --gc --bc --ac --invert-{r,g,b,a} --width --height --no-alpha -osrc/CLIPipeline.cpppack_textureswith full schemasrc/MCPServer.cppqml/PropertiesPanel.qml+qml/TextureChannelPackerDialog.qmlDesign notes
0.299·R + 0.587·G + 0.114·B). Per-channelinvertflag flips255 - vafter sampling — useful for roughness↔glossiness conversions.outputWidth/Heightoverrides.QImageWriter—.png/.tga/.jpg/.bmpall work.includeAlpha=falseproduces RGB888.InspectorButton,InspectorLabel,InspectorReadOnlyField,InspectorTextField,InspectorPercentField(TransformField up/down idiom), andInspectorCheckBoxprimitives overPropertiesPanelController.*theme colors.CLI examples
End-to-end smoke-tested locally: 3 PBR PNGs (2048×2048) packed into an ORM texture in <1 s, output verified pixel-perfect against the source greys.
Test plan
TextureChannelPackerTest— 9 unit tests: constants default, ORM 3-channel, invert, mismatched sizes, missing file, explicit size, RGBA vs RGB, file write round-trip, error paths.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
pack-texturessubcommand, andpack_texturesMCP tool