feat(materials): Sobel-filter normal map generator (Phase 5 slice H) - #480
Merged
Merged
Conversation
Generate a tangent-space normal map from a grayscale height/bump source
via a 3x3 Sobel kernel. Closes one of the two remaining Phase 5 epic
acceptance items for "Normal Map Generation":
- ✅ Generate from height/bump map (Sobel filter)
- ✅ Strength/intensity with real-time preview
- ✅ Invert R/G channels for DirectX ↔ OpenGL convention
- ⏳ AI-assisted from diffuse texture — separate slice (depends on SD)
Pure-data generator (src/NormalMapGenerator.{h,cpp}):
- GenSpec(sourcePath, strength, outputWidth/Height, invertR, invertG)
- generate() returns a GenResult with QImage; generateToFile writes via
QImageWriter (PNG/TGA/JPG/BMP).
- Sobel kernel applied to Rec.601 luminance with edge-clamped sampling.
Tangent-space normal n = normalize(-dx*strength, -dy*strength, 1).
- Output is RGB8. invertG is the OpenGL (+Y up, default) ↔ DirectX
(+Y down) switch. Strength clamped to [0..32] so a runaway value
can't produce all-saturated output.
CLI (src/CLIPipeline.cpp): `qtmesh normal-from-height`
- --src, -o, --strength, --invert-r, --invert-g (--directx alias),
--width, --height.
MCP (src/MCPServer.cpp): `generate_normal_map` tool with full schema,
plus `directx` alias for `invert_g`. SERVER_VERSION bumped 1.6.0→1.7.0.
MaterialEditorQML wrappers:
- previewNormalMap(...) returns "data:image/png;base64,..." for live
preview thumbnails (capped 32..512 px).
- generateNormalMap(...) writes the final PNG.
- saveNormalMapDialog() native save-as.
QML dialog (qml/NormalMapGeneratorDialog.qml): top-level Window with
Inspector-styled primitives. Source field with DropArea, strength
slider (0..10), OpenGL/DirectX toggle, save-as picker, 256x256 live
preview thumbnail that updates on every input change. Wired to the
"Generate Normal Map…" button in Material Mode → Mode Tools, right
after "Pack Texture Channels…".
Tests:
- 12 pure-data NormalMapGenerator_test cases (flat→up-normals,
H/V gradient tilts R/G, invert flags, zero strength, missing/empty
source, output size override, RGB888 format, file-write round-trip).
- 4 CLI cmdNormalFromHeight tests (missing args, missing source,
flat-input correctness, --invert-g flips G).
- 5 MCP generate_normal_map tests (missing args, flat-input write,
missing source, tools/list registration, directx alias).
- 5 MaterialEditorQML wrapper tests (generate happy/missing-source,
preview happy/missing/size-clamp).
End-to-end smoke-tested locally via CLI on a 2048x2048 height map and
via HTTP MCP (POST /api/tools/generate_normal_map). DirectX flip
verified pixel-perfect: G_dx + G_gl = 255.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request introduces a complete Normal Map Generator feature for converting grayscale height/bump maps to tangent-space normal maps using a Sobel filter. The feature is exposed across three interfaces: a QML Material Editor UI dialog with live preview, a CLI subcommand for headless workflows, and an MCP server tool for remote generation. The implementation includes core Sobel math, all UI and integration wiring, comprehensive test coverage across 25+ test cases, and updated documentation. ChangesNormal Map Generator Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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: ca2d6f83ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+37
to
+38
| const s = url.toString() | ||
| return s.startsWith("file://") ? s.substring(7) : s |
There was a problem hiding this comment.
The drop handler converts a QUrl to a path via toString() + substring(7), which leaves URL encoding intact (for example %20) and can produce malformed local paths on Windows/UNC forms. In those cases, preview and generation fail even though the user dropped a valid file. This should use a local-file conversion (for example toLocalFile() or equivalent decoding) so drag-and-drop works for paths with spaces, non-ASCII characters, and platform-specific file URL shapes.
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: 8
🧹 Nitpick comments (3)
src/MCPServer_test.cpp (2)
5906-6002: ⚡ Quick winTest coverage could include width/height resize and invert_r flag.
The current tests cover the core functionality, but the PR description mentions additional parameters (
width,height,invertR) that aren't exercised. Consider adding tests for:
- Custom output dimensions (e.g., 64×64 source → 128×128 output)
- The
invert_rflag- Strength bounds (PR says clamped to [0, 32])
These would strengthen confidence that the full feature set works as specified.
🤖 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 5906 - 6002, Add unit tests in MCPServer_test that exercise the generate_normal_map tool's width/height resizing, invert_r behavior, and strength clamping: create tests similar to GenerateNormalMap_FlatHeightWritesPng that use writeGreyPngForMcp (or construct a ramp image) and call server->callTool("generate_normal_map") with explicit "width"/"height" to verify output dimensions (e.g., 64→128), with "invert_r" set to true to assert the red channel is inverted, and with out-of-range "strength" values to assert the implementation clamps them into [0,32]; name the new tests clearly (e.g., GenerateNormalMap_ResizesOutput, GenerateNormalMap_InvertR, GenerateNormalMap_StrengthClamped) and follow the same ASSERT/EXPECT patterns used in existing tests to validate image pixels and error handling.
5977-6002: 💤 Low valueConsider adding explanatory comment for the DirectX test expectations.
The test correctly verifies that
directx=trueinverts the green channel, but the expectation at line 6001 (EXPECT_GT(qGreen(...), 135)) would benefit from a brief comment explaining why green should be >128 after inversion for a vertical ramp.💡 Suggested clarification
+ // Vertical ramp: Sobel dy produces green <128 (OpenGL +Y up). + // With directx=true (invert_g), green should flip to >128. EXPECT_GT(qGreen(gen.pixel(8, 8)), 135);🤖 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 5977 - 6002, In the test GenerateNormalMap_DirectxAliasInvertsGreen, add a short explanatory comment before the assertion that checks qGreen(gen.pixel(8, 8)) > 135 stating that the input vertical ramp produced green values <128 and setting args["directx"] = true (alias for invert_g) causes the green channel to be inverted, so the sampled pixel should be above 128 (hence the >135 threshold); place this comment immediately above the EXPECT_GT(...) assertion to clarify the expectation.src/CLIPipeline_test.cpp (1)
3103-3122: ⚡ Quick winAdd a dedicated
--directxalias testThe CLI contract includes
--invert-g/--directx; this suite validates--invert-gonly. A small alias test would lock compatibility and prevent regressions in argument parsing.Suggested test addition
+TEST(CLIPipelineCmdNormalFromHeight, DirectxAliasAlsoFlipsGreenChannel) +{ + QTemporaryDir tmp; + ASSERT_TRUE(tmp.isValid()); + const QByteArray src = writeGreyPng(tmp, "ramp_y.png", 16, 16, + [](int, int y){ return std::min(255, y * 16); }).toUtf8(); + const QByteArray outPath = tmp.filePath("normal_dx_alias.png").toUtf8(); + TestArgv args({"qtmesh", "normal-from-height", + "--src", src.constData(), + "--strength", "2.0", + "--directx", + "-o", outPath.constData()}); + EXPECT_EQ(CLIPipeline::cmdNormalFromHeight(args.argc(), args.argv()), 0); + + QImage img(outPath); + ASSERT_FALSE(img.isNull()); + EXPECT_GT(qGreen(img.pixel(8, 8)), 135); +}🤖 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_test.cpp` around lines 3103 - 3122, Add a new unit test mirroring TEST(CLIPipelineCmdNormalFromHeight, InvertGFlipsGreenChannel) that verifies the CLI alias --directx behaves identically to --invert-g: construct the same temporary ramp PNG, invoke CLIPipeline::cmdNormalFromHeight with args containing "--directx" instead of "--invert-g" (using the same TestArgv pattern and output path), assert the command returns 0, load the output QImage and assert qGreen(img.pixel(8,8)) is greater than the same threshold (e.g., 135); name the test something like TEST(CLIPipelineCmdNormalFromHeight, DirectXAliasInvertsGreen) so it clearly ties to the original test and prevents regressions in argument parsing.
🤖 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 `@src/CLIPipeline.cpp`:
- Around line 2705-2710: Replace the single "cli.normal-from-height" breadcrumb
with three breadcrumbs to reflect the I/O operations: keep the existing
SentryReporter::addBreadcrumb("cli.normal-from-height", ...) but add
SentryReporter::addBreadcrumb("file.import", ...) just before reading the source
(use spec.sourcePath / QFileInfo(spec.sourcePath).fileName() in the message) and
add SentryReporter::addBreadcrumb("file.export", ...) just before calling
NormalMapGenerator::generateToFile(spec, outputPath) (use outputPath /
QFileInfo(outputPath).fileName() in the message) so telemetry records both the
import and export around the NormalMapGenerator::generateToFile call.
- Around line 2696-2701: The help/usage text for the normal-from-height
subcommand is missing the new --directx alias and the global help listing
doesn't include the normal-from-height command; update the usage output in the
cmdNormalFromHeight handling (the block that prints "Usage: qtmesh
normal-from-height ...") to include the --directx option alongside --invert-g,
and add an entry for "normal-from-height" in the global printUsage() command
list so the subcommand is shown when users run --help; locate the CLI usage
strings in CLIPipeline.cpp (the cmdNormalFromHeight message) and the
printUsage() function and add the corresponding text entries.
In `@src/MaterialEditorQML_test.cpp`:
- Around line 2695-2702: The helper writeGreyPngForNormal currently ignores the
return value of img.save(...) which can hide fixture creation failures; modify
writeGreyPngForNormal to capture the bool result of img.save(path, "PNG") and
assert or fatal out if it returns false (e.g., Q_ASSERT(ok) or qFatal with a
message including the path and dimensions) so the test fails early and with a
clear message when PNG creation fails.
In `@src/MaterialEditorQML.cpp`:
- Around line 2924-2946: The saveNormalMapDialog function opens a user-facing
save dialog but doesn't record a breadcrumb; add a SentryReporter::addBreadcrumb
call before invoking QFileDialog in MaterialEditorQML::saveNormalMapDialog using
the 'file.export' category and a clear message like "Open Save Normal Map
dialog" so the action is tracked (place it immediately before the
QFileDialog::getSaveFileName call).
- Around line 2974-2990: In generateNormalMap, keep the existing
SentryReporter::addBreadcrumb("ui.action", "Generate normal map") but also
record the disk write by adding a SentryReporter::addBreadcrumb("file.export",
...) around the NormalMapGenerator::generateToFile call (use a descriptive
message like "Export normal map" and include the outputPath or other context),
so that NormalMapGenerator::generateToFile's I/O is tracked in addition to the
UI action.
In `@src/MCPServer.cpp`:
- Around line 3413-3416: The current logic treats "directx" as only setting
invertG when true and ignores conflicts; instead treat "directx" as a strict
alias of "invert_g": if args.contains("directx") set spec.invertG =
args.value("directx").toBool() (before/overriding any invert_g), and if both
"directx" and "invert_g" are present and their boolean values differ, either
reject the input or log an explicit conflict error; update the block that
references args.contains("directx"), args.value("directx"),
args.contains("invert_g"), and args.value("invert_g") accordingly.
- Around line 3401-3423: The code calls
SentryReporter::addBreadcrumb("ai.tool_call", "generate_normal_map") but does
not record the file I/O operations; add SentryReporter::addBreadcrumb calls
before reading the source and after writing the output: emit
SentryReporter::addBreadcrumb("file.import", spec.sourcePath) (or a descriptive
message) immediately after spec.sourcePath is set/validated and before calling
NormalMapGenerator::generateToFile, and emit
SentryReporter::addBreadcrumb("file.export", outPath) after generation succeeds
(i.e., after NormalMapGenerator::generateToFile returns ok) so both the input
and output paths are tracked alongside the existing ai.tool_call breadcrumb.
In `@src/NormalMapGenerator_test.cpp`:
- Around line 1-7: The test file is missing an explicit include of <algorithm>,
causing uses of std::min in NormalMapGenerator_test.cpp to rely on transitive
includes; add `#include` <algorithm> near the other includes at the top of the
file so std::min (used in the test cases) is declared portably and builds on
stricter toolchains.
---
Nitpick comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 3103-3122: Add a new unit test mirroring
TEST(CLIPipelineCmdNormalFromHeight, InvertGFlipsGreenChannel) that verifies the
CLI alias --directx behaves identically to --invert-g: construct the same
temporary ramp PNG, invoke CLIPipeline::cmdNormalFromHeight with args containing
"--directx" instead of "--invert-g" (using the same TestArgv pattern and output
path), assert the command returns 0, load the output QImage and assert
qGreen(img.pixel(8,8)) is greater than the same threshold (e.g., 135); name the
test something like TEST(CLIPipelineCmdNormalFromHeight,
DirectXAliasInvertsGreen) so it clearly ties to the original test and prevents
regressions in argument parsing.
In `@src/MCPServer_test.cpp`:
- Around line 5906-6002: Add unit tests in MCPServer_test that exercise the
generate_normal_map tool's width/height resizing, invert_r behavior, and
strength clamping: create tests similar to GenerateNormalMap_FlatHeightWritesPng
that use writeGreyPngForMcp (or construct a ramp image) and call
server->callTool("generate_normal_map") with explicit "width"/"height" to verify
output dimensions (e.g., 64→128), with "invert_r" set to true to assert the red
channel is inverted, and with out-of-range "strength" values to assert the
implementation clamps them into [0,32]; name the new tests clearly (e.g.,
GenerateNormalMap_ResizesOutput, GenerateNormalMap_InvertR,
GenerateNormalMap_StrengthClamped) and follow the same ASSERT/EXPECT patterns
used in existing tests to validate image pixels and error handling.
- Around line 5977-6002: In the test GenerateNormalMap_DirectxAliasInvertsGreen,
add a short explanatory comment before the assertion that checks
qGreen(gen.pixel(8, 8)) > 135 stating that the input vertical ramp produced
green values <128 and setting args["directx"] = true (alias for invert_g) causes
the green channel to be inverted, so the sampled pixel should be above 128
(hence the >135 threshold); place this comment immediately above the
EXPECT_GT(...) assertion to clarify the expectation.
🪄 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: 156f9578-8f37-4e83-a9ea-7eda4cadf564
📒 Files selected for processing (20)
CLAUDE.mdqml/NormalMapGeneratorDialog.qmlqml/PropertiesPanel.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/NormalMapGenerator.cppsrc/NormalMapGenerator.hsrc/NormalMapGenerator_test.cppsrc/main.cppsrc/qml_resources.qrctests/CMakeLists.txt
Comment on lines
+2696
to
+2701
| if (spec.sourcePath.isEmpty() || outputPath.isEmpty()) { | ||
| err() << "Error: missing --src or -o." << Qt::endl; | ||
| err() << "Usage: qtmesh normal-from-height --src <height.png>" << Qt::endl; | ||
| err() << " [--strength N] [--invert-r] [--invert-g]" << Qt::endl; | ||
| err() << " [--width N --height N]" << Qt::endl; | ||
| err() << " -o <normal.png>" << Qt::endl; |
There was a problem hiding this comment.
Document the new CLI alias and command in help output
cmdNormalFromHeight accepts --directx, but the usage text here only advertises --invert-g, and global printUsage() also needs the new normal-from-height command so users can discover it via --help.
Suggested patch
diff --git a/src/CLIPipeline.cpp b/src/CLIPipeline.cpp
@@
- err() << "Usage: qtmesh normal-from-height --src <height.png>" << Qt::endl;
- err() << " [--strength N] [--invert-r] [--invert-g]" << Qt::endl;
+ err() << "Usage: qtmesh normal-from-height --src <height.png>" << Qt::endl;
+ err() << " [--strength N] [--invert-r] [--invert-g|--directx]" << Qt::endl;
err() << " [--width N --height N]" << Qt::endl;
err() << " -o <normal.png>" << Qt::endl;
@@
" material --list-presets List the built-in preset names\n"
+ " normal-from-height --src <img> -o <out>\n"
+ " Generate tangent-space normal map (Sobel)\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 2696 - 2701, The help/usage text for the
normal-from-height subcommand is missing the new --directx alias and the global
help listing doesn't include the normal-from-height command; update the usage
output in the cmdNormalFromHeight handling (the block that prints "Usage: qtmesh
normal-from-height ...") to include the --directx option alongside --invert-g,
and add an entry for "normal-from-height" in the global printUsage() command
list so the subcommand is shown when users run --help; locate the CLI usage
strings in CLIPipeline.cpp (the cmdNormalFromHeight message) and the
printUsage() function and add the corresponding text entries.
Comment on lines
+2705
to
+2710
| SentryReporter::addBreadcrumb("cli.normal-from-height", | ||
| QString("Normal map from %1 -> %2") | ||
| .arg(QFileInfo(spec.sourcePath).fileName(), | ||
| QFileInfo(outputPath).fileName())); | ||
|
|
||
| auto r = NormalMapGenerator::generateToFile(spec, outputPath); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use file.import / file.export breadcrumb categories for this I/O operation
This path performs a source image read + output file write but only logs cli.normal-from-height. Add explicit file.import and file.export breadcrumbs to align with telemetry conventions.
Suggested patch
- SentryReporter::addBreadcrumb("cli.normal-from-height",
- QString("Normal map from %1 -> %2")
- .arg(QFileInfo(spec.sourcePath).fileName(),
- QFileInfo(outputPath).fileName()));
+ SentryReporter::addBreadcrumb("file.import",
+ QString("Read height map %1").arg(QFileInfo(spec.sourcePath).absoluteFilePath()));
+ SentryReporter::addBreadcrumb("file.export",
+ QString("Write normal map %1").arg(QFileInfo(outputPath).absoluteFilePath()));As per coding guidelines **/*.cpp: Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations.
📝 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
| SentryReporter::addBreadcrumb("cli.normal-from-height", | |
| QString("Normal map from %1 -> %2") | |
| .arg(QFileInfo(spec.sourcePath).fileName(), | |
| QFileInfo(outputPath).fileName())); | |
| auto r = NormalMapGenerator::generateToFile(spec, outputPath); | |
| SentryReporter::addBreadcrumb("file.import", | |
| QString("Read height map %1").arg(QFileInfo(spec.sourcePath).absoluteFilePath())); | |
| SentryReporter::addBreadcrumb("file.export", | |
| QString("Write normal map %1").arg(QFileInfo(outputPath).absoluteFilePath())); | |
| auto r = NormalMapGenerator::generateToFile(spec, outputPath); |
🤖 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 2705 - 2710, Replace the single
"cli.normal-from-height" breadcrumb with three breadcrumbs to reflect the I/O
operations: keep the existing
SentryReporter::addBreadcrumb("cli.normal-from-height", ...) but add
SentryReporter::addBreadcrumb("file.import", ...) just before reading the source
(use spec.sourcePath / QFileInfo(spec.sourcePath).fileName() in the message) and
add SentryReporter::addBreadcrumb("file.export", ...) just before calling
NormalMapGenerator::generateToFile(spec, outputPath) (use outputPath /
QFileInfo(outputPath).fileName() in the message) so telemetry records both the
import and export around the NormalMapGenerator::generateToFile call.
Comment on lines
+2695
to
+2702
| QString writeGreyPngForNormal(const QTemporaryDir& dir, const QString& name, | ||
| int w, int h, int grey) | ||
| { | ||
| QImage img(w, h, QImage::Format_RGBA8888); | ||
| img.fill(qRgba(grey, grey, grey, 255)); | ||
| const QString path = dir.filePath(name); | ||
| img.save(path, "PNG"); | ||
| return path; |
There was a problem hiding this comment.
Assert test fixture PNG creation succeeds.
Line 2701 ignores img.save(...) result. If fixture creation fails, failures surface later as less-informative generator errors.
Proposed patch
QString writeGreyPngForNormal(const QTemporaryDir& dir, const QString& name,
int w, int h, int grey)
{
QImage img(w, h, QImage::Format_RGBA8888);
img.fill(qRgba(grey, grey, grey, 255));
const QString path = dir.filePath(name);
- img.save(path, "PNG");
+ EXPECT_TRUE(img.save(path, "PNG")) << path.toStdString();
return path;
}🤖 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/MaterialEditorQML_test.cpp` around lines 2695 - 2702, The helper
writeGreyPngForNormal currently ignores the return value of img.save(...) which
can hide fixture creation failures; modify writeGreyPngForNormal to capture the
bool result of img.save(path, "PNG") and assert or fatal out if it returns false
(e.g., Q_ASSERT(ok) or qFatal with a message including the path and dimensions)
so the test fails early and with a clear message when PNG creation fails.
Comment on lines
+2924
to
+2946
| QString MaterialEditorQML::saveNormalMapDialog() | ||
| { | ||
| QString texturesPath = "./media/materials/textures"; | ||
| QDir texturesDir(texturesPath); | ||
| QString startDir = texturesDir.exists() ? texturesDir.absolutePath() : QDir::currentPath(); | ||
|
|
||
| QApplication::processEvents(); | ||
| if (QWidget *activeWin = QApplication::activeWindow()) { | ||
| activeWin->raise(); | ||
| activeWin->activateWindow(); | ||
| } | ||
| QApplication::processEvents(); | ||
|
|
||
| QString selectedFile = QFileDialog::getSaveFileName( | ||
| QApplication::activeWindow(), | ||
| "Save Normal Map", | ||
| startDir + "/normal.png", | ||
| "PNG (*.png);;TGA (*.tga);;JPEG (*.jpg *.jpeg);;BMP (*.bmp)", | ||
| nullptr, | ||
| QFileDialog::DontUseNativeDialog | QFileDialog::DontUseCustomDirectoryIcons | ||
| ); | ||
| return selectedFile; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add breadcrumb for opening the normal-map save dialog.
Line 2937 triggers a user-facing save workflow, but no breadcrumb is recorded for that action.
Proposed patch
QString MaterialEditorQML::saveNormalMapDialog()
{
+ SentryReporter::addBreadcrumb("ui.action", "Open normal map save dialog");
QString texturesPath = "./media/materials/textures";As per coding guidelines, "**/*.cpp: Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations".
🤖 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/MaterialEditorQML.cpp` around lines 2924 - 2946, The saveNormalMapDialog
function opens a user-facing save dialog but doesn't record a breadcrumb; add a
SentryReporter::addBreadcrumb call before invoking QFileDialog in
MaterialEditorQML::saveNormalMapDialog using the 'file.export' category and a
clear message like "Open Save Normal Map dialog" so the action is tracked (place
it immediately before the QFileDialog::getSaveFileName call).
Comment on lines
+2974
to
+2990
| QString MaterialEditorQML::generateNormalMap(const QString& sourcePath, | ||
| double strength, | ||
| bool invertR, | ||
| bool invertG, | ||
| const QString& outputPath) | ||
| { | ||
| SentryReporter::addBreadcrumb("ui.action", "Generate normal map"); | ||
|
|
||
| NormalMapGenerator::GenSpec spec; | ||
| spec.sourcePath = sourcePath; | ||
| spec.strength = static_cast<float>(strength); | ||
| spec.invertR = invertR; | ||
| spec.invertG = invertG; | ||
|
|
||
| auto r = NormalMapGenerator::generateToFile(spec, outputPath); | ||
| return r.ok ? QString() : r.error; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Record file-export breadcrumb for normal map generation output.
Line 2988 writes to disk (generateToFile), but only a "ui.action" breadcrumb is emitted. Add a "file.export" breadcrumb for the I/O operation.
Proposed patch
QString MaterialEditorQML::generateNormalMap(const QString& sourcePath,
double strength,
bool invertR,
bool invertG,
const QString& outputPath)
{
SentryReporter::addBreadcrumb("ui.action", "Generate normal map");
+ SentryReporter::addBreadcrumb("file.export", QString("Export normal map: %1").arg(outputPath));As per coding guidelines, "**/*.cpp: Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations".
📝 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
| QString MaterialEditorQML::generateNormalMap(const QString& sourcePath, | |
| double strength, | |
| bool invertR, | |
| bool invertG, | |
| const QString& outputPath) | |
| { | |
| SentryReporter::addBreadcrumb("ui.action", "Generate normal map"); | |
| NormalMapGenerator::GenSpec spec; | |
| spec.sourcePath = sourcePath; | |
| spec.strength = static_cast<float>(strength); | |
| spec.invertR = invertR; | |
| spec.invertG = invertG; | |
| auto r = NormalMapGenerator::generateToFile(spec, outputPath); | |
| return r.ok ? QString() : r.error; | |
| } | |
| QString MaterialEditorQML::generateNormalMap(const QString& sourcePath, | |
| double strength, | |
| bool invertR, | |
| bool invertG, | |
| const QString& outputPath) | |
| { | |
| SentryReporter::addBreadcrumb("ui.action", "Generate normal map"); | |
| SentryReporter::addBreadcrumb("file.export", QString("Export normal map: %1").arg(outputPath)); | |
| NormalMapGenerator::GenSpec spec; | |
| spec.sourcePath = sourcePath; | |
| spec.strength = static_cast<float>(strength); | |
| spec.invertR = invertR; | |
| spec.invertG = invertG; | |
| auto r = NormalMapGenerator::generateToFile(spec, outputPath); | |
| return r.ok ? QString() : r.error; | |
| } |
🤖 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/MaterialEditorQML.cpp` around lines 2974 - 2990, In generateNormalMap,
keep the existing SentryReporter::addBreadcrumb("ui.action", "Generate normal
map") but also record the disk write by adding a
SentryReporter::addBreadcrumb("file.export", ...) around the
NormalMapGenerator::generateToFile call (use a descriptive message like "Export
normal map" and include the outputPath or other context), so that
NormalMapGenerator::generateToFile's I/O is tracked in addition to the UI
action.
Comment on lines
+3401
to
+3423
| SentryReporter::addBreadcrumb("ai.tool_call", "generate_normal_map"); | ||
|
|
||
| NormalMapGenerator::GenSpec spec; | ||
| spec.sourcePath = args.value("source").toString(); | ||
| if (args.contains("strength")) | ||
| spec.strength = static_cast<float>(args.value("strength").toDouble()); | ||
| if (args.contains("width")) | ||
| spec.outputWidth = args.value("width").toInt(); | ||
| if (args.contains("height")) | ||
| spec.outputHeight = args.value("height").toInt(); | ||
| if (args.contains("invert_r")) | ||
| spec.invertR = args.value("invert_r").toBool(); | ||
| if (args.contains("invert_g")) | ||
| spec.invertG = args.value("invert_g").toBool(); | ||
| if (args.contains("directx") && args.value("directx").toBool()) | ||
| spec.invertG = true; // alias for invert_g | ||
|
|
||
| const QString outPath = args.value("output").toString(); | ||
| if (spec.sourcePath.isEmpty() || outPath.isEmpty()) | ||
| return makeErrorResult("Error: missing required 'source' and 'output' arguments"); | ||
|
|
||
| auto r = NormalMapGenerator::generateToFile(spec, outPath); | ||
| if (!r.ok) |
There was a problem hiding this comment.
Add file I/O breadcrumbs for this tool path.
This flow performs file read/write but only emits ai.tool_call. Add file.import and file.export breadcrumbs around generation to satisfy telemetry requirements.
Proposed patch
QJsonObject MCPServer::toolGenerateNormalMap(const QJsonObject &args)
{
SentryReporter::addBreadcrumb("ai.tool_call", "generate_normal_map");
@@
const QString outPath = args.value("output").toString();
if (spec.sourcePath.isEmpty() || outPath.isEmpty())
return makeErrorResult("Error: missing required 'source' and 'output' arguments");
+ SentryReporter::addBreadcrumb("file.import",
+ QStringLiteral("Read height map: %1").arg(spec.sourcePath));
+ SentryReporter::addBreadcrumb("file.export",
+ QStringLiteral("Write normal map: %1").arg(outPath));
+
auto r = NormalMapGenerator::generateToFile(spec, outPath);
if (!r.ok)
return makeErrorResult(QString("Error: %1").arg(r.error));As per coding guidelines, "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations".
🤖 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 3401 - 3423, The code calls
SentryReporter::addBreadcrumb("ai.tool_call", "generate_normal_map") but does
not record the file I/O operations; add SentryReporter::addBreadcrumb calls
before reading the source and after writing the output: emit
SentryReporter::addBreadcrumb("file.import", spec.sourcePath) (or a descriptive
message) immediately after spec.sourcePath is set/validated and before calling
NormalMapGenerator::generateToFile, and emit
SentryReporter::addBreadcrumb("file.export", outPath) after generation succeeds
(i.e., after NormalMapGenerator::generateToFile returns ok) so both the input
and output paths are tracked alongside the existing ai.tool_call breadcrumb.
Comment on lines
+3413
to
+3416
| if (args.contains("invert_g")) | ||
| spec.invertG = args.value("invert_g").toBool(); | ||
| if (args.contains("directx") && args.value("directx").toBool()) | ||
| spec.invertG = true; // alias for invert_g |
There was a problem hiding this comment.
Make directx a strict alias of invert_g.
At Line [3415], directx=false is ignored. If both fields are sent with conflicting values, output can be unintuitive. If directx is present, set spec.invertG directly from it (or reject conflicts).
Proposed patch
if (args.contains("invert_g"))
spec.invertG = args.value("invert_g").toBool();
- if (args.contains("directx") && args.value("directx").toBool())
- spec.invertG = true; // alias for invert_g
+ if (args.contains("directx"))
+ spec.invertG = args.value("directx").toBool(); // alias for invert_g🤖 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 3413 - 3416, The current logic treats
"directx" as only setting invertG when true and ignores conflicts; instead treat
"directx" as a strict alias of "invert_g": if args.contains("directx") set
spec.invertG = args.value("directx").toBool() (before/overriding any invert_g),
and if both "directx" and "invert_g" are present and their boolean values
differ, either reject the input or log an explicit conflict error; update the
block that references args.contains("directx"), args.value("directx"),
args.contains("invert_g"), and args.value("invert_g") accordingly.
Comment on lines
+1
to
+7
| #include <gtest/gtest.h> | ||
|
|
||
| #include <QImage> | ||
| #include <QTemporaryDir> | ||
|
|
||
| #include "NormalMapGenerator.h" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify std::min usage and missing direct include in this file.
rg -n 'std::min\s*\(' src/NormalMapGenerator_test.cpp
rg -n '^#include <algorithm>$' src/NormalMapGenerator_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 419
🏁 Script executed:
cat -n src/NormalMapGenerator_test.cpp | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 1064
🏁 Script executed:
# Check what Qt headers and gtest might include
rg -A 5 '^#include' src/NormalMapGenerator_test.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 223
🏁 Script executed:
cat -n src/NormalMapGenerator.h | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 2121
🏁 Script executed:
# Check if NormalMapGenerator.h includes algorithm
rg '^#include.*algorithm' src/NormalMapGenerator.hRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if gtest.h or Qt headers might transitively include algorithm
rg 'std::min|#include.*algorithm' /usr/include/gtest/gtest.h 2>/dev/null | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Better check: Look at the full test file to understand the context
wc -l src/NormalMapGenerator_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 106
🏁 Script executed:
# Get the lines where std::min is used to see the full context
sed -n '55,65p; 75,85p; 92,102p; 110,120p' src/NormalMapGenerator_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1652
🏁 Script executed:
# Check if other _test.cpp files explicitly include <algorithm>
fd -e '\.cpp' -e '\.h' src/ | grep -i test | xargs rg '#include.*algorithm' 2>/dev/nullRepository: fernandotonon/QtMeshEditor
Length of output: 1202
🏁 Script executed:
# Check if any other source files use std::min and how they handle it
rg 'std::min' src/ | grep -v '_test.cpp' | head -10Repository: fernandotonon/QtMeshEditor
Length of output: 1007
🏁 Script executed:
# Verify the coding guidelines requirement for test files
echo "Checking if test files must compile portably across toolchains..."
# Look for any makefile or build config that shows compiler strictness
fd -e CMakeLists.txt -e '*.mk' | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 136
Add explicit <algorithm> include for std::min portability.
std::min is used at lines 61, 80, 97, and 115, but this file doesn't include <algorithm>. Relying on transitive includes breaks builds on stricter toolchains. Other test files in the codebase (FBXExporter_test.cpp, ScanEngine_test.cpp) explicitly include it.
Proposed patch
`#include` <gtest/gtest.h>
+#include <algorithm>
`#include` <QImage>
`#include` <QTemporaryDir>📝 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
| #include <gtest/gtest.h> | |
| #include <QImage> | |
| #include <QTemporaryDir> | |
| #include "NormalMapGenerator.h" | |
| `#include` <gtest/gtest.h> | |
| `#include` <algorithm> | |
| `#include` <QImage> | |
| `#include` <QTemporaryDir> | |
| `#include` "NormalMapGenerator.h" | |
🤖 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/NormalMapGenerator_test.cpp` around lines 1 - 7, The test file is missing
an explicit include of <algorithm>, causing uses of std::min in
NormalMapGenerator_test.cpp to rely on transitive includes; add `#include`
<algorithm> near the other includes at the top of the file so std::min (used in
the test cases) is declared portably and builds on stricter toolchains.
|
This was referenced May 12, 2026
2 tasks
5 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 H — Sobel-filter normal map generator. Closes one of the two remaining Phase 5 epic acceptance items for Normal Map Generation:
What ships
generate(),generateToFile()src/NormalMapGenerator.{h,cpp}qtmesh normal-from-height --src bump.png [--strength N] [--invert-r] [--invert-g/--directx] [--width N --height N] -o normal.pngsrc/CLIPipeline.cppgenerate_normal_map(withdirectxalias). SERVER_VERSION 1.6.0 → 1.7.0src/MCPServer.cppqml/PropertiesPanel.qml+qml/NormalMapGeneratorDialog.qmlDesign notes
n = normalize(-dx*strength, -dy*strength, 1).invertGflips the green channel: that's the OpenGL (+Y up, default) ↔ DirectX (+Y down) switch. MCP/CLI also accept--directx/"directx": trueas an alias.[0, 32]so a runaway value can't produce all-saturated output.Windowstyled with Inspector primitives (Rectangle + Text + MouseAreaoverPropertiesPanelController.*colors) — same idiom asTextureChannelPackerDialog.CLI examples
End-to-end smoke-tested:
--strength 5,--invert-g, resized to 256, missing-file error). DirectX flip verified pixel-perfect:G_dx + G_gl = 255.POST /api/tools/generate_normal_map: happy path,directxalias, missing source error, missing-args error — all match expected behaviour.Test plan
NormalMapGeneratorTestcases (flat→up-normals, H/V gradient tilts R/G, invert flags, zero strength, missing/empty source, output size override, RGB888 format, file-write round-trip, error paths)CLIPipelineCmdNormalFromHeighttests (missing args, missing source, flat-input correctness,--invert-gflips green)MCPServerTest.GenerateNormalMap*tests (missing args, flat-input write with width/height in result, missing source, tools/list registration,directxalias)MaterialEditorQMLTestwrapper tests (generate happy/missing-source, preview happy/missing/size-clamp)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
normal-from-heightcommand), and programmatic access