feat(textures): apply-atlas — consume a packed manifest (Phase 6 slice E2)#511
Open
fernandotonon wants to merge 3 commits into
Open
feat(textures): apply-atlas — consume a packed manifest (Phase 6 slice E2)#511fernandotonon wants to merge 3 commits into
fernandotonon wants to merge 3 commits into
Conversation
…e E2) Slice E (#505) shipped the packer + UV-manifest writer. This slice ships the consumption side: a pure-data `ApplyAtlas` module that reads the manifest produced by `TextureAtlasPacker::manifestToJson` and rewrites a mesh so it renders the same scene with one binding instead of N. ## What it does - Reads the manifest JSON strictly — half-baked manifests surface the specific missing field instead of producing wrong UVs downstream. - Two-pass walk per Ogre::Entity: 1. Snapshot every submesh's pre-mutation diffuse texture name. Mixamo-style assets share one Material across many submeshes (one Skin_MAT for face / arms / body); a naive single-pass walk sees the swapped-to-atlas binding on the second submesh and reports "no match" for everything after the first. 2. Rewrite UV0 in place (FLOAT2 only) — scale+bias from `[0..1]` into the matched tile's `[u0..u1, v0..v1]` sub-rect — then retarget each unique Material's diffuse TUS exactly once. - After the swap, strips every non-diffuse TUS on affected materials (normal, AO, emissive, …) because they sample UV0 — which is now diffuse-atlas-relative, so they'd sample the wrong region. Also clears the slice #507 `qtme.normal_map` UOB hint so FBX export doesn't re-emit a stale NormalMap connection. `--keep-extras` (CLI) / `keep_extras: true` (MCP) opts out for users who pre- atlased auxiliary channels to match. - After each unique Material is mutated, calls `RTShaderHelper::wirePbrSlotsForFFP` + `mat->compile()/reload()` so the FFP+RTSS lighting path recomputes against the new binding. Without this, lighting reads back the cached pre-swap binding and looks subtly off. ## UV-clamping policy UVs outside `[0..1]` are clamped before remapping by default — that's what every other game-engine atlas tool does, and tiling is impossible to preserve through a sub-rect anyway. Pass `--no-clamp` / `no_clamp: true` to skip out-of-range UVs and have them surface in the report's `outOfRangeUVs` count instead. ## Surfaces - CLI `qtmesh atlas-apply <file> -o <output> --manifest <atlas.json> --atlas <atlas.png> [--match {basename|fullpath}] [--no-clamp] [--keep-extras] [--json]` - MCP tool `apply_atlas` with the same option set - Inspector "Apply Atlas to Mesh…" button (Material Mode → Mode Tools, sibling of "Pack Atlas…") with `qml/ApplyAtlasDialog.qml`. Three drop-area path inputs (mesh / manifest / atlas) + output + match- mode pills + the two checkboxes. ## Tests Standalone (no Ogre needed) — manifest parser round-trip + rejection of malformed JSON + report JSON shape. Auto-discovered by the gtest glob; Linux CI runs them. ## Verified locally Packed a 2-texture atlas (Boss_diffuse + soccer_ball), applied to Rumba Dancing.fbx via the CLI: 11/11 submeshes rewritten, normal map TUSes stripped (no more "Failed to load Boss_normal.png" on re-import), lighting reads against the atlas correctly. Phase 6 epic #261 — the last remaining slice (apart from Draco / slice F #506) is now done. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds an "ApplyAtlas" feature enabling texture atlas application to meshes. It implements manifest JSON parsing, UV0 remapping into atlas sub-rectangles, diffuse texture rebinding, and exports the modified mesh. The feature is exposed via QML dialog UI, C++ QML bridge, CLI command, and MCP protocol tool. ChangesApplyAtlas Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
Niche follow-up tools shouldn't take general-UI space. The "Apply Atlas to Mesh…" button now lives inside the Pack Atlas dialog's action row, not in Material Mode → Mode Tools. Clicking it loads the Apply Atlas sub-dialog and auto-fills the just-packed atlas + manifest paths so the typical "pack → apply" flow is one extra click after Pack. The PropertiesPanel button + standalone Loader are removed; the ApplyAtlasDialog.qml file itself is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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: bec472f68b
ℹ️ 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
+126
to
+127
| const size_t numV = vData->vertexCount; | ||
| for (size_t v = 0; v < numV; ++v) { |
There was a problem hiding this comment.
When useSharedVertices is true, this loop rewrites every vertex in mesh->sharedVertexData for each matched submesh, not just the vertices indexed by that submesh. In meshes where multiple submeshes share the vertex buffer, this causes cross-submesh UV corruption (including repeated remaps when multiple submeshes match different tiles), so unrelated or later-processed parts of the mesh get incorrect atlas coordinates.
Useful? React with 👍 / 👎.
| const auto mat = p.subEnt->getMaterial(); | ||
| const std::string matName = mat ? mat->getName() : std::string(); | ||
| if (!matName.empty() && retargetedMats.insert(matName).second) { | ||
| sr.materialUpdated = retargetDiffuseTus(p.subEnt, atlasTex); |
There was a problem hiding this comment.
The material is retargeted to the atlas unconditionally for matched submeshes even if rewriteUv0 touched zero vertices (e.g., missing UV0, unsupported UV element type, or all UVs skipped with --no-clamp). That leaves the material sampling atlas-space while geometry still uses original UVs, producing incorrect texturing; retargeting should be gated on successful UV rewrite for that material/submesh set.
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: 6
🧹 Nitpick comments (2)
src/MaterialEditorQML.cpp (2)
3181-3232: 💤 Low valueThree file-dialog helpers are near-identical — extract a common helper.
openManifestDialog,openMeshDialog, andsaveAtlasedMeshDialogall repeat the same boilerplate (computestartDir,processEvents(), raise/activate the active window,processEvents()again, then callQFileDialog::get{Open,Save}FileNamewith the same options). A small static helper taking (title, startDir, filter, isSave, defaultName) would remove ~40 lines of duplication and keep all three in sync if the dialog options ever change.🤖 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 3181 - 3232, The three near-identical functions openManifestDialog, openMeshDialog, and saveAtlasedMeshDialog should be consolidated by adding a single static helper (e.g., showFileDialog) that performs the shared boilerplate (compute startDir, call QApplication::processEvents(), raise/activate QApplication::activeWindow(), processEvents() again) and then calls QFileDialog::getOpenFileName or QFileDialog::getSaveFileName based on a parameter; the helper should accept parameters: title, startDir (or compute inside if empty), filter, a boolean isSave, and an optional defaultName for save dialogs, and must preserve the existing QFileDialog::DontUseNativeDialog | QFileDialog::DontUseCustomDirectoryIcons flags and the nullptr selectedFilter argument; replace the bodies of openManifestDialog, openMeshDialog, and saveAtlasedMeshDialog with calls to this helper passing the appropriate title, filter, isSave flag and default name (e.g., "atlased.fbx").
3308-3313: ⚡ Quick winAtlas resource location is never removed — state leaks across invocations.
addResourceLocation(..., DEFAULT_RESOURCE_GROUP_NAME, false, true)permanently adds the atlas image's directory to the default group for the lifetime of the editor process. Repeated apply-atlas runs (especially against different atlases) accumulate locations in the default group, can shadow same-named files from other dirs, and tie the imported atlas's lifetime to the editor session even after the imported scene nodes have been torn down inImportCleanup.Consider either (a) using a dedicated, named resource group that is created/destroyed alongside the import (so the location goes away with
destroyResourceGroup), or (b) callingremoveResourceLocationafter export to undo the registration. A dedicated group also gives you a natural scope for the temporary import.🤖 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 3308 - 3313, The atlas directory is being permanently registered into Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME via addResourceLocation in MaterialEditorQML.cpp which leaks state across imports; change the import flow to create and use a dedicated temporary resource group (e.g. generate a unique group name like "ImportAtlas_<uid>"), register the atlas path with addResourceLocation into that group instead of DEFAULT_RESOURCE_GROUP_NAME, and ensure you clean it up after the import/export by calling ResourceGroupManager::getSingleton().removeResourceLocation(...) and then destroyResourceGroup or ResourceGroupManager::getSingleton().destroyResourceGroup(...) (tie this cleanup to the same lifecycle code that currently runs ImportCleanup) so the atlas registration does not persist beyond the import.
🤖 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 87: The recognized-subcommand list in the CLI activation sentence is
incomplete; update the sentence that lists recognized subcommands (the line
containing "`info`, `fix`, `convert`, ... `optimize`") to include the missing
canonical subcommands `memory`, `analyze`, `vertex-cache`, and `decimate` so it
matches the full CLI list documented elsewhere.
- Line 176: Add documentation for the CLI/material-tool option that prevents
stripping extra texture slots: in the ApplyAtlas description (the paragraph
describing ApplyAtlas, manifestToJson, qtmesh atlas-apply and the listed options
`--match`, `--no-clamp`, `--json`) add the `--keep-extras` flag and a
one-sentence explanation: that by default extra texture slots are removed when
rebinding atlas tiles but `--keep-extras` preserves those slots (opt-out), and
mention any effect on the generated report (i.e., extras retained versus
stripped). Reference the ApplyAtlas tool name (ApplyAtlas / apply_atlas / qtmesh
atlas-apply) and the `--keep-extras`/`--no-clamp`/`--match` options so readers
see it alongside the existing flags.
In `@qml/ApplyAtlasDialog.qml`:
- Around line 37-40: The current normaliseDroppedPath(url) implementation slices
the string and leaves percent-encoding and Windows-leading-slash issues; change
it to parse the incoming URL with the URL API (new URL(url)), ensure the
protocol is "file:", take urlObj.pathname, decode it with decodeURIComponent,
and for Windows remove a leading slash when the pathname matches
/^\/[A-Za-z]:\// so "file:///C:/..." becomes "C:/..."; if the input is not a
file: URL, return the original string. Use the function name
normaliseDroppedPath to locate and replace the logic.
In `@src/ApplyAtlas.cpp`:
- Around line 98-145: The vertex buffer lock in rewriteUv0 currently calls
vbuf->lock(...) and only unlocks at the end, risking a leak if an exception
occurs; wrap the lock/unlock in an RAII guard (e.g., a small local LockGuard
that calls vbuf->unlock() in its destructor) or use a try/finally style where
vbuf->unlock() is guaranteed in all exit paths, ensure the guard references the
same vbuf used to lock and that any caught exceptions are rethrown so behavior
is unchanged.
In `@src/MaterialEditorQML.cpp`:
- Around line 3253-3255: The same-file check uses QFileInfo::canonicalFilePath()
which is empty for non-existent outputs; update the comparison to normalize and
compare absolute paths instead (replace uses of
QFileInfo(...).canonicalFilePath() for meshPath and outputPath with
QDir::cleanPath(QFileInfo(...).absoluteFilePath())), and perform a
case-insensitive comparison on Windows/macOS (e.g., using QString comparison
with Qt::CaseInsensitive or lowercasing only on those platforms); ensure you
still guard against empty normalized paths before returning the error message.
In `@src/MCPServer.cpp`:
- Around line 3744-3757: The snapshot using mgr->getEntities() is unsafe because
it can return non-Entity MovableObjects (e.g. ManualObject); replace both
before/after snapshots with a safe scene walk that iterates MovableObjects (or
scene nodes) and only records objects whose obj->getMovableType() == "Entity"
before casting to Ogre::Entity*. Update the code that builds beforeSet and
entities (referencing beforeSet, entities and mgr) to perform this type check
(use obj->getMovableType() == "Entity") so apply_atlas /
MeshImporterExporter::importer handling cannot encounter ManualObject casts.
---
Nitpick comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 3181-3232: The three near-identical functions openManifestDialog,
openMeshDialog, and saveAtlasedMeshDialog should be consolidated by adding a
single static helper (e.g., showFileDialog) that performs the shared boilerplate
(compute startDir, call QApplication::processEvents(), raise/activate
QApplication::activeWindow(), processEvents() again) and then calls
QFileDialog::getOpenFileName or QFileDialog::getSaveFileName based on a
parameter; the helper should accept parameters: title, startDir (or compute
inside if empty), filter, a boolean isSave, and an optional defaultName for save
dialogs, and must preserve the existing QFileDialog::DontUseNativeDialog |
QFileDialog::DontUseCustomDirectoryIcons flags and the nullptr selectedFilter
argument; replace the bodies of openManifestDialog, openMeshDialog, and
saveAtlasedMeshDialog with calls to this helper passing the appropriate title,
filter, isSave flag and default name (e.g., "atlased.fbx").
- Around line 3308-3313: The atlas directory is being permanently registered
into Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME via
addResourceLocation in MaterialEditorQML.cpp which leaks state across imports;
change the import flow to create and use a dedicated temporary resource group
(e.g. generate a unique group name like "ImportAtlas_<uid>"), register the atlas
path with addResourceLocation into that group instead of
DEFAULT_RESOURCE_GROUP_NAME, and ensure you clean it up after the import/export
by calling ResourceGroupManager::getSingleton().removeResourceLocation(...) and
then destroyResourceGroup or
ResourceGroupManager::getSingleton().destroyResourceGroup(...) (tie this cleanup
to the same lifecycle code that currently runs ImportCleanup) so the atlas
registration does not persist beyond the import.
🪄 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: 7ea54981-e7bf-4d7c-81cb-2eb1e54d3410
📒 Files selected for processing (17)
CLAUDE.mdqml/ApplyAtlasDialog.qmlqml/PropertiesPanel.qmlqml/qmldirsrc/ApplyAtlas.cppsrc/ApplyAtlas.hsrc/ApplyAtlas_test.cppsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/main.cppsrc/qml_resources.qrctests/CMakeLists.txt
Comment on lines
+3744
to
+3757
| QSet<Ogre::Entity*> beforeSet; | ||
| for (Ogre::Entity* e : mgr->getEntities()) beforeSet.insert(e); | ||
|
|
||
| try { | ||
| MeshImporterExporter::importer({QFileInfo(filePath).absoluteFilePath()}); | ||
| } catch (const std::exception& e) { | ||
| return makeErrorResult(QString("Importer threw: %1").arg(QString::fromUtf8(e.what()))); | ||
| } catch (...) { | ||
| return makeErrorResult("Importer threw (unknown exception type)"); | ||
| } | ||
|
|
||
| QList<Ogre::Entity*> entities; | ||
| for (Ogre::Entity* e : mgr->getEntities()) | ||
| if (!beforeSet.contains(e)) entities.append(e); |
There was a problem hiding this comment.
Replace Manager::getEntities() here with a safe scene walk.
This tool reintroduces the same ManualObject crash path the rest of this file already avoids: mgr->getEntities() can surface non-Entity attachments, so using it for the before/after snapshot makes apply_atlas unsafe in scenes with overlays or other ManualObjects.
Suggested fix
+ const auto collectEntities = [mgr]() {
+ QList<Ogre::Entity*> out;
+ for (Ogre::SceneNode* node : mgr->getSceneNodes()) {
+ if (!node) continue;
+ for (int i = 0; i < static_cast<int>(node->numAttachedObjects()); ++i) {
+ Ogre::MovableObject* obj = node->getAttachedObject(i);
+ if (!obj || obj->getMovableType() != "Entity") continue;
+ out.append(static_cast<Ogre::Entity*>(obj));
+ }
+ }
+ return out;
+ };
+
QSet<Ogre::Entity*> beforeSet;
- for (Ogre::Entity* e : mgr->getEntities()) beforeSet.insert(e);
+ for (Ogre::Entity* e : collectEntities()) beforeSet.insert(e);
...
QList<Ogre::Entity*> entities;
- for (Ogre::Entity* e : mgr->getEntities())
+ for (Ogre::Entity* e : collectEntities())
if (!beforeSet.contains(e)) entities.append(e);As per coding guidelines: Before casting MovableObject to Ogre::Entity*, check obj->getMovableType() == "Entity" to avoid crashes with ManualObjects
🤖 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 3744 - 3757, The snapshot using
mgr->getEntities() is unsafe because it can return non-Entity MovableObjects
(e.g. ManualObject); replace both before/after snapshots with a safe scene walk
that iterates MovableObjects (or scene nodes) and only records objects whose
obj->getMovableType() == "Entity" before casting to Ogre::Entity*. Update the
code that builds beforeSet and entities (referencing beforeSet, entities and
mgr) to perform this type check (use obj->getMovableType() == "Entity") so
apply_atlas / MeshImporterExporter::importer handling cannot encounter
ManualObject casts.
CodeRabbit flagged 6 actionable + 2 nitpick items. 5 are real fixes,
one was a stale-doc finding; one was wrong (Manager::getEntities
already type-checks for "Entity" — line 799 of Manager.cpp filters
non-Entity MovableObjects before push_back).
## Fixes
1. CLAUDE.md recognized-subcommand list was missing memory / analyze /
vertex-cache / decimate. Doc-only.
2. CLAUDE.md ApplyAtlas paragraph didn't mention --keep-extras; users
reading the doc had no visibility into the strip-extras behavior.
Doc-only.
3. qml/ApplyAtlasDialog.qml normaliseDroppedPath stripped "file://"
blindly — broken for Windows ("file:///C:/path" → "/C:/path") and
for percent-encoded paths from Finder (spaces, accents). Use a
proper file:// parse, decodeURIComponent, strip the leading slash
before a Windows drive letter, ensure a leading slash on POSIX.
4. ApplyAtlas::rewriteUv0 locked vbuf and only unlocked at function
end — an exception in baseVertexPointerToElement would leave the
buffer locked. Wrapped in a local LockGuard RAII so unlock runs
on every exit path.
5. Same-file guard in MaterialEditorQML::applyAtlas (and the mirror
in MCPServer::toolApplyAtlas) used QFileInfo::canonicalFilePath()
to compare input vs output — canonicalFilePath returns "" for
non-existent paths, and the output doesn't exist yet, so the
guard never fired. Compare normalized absolute paths
(QDir::cleanPath(absoluteFilePath())) with case-insensitive
compare on macOS / Windows.
6. Atlas-image directory was registered into the default resource
group without an unregister, accumulating across repeated apply-
atlas runs and risking same-name shadowing of other dirs. Wrapped
both sites (Inspector + MCP) in a LocationCleanup RAII so the
location is removed on every return path. The CLI subcommand
uses _exit() per CLAUDE.md so it has no leak risk.
## Skipped
- CodeRabbit's "non-Entity MovableObject" finding: Manager::getEntities
already filters by getMovableType() == "Entity" inside
collectEntitiesRecursive. Confirmed by reading Manager.cpp:788-806.
- The 3-file-dialog-helper dedup nitpick: cosmetic, ~40 LoC saved at
the cost of an extra static helper indirection. Skipping for now.
Verified locally:
qtmesh atlas-apply Rumba\ Dancing.fbx -o ... --manifest ... --atlas ...
→ 11/11 submeshes rewritten (regression test of the apply flow).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation