feat(#406): LLM-assisted material from description — CLI + MCP parity - #752
Conversation
The Material Editor already turns a natural-language prompt into a material via the local LLM (generateMaterialFromPrompt → LLMManager::generateMaterial). Issue #406's real gap was CLI + MCP parity; this adds it by reusing that exact path headlessly rather than building a redundant JSON-patch/PBR-preset contract. - CLIPipeline::llmDescribeMaterialToEntity: shared core. Resolves a GGUF model (--model/model override, else last-used / first available), drives LLMManager::generateMaterial synchronously through two QEventLoops (load then generate — mirrors the SD texture CLI), strips markdown fences, parses the generated Ogre material script, compiles it, honors a pbr_workflow tag (RTShaderHelper::applyPbrIfTagged), and binds it to every submesh. - CLI: `qtmesh material <file> --describe "<prompt>" [--model <name>] [-o out]` (cmdMaterialDescribe) → import, apply, re-export. - MCP: `describe_material` { prompt, mesh?, model?, output_path? } (toolDescribeMaterial) → apply to named/selected entity, optional re-export. - Both fail gracefully (exit 1 / error result, no output) with a clear "no LLM model found" message when no model is loaded or the build lacks llama.cpp. No #ifdef needed — LLMManager always compiles; only llama linking is guarded. - Sentry breadcrumb ai.assist.describe_material. - Coverage tests: nonexistent-file (1), empty-prompt (2), no-model-fails-cleanly (1, points LLM at an empty models dir so it's deterministic + can't block). - CLAUDE.md: CLI examples + architecture note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 33 minutes and 3 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds LLM-assisted material generation via a new ChangesLLM-Assisted Material Generation (CLI + MCP)
GitHub Actions macOS Cache Versioning
Sequence DiagramsequenceDiagram
actor User
participant CLI as qtmesh CLI / MCP Client
participant CLIPipeline
participant LLMManager
participant QtEventLoop
participant OgreMaterialManager
User->>CLI: material model.fbx --describe "rusty bronze" [-o out.fbx]
CLI->>CLIPipeline: cmdMaterialDescribe(inputPath, outputPath, prompt, modelName)
CLIPipeline->>CLIPipeline: import mesh, locate Ogre::Entity
CLIPipeline->>CLIPipeline: llmDescribeMaterialToEntity(entity, prompt, modelName, error)
CLIPipeline->>LLMManager: load/select GGUF model
LLMManager-->>CLIPipeline: model ready
CLIPipeline->>QtEventLoop: generate material script synchronously
QtEventLoop-->>CLIPipeline: raw script text
CLIPipeline->>CLIPipeline: strip Markdown fences, extract name via QRegularExpression
CLIPipeline->>OgreMaterialManager: remove existing, parse, compile, apply PBR tag, bind to entity
OgreMaterialManager-->>CLIPipeline: material name
CLIPipeline-->>CLI: success / material name
CLI-->>User: output mesh exported or in-session material applied
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: f38dd6b6fc
ℹ️ 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".
| #ifdef ENABLE_STABLE_DIFFUSION | ||
| #include "SDManager.h" | ||
| #include "MeshDepthRenderer.h" | ||
| #include "LLMManager.h" | ||
| #include <QEventLoop> |
There was a problem hiding this comment.
In the default CMake configuration (ENABLE_LOCAL_LLM is ON, while ENABLE_STABLE_DIFFUSION and ENABLE_ONNX are OFF), this guard skips the new LLM/QEventLoop includes even though llmDescribeMaterialToEntity() is compiled unconditionally and references LLMManager, QEventLoop, QRegularExpression, and later RTShaderHelper. A normal LLM-only/default build of CLIPipeline.cpp therefore fails to compile before the new CLI/MCP path can be used; move these LLM/Qt includes (and the RTShaderHelper include used unconditionally below) outside the unrelated feature guards or guard the implementation consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3a83a7c — moved LLMManager.h / <QEventLoop> / <QRegularExpression> (and lifted <QImage>) out of the ENABLE_STABLE_DIFFUSION guard so the always-compiled llmDescribeMaterialToEntity builds in the default LLM-only config. This was indeed the CI failure. Thanks!
There was a problem hiding this comment.
Follow-up: 79e23e7 also moves the RTShaderHelper.h include out of the ENABLE_ONNX guard — you'd specifically called this out ("the RTShaderHelper include used unconditionally below"). Windows MinGW (ONNX off) was failing with 'RTShaderHelper' has not been declared; it's a core RTSS helper with no ONNX dependency so the include is now unconditional. Audited the rest of the describe path for other leaking optional-feature symbols — none.
| llm->loadModel(chosen); | ||
| loadLoop.exec(); |
There was a problem hiding this comment.
When the caller passes an explicit missing model name, LLMManager::loadModel() emits modelLoadError synchronously before returning, so the lambda sets loadErr and calls loadLoop.quit() before loadLoop.exec() starts. The subsequent exec() then waits with no further signal, causing qtmesh material --describe --model missing.gguf and the MCP model override to hang instead of returning the intended graceful error; check loadErr/loadOk after loadModel() and only enter the loop if the load is actually pending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a53aa28. loadModel()/generateMaterial() can emit synchronously (e.g. explicit missing --model fails validation before returning), so I now set a done flag in the slots and only enter exec() if not already done, plus a safety QTimer timeout. Verified qtmesh material --describe --model missing.gguf now returns in 0s with exit 1 instead of hanging.
![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
🤖 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_cmdmaterial_coverage_test.cpp`:
- Around line 492-506: The test saves the previous models directory in prevDir
and restores it at the end, but if any assertion between the initial
setModelsDirectory call and the restoration fails, the global LLMManager state
will remain mutated for subsequent tests. Use RAII-style scope-bound cleanup to
guarantee restoration happens unconditionally even if assertions fail. Implement
a guard object that restores the directory in its destructor, or use a similar
scope-based cleanup mechanism that wraps the prevDir variable and automatically
calls llm->setModelsDirectory(prevDir) when the guard goes out of scope.
In `@src/CLIPipeline.cpp`:
- Around line 4222-4232: After the successful mesh export check in the
MeshImporterExporter::exporter call, persist the generated material definition
by serializing it using Ogre's serializer.queueForExport(mat) and
serializer.getQueuedAsString(), then write the serialized material to a sidecar
file alongside the exported mesh file. Additionally, add a breadcrumb to track
this export operation using SentryReporter::addBreadcrumb with category
'file.export' and an appropriate message describing the material export. This
ensures the referenced matName material definition is available when the
exported mesh is used.
- Around line 4122-4141: Instead of removing pre-existing materials when the
extracted matName already exists, generate a unique material name by appending a
suffix to matName (as referenced in the comment about providing a unique suffix
to avoid collisions). Rewrite the material header in the cleaned script by
replacing the original material name in the header with this unique name using
the same QRegularExpression pattern or a string replacement operation. Then
remove the mm.resourceExists check and mm.remove call at line 4140-4141 since
the uniquified name will naturally avoid any collisions with existing scene
materials.
- Around line 4074-4083: The event loop in the LLMManager model loading section
can deadlock indefinitely because signals like modelLoadCompleted and
modelLoadError may be emitted synchronously (e.g., for validation errors in
loadModel such as file not found) before the QEventLoop is even started with
loadLoop.exec(). To fix this, add a timeout to the QEventLoop using
QTimer::singleShot() or setInterval() to ensure the loop exits after a
reasonable duration even if no signal is received, and consider deferring the
llm->loadModel(chosen) call using QTimer::singleShot() with a zero delay to
ensure all signal connections are established before the actual model loading
begins, preventing the race condition where synchronous errors are emitted
before the event loop starts listening.
In `@src/MCPServer.cpp`:
- Around line 1162-1163: The SentryReporter::addBreadcrumb call on line 1163 in
MCPServer.cpp is logging the raw user-controlled material prompt which may
contain sensitive information like proprietary descriptions, emails, or secrets.
Keep the breadcrumb with the "ai.assist.describe_material" category, but replace
the arg(prompt) with safe metadata such as the prompt length or a generic
indicator (e.g., "MCP describe_material: prompt received") instead of the actual
prompt content to maintain telemetry while protecting sensitive user data.
- Around line 1173-1182: The code block calls MeshImporterExporter::exporter to
perform a file export operation but does not record a breadcrumb to track this
I/O operation. Add a SentryReporter::addBreadcrumb call with category
"file.export" and an appropriate message describing the mesh export action
(including the output path) immediately before the
MeshImporterExporter::exporter call to comply with the requirement that all
significant I/O operations be tracked with breadcrumbs.
🪄 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: be6f469d-514e-427a-89c1-e53939332e98
📒 Files selected for processing (6)
CLAUDE.mdsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_cmdmaterial_coverage_test.cppsrc/MCPServer.cppsrc/MCPServer.h
llmDescribeMaterialToEntity is compiled unconditionally, but its includes (LLMManager.h, <QEventLoop>, <QRegularExpression>) were accidentally placed inside the #ifdef ENABLE_STABLE_DIFFUSION block — so any build without SD (unit-tests-linux coverage, Windows MinGW) failed with "'LLMManager' was not declared in this scope". LLMManager always compiles (only llama linking is gated by ENABLE_LOCAL_LLM), so the include must be unconditional. Also lifted <QImage> out since turntable/isometric/upscale use it without the SD guard (previously relied on a transitive include). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…telemetry
Code review (Codex + CodeRabbit) findings on the describe-material path:
- No-hang event loops: loadModel()/generateMaterial() can emit their result
signal SYNCHRONOUSLY (e.g. an explicit missing --model fails validation before
returning) — quit() before exec() is a no-op, so the loop would block forever.
Guard exec() with a `done` flag set in the slots, and add a safety QTimer
timeout (180s load / 300s generate). Verified: `--model missing.gguf` now
returns in 0s with exit 1 instead of hanging.
- Unique material name: rewrite the `material <name>` header to
"<name>_<uuid>" before parsing instead of remove()-ing any existing resource,
so a generic LLM name ("Material") can't clobber an unrelated scene material.
Header regex anchored MultilineOption.
- Persist the generated material: write a <basename>.material sidecar next to
the exported mesh (mirrors the preset path) + file.export breadcrumbs — an
exported .mesh otherwise references a material with no definition on disk.
- Telemetry privacy: MCP describe_material breadcrumb logs the prompt LENGTH,
not the raw user prompt (may contain proprietary text / secrets). Added a
file.export breadcrumb before the optional MCP re-export.
- Test: restore LLMManager models dir via an RAII guard so a fatal assertion
can't leak mutated global state into later suites.
(The Codex P1 "LLM headers inside the SD guard" finding was already fixed in
3a83a7c.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Windows MinGW builds with ONNX (and SD) OFF failed with "'RTShaderHelper' has not been declared" — the include sat inside #ifdef ENABLE_ONNX, but the always-compiled describe-material path calls RTShaderHelper::applyPbrIfTagged unconditionally. RTShaderHelper is a core RTSS helper with no ONNX dependency, so the include belongs outside the guard. (Same class of bug as the earlier LLMManager-in-SD-guard fix; Codex's P1 had flagged the RTShaderHelper include specifically.) Audited the describe path — no other SD/ONNX-only symbols leak into the unconditional code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
build-macos failed with: make: No rule to make target '.../Xcode_16.4.app/.../MacOSX.sdk/usr/lib/libz.tbd' while the runner had upgraded to Xcode_26.5. The cached OGRE (and Assimp) SDKs bake absolute Xcode SDK paths into their CMake exports, but the macOS cache keys had no Xcode/SDK dimension — so a stale cache hit survived the runner image's Xcode bump and the consumer build linked against a now-missing libz.tbd path. Add a MACOS_CACHE_VERSION env (='xcode26') appended to all macOS assimp + ogre cache keys (producer + consumer jobs, restore-keys included) so the SDK is rebuilt against the current Xcode. Bump this value whenever the runner's Xcode changes. Not specific to #406 — this was failing every macOS build after the runner image update. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/deploy.yml:
- Around line 1637-1649: The Cache Ogre step uses the deprecated
actions/cache@v3 which should be upgraded to actions/cache@v4. Additionally,
there is an inconsistency in the restore-keys configuration across cache blocks.
In the step with id cache-ogre-macos, upgrade the uses field from
actions/cache@v3 to actions/cache@v4, and ensure the restore-keys configuration
is consistent with the Assimp cache block by removing the restore-keys section
(or making both blocks consistent) since omitting restore-keys is the correct
approach when the version bump is intended to force rebuilds.
- Around line 1594-1597: Update the cache action from actions/cache@v3 to
actions/cache@v4 to address the deprecation issue and ensure compatibility with
newer GitHub runners. Additionally, remove the restore-keys section that
contains the ineffective pattern with the trailing hyphen (the restore-keys
block ending with ${{ env.MACOS_CACHE_VERSION }}-), since the key intentionally
uses MACOS_CACHE_VERSION to force cache misses when Xcode changes, making
partial key matches unnecessary and confusing.
- Around line 1727-1737: The cache blocks in the workflow are using the
deprecated actions/cache@v3. Update the uses field in all cache action blocks
(such as the cache-ogre-macos block) from actions/cache@v3 to actions/cache@v4
to use the current version of the GitHub Actions cache action.
🪄 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: 83cd2c05-a037-429f-a794-0e9f0112d21e
📒 Files selected for processing (1)
.github/workflows/deploy.yml
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-build-${{ env.cache-name }}- | ||
| ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}- | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Upgrade to actions/cache@v4 and remove ineffective restore-keys.
Two issues here:
-
Critical:
actions/cache@v3is deprecated and may fail on newer GitHub runners. Upgrade to@v4. -
Restore-keys pattern is ineffective: The key is
...-xcode26but restore-keys is...-xcode26-. The trailing-expects keys with suffixes (e.g.,...-xcode26-abc), but nothing creates such keys. Since the purpose ofMACOS_CACHE_VERSIONis to force a cache miss when Xcode changes, restore-keys should probably be omitted entirely to avoid confusion.
🔄 Proposed fix
- name: Cache Assimp
id: cache-assimp-macos
- uses: actions/cache@v3
+ uses: actions/cache@v4
env:
cache-name: cache-assimp-macos
with:
path: |
/usr/local/lib/cmake
/usr/local/include/assimp
/usr/local/include/contrib
/usr/local/lib/pkgconfig/assimp.pc
/usr/local/lib/libassimp*
/usr/local/lib/libzlibstatic.a
- key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}
- restore-keys: |
- ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}-
+ key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}📝 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.
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} | |
| restore-keys: | | |
| ${{ runner.os }}-build-${{ env.cache-name }}- | |
| ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}- | |
| - name: Cache Assimp | |
| id: cache-assimp-macos | |
| uses: actions/cache@v4 | |
| env: | |
| cache-name: cache-assimp-macos | |
| with: | |
| path: | | |
| /usr/local/lib/cmake | |
| /usr/local/include/assimp | |
| /usr/local/include/contrib | |
| /usr/local/lib/pkgconfig/assimp.pc | |
| /usr/local/lib/libassimp* | |
| /usr/local/lib/libzlibstatic.a | |
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} |
🤖 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 @.github/workflows/deploy.yml around lines 1594 - 1597, Update the cache
action from actions/cache@v3 to actions/cache@v4 to address the deprecation
issue and ensure compatibility with newer GitHub runners. Additionally, remove
the restore-keys section that contains the ineffective pattern with the trailing
hyphen (the restore-keys block ending with ${{ env.MACOS_CACHE_VERSION }}-),
since the key intentionally uses MACOS_CACHE_VERSION to force cache misses when
Xcode changes, making partial key matches unnecessary and confusing.
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-build-${{ env.cache-name }}- | ||
| ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}- | ||
|
|
||
| - name: Cache Ogre | ||
| id: cache-ogre-macos | ||
| uses: actions/cache@v3 | ||
| env: | ||
| cache-name: cache-ogre-macos | ||
| with: | ||
| path: ${{github.workspace}}/ogre/SDK | ||
| key: ${{ runner.os }}-build-${{ env.cache-name }} | ||
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Upgrade to actions/cache@v4 and fix inconsistent restore-keys.
Same issues as the previous cache blocks:
-
Critical:
actions/cache@v3is deprecated—upgrade to@v4. -
Inconsistent restore-keys: The Assimp cache (lines 1637-1640) has the ineffective
...-xcode26-pattern, while the Ogre cache (lines 1641-1649) omits restore-keys entirely. Since the version bump is meant to force rebuilds, omitting restore-keys is correct; remove them from the Assimp cache for consistency.
🔄 Proposed fix
- name: Cache Assimp
id: cache-assimp-macos
- uses: actions/cache@v3
+ uses: actions/cache@v4
env:
cache-name: cache-assimp-macos
with:
path: |
/usr/local/lib/cmake
/usr/local/include/assimp
/usr/local/include/contrib
/usr/local/lib/pkgconfig/assimp.pc
/usr/local/lib/libassimp*
/usr/local/lib/libzlibstatic.a
key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}
- restore-keys: |
- ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}-
- name: Cache Ogre
id: cache-ogre-macos
- uses: actions/cache@v3
+ uses: actions/cache@v4
env:
cache-name: cache-ogre-macos
with:
path: ${{github.workspace}}/ogre/SDK
key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}🧰 Tools
🪛 actionlint (1.7.12)
[error] 1643-1643: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 zizmor (1.26.1)
[error] 1643-1643: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 1643-1643: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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 @.github/workflows/deploy.yml around lines 1637 - 1649, The Cache Ogre step
uses the deprecated actions/cache@v3 which should be upgraded to
actions/cache@v4. Additionally, there is an inconsistency in the restore-keys
configuration across cache blocks. In the step with id cache-ogre-macos, upgrade
the uses field from actions/cache@v3 to actions/cache@v4, and ensure the
restore-keys configuration is consistent with the Assimp cache block by removing
the restore-keys section (or making both blocks consistent) since omitting
restore-keys is the correct approach when the version bump is intended to force
rebuilds.
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} | ||
|
|
||
| - name: Cache Ogre | ||
| id: cache-ogre-macos | ||
| uses: actions/cache@v3 | ||
| env: | ||
| cache-name: cache-ogre-macos | ||
| with: | ||
| path: ${{github.workspace}}/ogre/SDK | ||
| key: ${{ runner.os }}-build-${{ env.cache-name }} | ||
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Upgrade to actions/cache@v4.
These cache blocks correctly omit restore-keys (forcing a rebuild when MACOS_CACHE_VERSION changes), but still use the deprecated actions/cache@v3.
🔄 Proposed fix
- name: Cache Assimp
id: cache-assimp-macos
- uses: actions/cache@v3
+ uses: actions/cache@v4
env:
cache-name: cache-assimp-macos
with:
path: |
/usr/local/lib/cmake
/usr/local/include/assimp
/usr/local/include/contrib
/usr/local/lib/pkgconfig/assimp.pc
/usr/local/lib/libassimp*
/usr/local/lib/libzlibstatic.a
key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}
- name: Cache Ogre
id: cache-ogre-macos
- uses: actions/cache@v3
+ uses: actions/cache@v4
env:
cache-name: cache-ogre-macos
with:
path: ${{github.workspace}}/ogre/SDK
key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }}📝 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.
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} | |
| - name: Cache Ogre | |
| id: cache-ogre-macos | |
| uses: actions/cache@v3 | |
| env: | |
| cache-name: cache-ogre-macos | |
| with: | |
| path: ${{github.workspace}}/ogre/SDK | |
| key: ${{ runner.os }}-build-${{ env.cache-name }} | |
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} | |
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} | |
| - name: Cache Ogre | |
| id: cache-ogre-macos | |
| uses: actions/cache@v4 | |
| env: | |
| cache-name: cache-ogre-macos | |
| with: | |
| path: ${{github.workspace}}/ogre/SDK | |
| key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ env.MACOS_CACHE_VERSION }} |
🧰 Tools
🪛 actionlint (1.7.12)
[error] 1731-1731: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 zizmor (1.26.1)
[error] 1731-1731: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 1731-1731: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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 @.github/workflows/deploy.yml around lines 1727 - 1737, The cache blocks in
the workflow are using the deprecated actions/cache@v3. Update the uses field in
all cache action blocks (such as the cache-ogre-macos block) from
actions/cache@v3 to actions/cache@v4 to use the current version of the GitHub
Actions cache action.
Root cause of the build-macos failure: the producer (build-n-cache-ogre-macos) defaulted to Xcode 16.4 and baked .../Xcode_16.4.app/.../libz.tbd into OGRE's CMake export, while the consumer (build-macos) defaulted to Xcode 26.5 and then failed with "No rule to make target '<16.4-SDK>/libz.tbd'". The earlier cache-version bump correctly forced an OGRE rebuild, but that rebuild STILL ran under 16.4, so the stale path persisted. Fix: add a "Pin newest stable Xcode" step to all three macOS jobs (assimp/ogre/build) that selects the highest-versioned /Applications/Xcode_*.app via `sort -V | tail -1` and exports DEVELOPER_DIR — self-healing across future runner Xcode bumps. Bump MACOS_CACHE_VERSION to xcode26b to discard the bad OGRE cache that was saved with 16.4 paths under the previous key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
1 participant
Closes #406.
Summary
The Material Editor already turns a natural-language prompt into a material via the local LLM (
generateMaterialFromPrompt→LLMManager::generateMaterial). Issue #406's real gap was CLI + MCP parity — every other AI-assist feature (#403/#404/#405) has it; this one was GUI-only. This PR adds the parity by reusing the existing LLM material-generation path headlessly, rather than building the redundant JSON-patch/PBR-preset contract the issue sketched (the engine's materials are FFP-approximated, not glTF-PBR floats, and the free-form script path already produces good materials).What's new
CLIPipeline::llmDescribeMaterialToEntity— shared core for CLI + MCP. Resolves a GGUF model (--model/modeloverride, else last-used / first available), drivesLLMManager::generateMaterialsynchronously through twoQEventLoops (model-load then generation — mirrors the SD texture CLI), strips markdown code fences, parses the generated Ogre material script (MaterialManager::parseScript),compile()s, honors apbr_workflowtag viaRTShaderHelper::applyPbrIfTagged, and binds the material to every submesh.qtmesh material <file> --describe "<prompt>" [--model <name>] [-o out](cmdMaterialDescribe) — import → apply → re-export.describe_material{ prompt, mesh?, model?, output_path? }(toolDescribeMaterial) — apply to named/selected entity in-session, optional re-export.#ifdef ENABLE_LOCAL_LLMneeded —LLMManageralways compiles; only llama linking is guarded.ai.assist.describe_material.Tests
Added to
CLIPipeline_cmdmaterial_coverage_test.cpp(same style as the #403/#404/#405 coverage):DescribeNonexistentFileReturnsError→ exit 1DescribeEmptyPromptIsUsageError→ exit 2DescribeNoModelFailsCleanly→ exit 1, no output (points the LLM at an empty models dir so it's deterministic and can't block on a load).The model-loaded generation path needs a real GGUF model, so (like the SD/PBR suites) it isn't exercised in CI.
Acceptance criteria
ai.assist.describe_material.Notes
Builds clean (
QtMeshEditor+UnitTests) on macOS arm64 with-DENABLE_LOCAL_LLM=ON. Verified the CLI no-model fallback end-to-end;describe_materialis advertised in the MCP tool list.🤖 Generated with Claude Code
Summary by CodeRabbit
describe_material)material --describe "<description>"with optional GGUF model selection, including support for exporting results and writing an adjacent.materialsidecar when available--describeexamples