feat(materials): PBR preset templates + CLI/MCP/GUI parity (slice E) - #396
Merged
Conversation
Adds three new built-in material presets — Metallic-Roughness, Specular-Glossiness, and Unlit PBR — alongside the existing Plastic/Metal/Wood/Glass/Unlit/Wireframe set. Each PBR template creates six canonical TUS slots (albedo, normal_map, metallic, roughness, ao, emissive) and tags the pass with a "pbr_workflow" user-object binding so a future slice can swap in real PBR shading by reading the slot names + workflow tag without rebuilding the material. Slice E ships the structure; slice F will add shaders. Exposed via: - Inspector "Apply Preset" panel: new "PBR" category with M-R, S-G, Unlit chips. - CLI: `qtmesh material <file> --preset <name> [-o <output>]` and `qtmesh material --list-presets`. Writes a sidecar `.material` script next to the output mesh. - MCP: `list_material_presets` + `apply_material_preset` tools (SERVER_VERSION bumped to 1.6.0). Apply accepts an optional `mesh`/`entity` arg and briefly swaps SelectionSet to target it. Tests: 5 new gtest cases covering preset enumeration, slot creation, workflow tagging, unlit lighting state, and re-apply idempotence. 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 (12)
📝 WalkthroughWalkthroughThis PR introduces material preset functionality across the application. It adds PBR (Physically-Based Rendering) workflow support to MaterialPresetLibrary, exposes material presets via CLI and MCP server APIs, updates the QML UI with new preset options, and adds comprehensive test coverage. The changes enable users to apply built-in material presets to meshes via command-line, server tools, or the properties panel, with serialization to sidecar files. ChangesMaterial Preset System
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI/Main
participant Library as MaterialPresetLibrary
participant Mesh as Mesh/Material
participant File as File System
User->>CLI: qtmesh material model.fbx --preset "Metallic-Roughness"
CLI->>Library: presetNames()
Library-->>CLI: ["Wireframe", "Metallic-Roughness", ...]
CLI->>CLI: Validate preset exists
CLI->>Mesh: Load mesh & apply preset
Mesh->>Library: applyPreset("Metallic-Roughness", material)
Library->>Mesh: Configure PBR slots, set workflow
Mesh-->>Library: Material configured
CLI->>Mesh: Export mesh to output format
Mesh->>File: Write mesh file
CLI->>Mesh: Serialize material to sidecar
Mesh->>File: Write .material sidecar
File-->>User: ✓ Material preset applied & exported
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ccd8423c3
ℹ️ 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
+177
to
+178
| } else if (name == "Metallic-Roughness") { | ||
| applyPbrTemplate(mat, kPbrWorkflowMetallic); |
There was a problem hiding this comment.
applyPreset("Metallic-Roughness") can never reach this new PBR branch because the earlier name.startsWith("Metal") condition matches first, so the material is initialized as a regular metal preset instead of creating the six canonical PBR slots and pbr_workflow tag. This breaks the advertised PBR-template behavior across GUI/CLI/MCP for the Metallic-Roughness preset.
Useful? React with 👍 / 👎.
Comment on lines
+897
to
+901
| auto prevNodes = sel->getNodesSelectionList(); | ||
| sel->clear(); | ||
| sel->append(target->getParentSceneNode()); | ||
| lib->applyPreset(preset); | ||
| sel->clear(); |
There was a problem hiding this comment.
When apply_material_preset is called with mesh/entity, the temporary selection swap snapshots only node selections, then calls clear(), which drops previously selected entities and sub-entities permanently. After success, only nodes are restored, so callers that relied on entity/sub-entity selection state get an unintended selection reset.
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: 7
🧹 Nitpick comments (2)
src/MaterialPresetLibrary_test.cpp (2)
332-332: ⚡ Quick winReplace double-negative assertion with
ASSERT_TRUE.
ASSERT_FALSE(tag.has_value() == false)is harder to read thanASSERT_TRUE(tag.has_value())and produces a less informative gtest failure message.📝 Suggested fix
- ASSERT_FALSE(tag.has_value() == false); + ASSERT_TRUE(tag.has_value());🤖 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/MaterialPresetLibrary_test.cpp` at line 332, Replace the confusing double-negative assertion on the optional `tag` by using a direct positive assertion: change the `ASSERT_FALSE(tag.has_value() == false)` assertion to `ASSERT_TRUE(tag.has_value())` so the test reads clearly and yields a better gtest failure message; locate the assertion in the MaterialPresetLibrary_test around the `tag` variable usage and update that single assertion.
362-365: ⚡ Quick winGuard
any_castwithhas_value()for SG/Unlit tests.
SpecularGlossinessTemplateTagsWorkflowandUnlitPbrDisablesLightingButKeepsSlotscallOgre::any_cast<Ogre::String>(tag)without first assertingtag.has_value(). If the user-binding ever fails to be set (regression inapplyPbrTemplate),any_caston an emptyOgre::Anythrows and you'll see a confusing exception in the test output instead of a clear gtest failure — and the EXPECT_EQ on lines 339/366 (lighting) won't run. TheMetallicRoughnessTemplateCreatesSixCanonicalSlotstest on line 332 already does this check; bringing the SG and Unlit tests in line keeps the diagnostics consistent.📝 Suggested fix
auto tag = pass->getUserObjectBindings().getUserAny( MaterialPresetLibrary::kPbrWorkflowKey); + ASSERT_TRUE(tag.has_value()); EXPECT_EQ(Ogre::any_cast<Ogre::String>(tag), Ogre::String(MaterialPresetLibrary::kPbrWorkflowSpecular));(and the same guard before the Unlit
any_castblock)Also applies to: 387-390
🤖 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/MaterialPresetLibrary_test.cpp` around lines 362 - 365, The tests SpecularGlossinessTemplateTagsWorkflow and UnlitPbrDisablesLightingButKeepsSlots call Ogre::any_cast<Ogre::String>(tag) on the user-binding retrieved with MaterialPresetLibrary::kPbrWorkflowKey without checking that the Ogre::Any actually has a value; add the same guard used in MetallicRoughnessTemplateCreatesSixCanonicalSlots by asserting tag.has_value() (e.g., ASSERT_TRUE or EXPECT_TRUE(tag.has_value())) immediately after obtaining tag and before any_cast, so the test fails with a clear gtest assertion if the binding is missing rather than throwing an exception when casting to MaterialPresetLibrary::kPbrWorkflowSpecular.
🤖 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 2506-2531: Add explicit Sentry breadcrumbs around the file I/O
calls: before calling MeshImporterExporter::importer({fi.absoluteFilePath()})
call SentryReporter::addBreadcrumb("file.import", QString("Importing file
%1").arg(fi.absoluteFilePath())), and before calling
MeshImporterExporter::exporter(node, outFi.absoluteFilePath(), ...) call
SentryReporter::addBreadcrumb("file.export", QString("Exporting file
%1").arg(outFi.absoluteFilePath())); also add a file.export breadcrumb (or
file.import if applicable) around any sidecar write operations and the similar
import/export sequence referenced at the later location (around the code block
near lib->applyPreset / SelectionSet::getSingleton usage) so each user-facing
I/O action is tracked.
- Line 547: Update the help text for the material command to reflect the actual
parser behavior: change the string that currently reads "material <file>
--list-presets List the built-in preset names" to "material --list-presets
List the built-in preset names" (remove the "<file>" token) so it matches how
cmdMaterial parses the option; locate the help/usage string associated with
cmdMaterial in CLIPipeline.cpp and replace the literal accordingly.
- Around line 2512-2523: Manager::getSingleton()->getEntities() may include
non-Entity movables (e.g., ManualObject), so before casting and appending to
SelectionSet::getSingleton() you must guard each item: iterate the returned
collection and for each obj check obj->getMovableType() == "Entity" (or
equivalent runtime type check) before casting to Ogre::Entity* and calling
sel->append(entity); skip any non-Entity objects to avoid invalid casts/crashes.
- Around line 2539-2558: The sidecar generation silently ignores failures;
update the CLIPipeline code around matName/material export so that if the
material resource does not exist (matMgr->resourceExists(matName) is false or
matMgr->getByName(matName) returns null) or if the QFile matFile fails to open
for writing, the function returns/propagates an error instead of continuing to
report success. Specifically, check and treat missing material
(matMgr/resourceExists/getByName) and matFile.open(QIODevice::WriteOnly |
QIODevice::Text) failures as fatal for the operation that writes the .material
sidecar (the block using Ogre::MaterialSerializer ms, matText, sidecarPath, and
matFile) and return a non-success status or set the overall command result to
failure accordingly.
In `@src/MaterialPresetLibrary.h`:
- Around line 24-31: Update the comment above kPbrWorkflowKey,
kPbrWorkflowMetallic, kPbrWorkflowSpecular, and kPbrWorkflowUnlit so it no
longer calls them "TUS slot names" and instead states they are the user-binding
key and workflow identifiers; also add a short note pointing implementers to the
canonical texture-slot list held in kPbrSlots (in MaterialPresetLibrary.cpp) for
the actual slot names like albedo / normal_map / metallic / roughness / ao /
emissive.
In `@src/MCPServer.cpp`:
- Around line 896-913: Snapshot both the node and sub-entity selections before
you clear them and ensure they are restored even if lib->applyPreset(preset)
throws: capture sel->getNodesSelectionList() and
sel->getSubEntitiesSelectionList() into locals, then install a QScopeGuard (or
small RAII guard) that on scope exit clears sel and re-appends the saved nodes
and sub-entities; perform sel->clear(),
sel->append(target->getParentSceneNode()), call lib->applyPreset(preset), and
let the guard restore the original selection whether applyPreset succeeds or
throws so no selection or sub-entity state is lost.
- Around line 887-892: The loop over mgr->getEntities() is doing unsafe casting
and can crash on ManualObject entries; replace this manual search with the
existing safe helper findEntityByName to locate the Ogre::Entity by meshName. In
toolApplyMaterial (the block setting target), call findEntityByName(meshName)
(or the equivalent helper already defined) and assign its result to target
instead of iterating mgr->getEntities(), so the helper’s getMovableType() ==
"Entity" check is reused and the crash/duplication is avoided.
---
Nitpick comments:
In `@src/MaterialPresetLibrary_test.cpp`:
- Line 332: Replace the confusing double-negative assertion on the optional
`tag` by using a direct positive assertion: change the
`ASSERT_FALSE(tag.has_value() == false)` assertion to
`ASSERT_TRUE(tag.has_value())` so the test reads clearly and yields a better
gtest failure message; locate the assertion in the MaterialPresetLibrary_test
around the `tag` variable usage and update that single assertion.
- Around line 362-365: The tests SpecularGlossinessTemplateTagsWorkflow and
UnlitPbrDisablesLightingButKeepsSlots call Ogre::any_cast<Ogre::String>(tag) on
the user-binding retrieved with MaterialPresetLibrary::kPbrWorkflowKey without
checking that the Ogre::Any actually has a value; add the same guard used in
MetallicRoughnessTemplateCreatesSixCanonicalSlots by asserting tag.has_value()
(e.g., ASSERT_TRUE or EXPECT_TRUE(tag.has_value())) immediately after obtaining
tag and before any_cast, so the test fails with a clear gtest assertion if the
binding is missing rather than throwing an exception when casting to
MaterialPresetLibrary::kPbrWorkflowSpecular.
🪄 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: 70c0d126-8962-4622-8509-c14c35bea8fd
📒 Files selected for processing (10)
CLAUDE.mdqml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/MCPServer.cppsrc/MCPServer.hsrc/MaterialPresetLibrary.cppsrc/MaterialPresetLibrary.hsrc/MaterialPresetLibrary_test.cppsrc/main.cpp
Material::compile() defaults to autoManageTextureUnits=true, which lets Ogre split a pass into multiple hardware passes when the single-pass TUS count exceeds the render system's reported cap. On Linux CI (Mesa software), the cap query for the very first compile in a fresh Root sometimes returns a low value, so the 6-slot PBR pass got split, leaving getTechnique(0)->getPass(0) with 0 TUS. The PBR slot count is structural — slice F's shader will index into those exact slot names — so we want all 6 on a single pass even if the runtime can't sample them all simultaneously. Compile with autoManageTextureUnits=false to disable the splitting. Fixes the two MaterialPresetLibraryTests failures on Linux CI: - MetallicRoughnessTemplateCreatesSixCanonicalSlots - PbrTemplateReapplyIsIdempotent Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PBR template dispatch was placed at the end of the preset
if/else chain, after the legacy 'Metal' branch. But "Metallic-
Roughness".startsWith("Metal") returns true, so the M-R preset
was hitting the silver-metal Phong path and never reaching
applyPbrTemplate — leaving the pass with 0 TUS instead of the 6
canonical PBR slots.
Move the three PBR exact-name matches (Metallic-Roughness,
Specular-Glossiness, Unlit PBR) to the top of the chain. Specular-
Glossiness and Unlit PBR were unaffected because their names don't
prefix any other preset; M-R was the only collision.
Fixes Linux CI:
- MetallicRoughnessTemplateCreatesSixCanonicalSlots
- PbrTemplateReapplyIsIdempotent
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The slice E PBR slots (albedo, normal_map, metallic, roughness, ao,
emissive) shipped as metadata-only placeholders. Slice F was meant to
add the real PBR shader. In the meantime, dropping a texture into any
slot rendered as a stacked FFP layer overlapping the diffuse — visible
"effect" with no semantic meaning, confusing for users testing slots.
Wire the slots into Ogre's FFP texture chain via per-slot colour-op-ex
so each one has approximately-correct visual contribution:
albedo → MODULATE(TEXTURE, DIFFUSE) textured base colour
ao → MODULATE(TEXTURE, DIFFUSE) darkens lit base
emissive → ADD(TEXTURE, CURRENT) additive glow (visible
even with ambient=black)
metallic → ADD_SIGNED(TEXTURE, CURRENT) brightens / tints toward
metal in textured regions
roughness → MODULATE_X2(TEXTURE, CURRENT) brightens smooth
(low-roughness) regions
normal_map → marked non-FFP; routed through SRS_NORMALMAP via
RTShaderHelper::applyNormalMap when textured
AO and metallic/roughness use LBS_DIFFUSE (per-vertex from lighting +
material) instead of LBS_CURRENT for AO so the slot's effect doesn't
depend on its position in the TUS chain. Without that, AO rendered as
another texture *layer* instead of darkening the surface.
Slice F's PBR SubRenderState will replace these FFP approximations
with a real shader. The slot names + workflow user-binding tag
remain the contract.
The same wiring runs after applyMaterial() parses user-edited material
text, so PBR slots work whether they originated from a preset, the
inspector "PBR" chips, or hand-typed in the script editor. Also wired
into updateMaterialText() so per-property edits (e.g., changing the
diffuse colour) preserve PBR slot semantics.
While there: updateMaterialText() now re-wires the normal map through
RTShaderHelper::applyNormalMap after dropping RTSS techniques. Without
this, opening the Material Editor would silently drop the normal-map
effect (only re-applied on Apply). A user-visible regression we hit
while testing slice E.
Tests: added PbrSlotColourOpsApproximatePbrSemantics in
MaterialPresetLibrary_test.cpp covering the colour-op-ex on each of
the 5 FFP-wired slots. Slot semantics are forward-compatible — slice F
will swap in shader-driven sampling without changing the slot names.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Material Editor's "📝 Templates" button shipped with four legacy templates (Basic / Textured / Transparent / Normal Map). Slice E adds three PBR templates so users can drop the canonical 6-slot layout straight into the script editor and edit from there. The templates mirror the structure used by MaterialPresetLibrary's PBR presets: PBR — Metallic-Roughness 6 slots, Phong approx with shininess 40 PBR — Specular-Glossiness 6 slots, Phong approx with shininess 60 PBR — Unlit 6 slots, lighting off, white diffuse A separator divides the PBR group from the legacy templates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…selected
Loading a skinned model auto-selects the first bone in the Animation
Control panel (AnimationControlController::setSelectedSkeleton calls
selectBone(boneNames.first()) at line 266). Once a bone was selected,
TransformOperator::mousePressEvent's bone-gizmo branch fired on every
TS_TRANSLATE click — and silently returned when boneCanTranslate()
was false (rigged non-root bones, which is the common case). The
entity-translate branch below it was unreachable, so the user could
not move the imported skinned model at all.
Restructure the bone-gizmo gate so it only takes the bone path when
the action is actually doable on the bone:
TS_ROTATE / TS_SCALE → always (primary posing workflow)
TS_TRANSLATE → only when boneCanTranslate is true
(root or unrigged bone). Otherwise fall
through to the entity-translate branch.
Extract the decision into a public static helper
TransformOperator::shouldRouteToBoneGizmo so it's unit-testable
without setting up a viewport, gizmos, ray query, etc.
Tests: 5 new gtest cases in TransformOperator_test.cpp covering
no-bone-selected, rotate/scale always-route, translate respecting
boneCanTranslate (the bug-fix case), and non-transform states. The
helper treats the bone pointer as an opaque non-null marker so the
tests stay header-only without an Ogre::Bone instance.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit + Codex review feedback: CLI (src/CLIPipeline.cpp + tests): - Fix --list-presets help text: it never required <file>, the help string was wrong (CodeRabbit, line 547). - Guard entity selection against non-Entity movables: getEntities() returns the QList<Ogre::Entity*> but may include ManualObjects cast to Entity*; check getMovableType() before appending to the SelectionSet (CodeRabbit, line 2522). - Add file.import / file.export breadcrumbs around the I/O calls so observability matches other CLI subcommands (CodeRabbit, line 2531). - Fail loudly on sidecar generation errors instead of silently succeeding when the material resource is missing or the file write fails (CodeRabbit, line 2558). - Tests: 5 new error-path / list-presets tests covering missing args, unknown presets, non-existent files, and the standalone --list-presets exit (which works without Ogre init). MCP (src/MCPServer.cpp): - Use the existing safe findEntityByName helper in toolApplyMaterialPreset instead of the open-coded loop over Manager::getEntities() — same crash-on-ManualObject hazard the helper was written to avoid (CodeRabbit, line 892). - Snapshot AND restore both nodes and sub-entity selections (Codex + CodeRabbit, lines 901 / 913). Previous version snapshot only nodes and silently dropped sub-entity selections on success. - Wrap the selection swap in QScopeGuard so the original selection is restored even when applyPreset throws — previously the catch handler returned an error result without restoring, leaving the caller with an unintended single-node selection. Header doc (src/MaterialPresetLibrary.h): - The kPbrWorkflow* constants are workflow tagging keys, not TUS slot names; comment now says so and points at kPbrSlots in the .cpp for the actual canonical slot list (CodeRabbit, line 31). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
5 tasks
This was referenced Jun 21, 2026
This was referenced Jul 2, 2026
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
albedo,normal_map,metallic,roughness,ao,emissive) and tags the pass with apbr_workflowuser-object binding so a future slice can swap in real PBR shading by reading the slot names + workflow tag without rebuilding the material.Surfaces
qtmesh material <file> --preset <name> [-o <output>]+qtmesh material --list-presets. Writes a sidecar.materialnext to the output mesh so engines/tools that consume Ogre material scripts pick up the preset.list_material_presets+apply_material_presettools. SERVER_VERSION bumped to 1.6.0. The apply tool accepts an optionalmesh/entityarg and briefly swaps SelectionSet to target it.Test plan
qtmesh material --list-presetsshows all 15 presets including the 3 PBR templates.qtmesh material robot.mesh --preset \"Metallic-Roughness\" -o robot_pbr.meshwrites mesh + sidecar.material.UnitTestsbinary:PresetNamesContainsPbrTemplatesMetallicRoughnessTemplateCreatesSixCanonicalSlotsSpecularGlossinessTemplateTagsWorkflowUnlitPbrDisablesLightingButKeepsSlotsPbrTemplateReapplyIsIdempotenttryInitOgre()returns false locally without a working DISPLAY).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation