fix(material): embedded-texture preview + crash on rapid texture switch + live material lists#734
fix(material): embedded-texture preview + crash on rapid texture switch + live material lists#734fernandotonon wants to merge 4 commits into
Conversation
…material list Texture preview + Material Editor texture-switching robustness: - Preview resolution (getTexturePreviewPath): resolve textures group- agnostically (FBX imports load into a per-asset resource group, which getByName's default-group lookup missed → blank preview), and decode embedded-FBX textures from EmbeddedTextureCache. Removed the convertToImage() GPU buffer-readback fallback entirely: for textures loaded via loadImage/loadRawData the GPU buffer can't be read back and convertToImage faulted with EXC_BAD_ACCESS — a hardware fault a C++ try/catch cannot intercept, which crashed the app when switching back to an embedded texture. - Memoize resolved preview paths + re-entrancy guard: the QML Image source binding evaluated the getter twice per change and reloadPreview called it again, re-running GPU/disk work 3× per switch; the preview is now driven imperatively and cached per texture name (invalidated when a generated image reuses a name). - setTextureName: rebuild the cached technique/pass/TUS pointers before use (a prior apply/recompile dangled the cached TUS → crash in TextureUnitState::retrieveTexture/Pass::getResourceGroup), then invalidate+validate the material in the RTSS scheme and recompile so the viewport stops serving the shader generated against the previous texture (stale-shader blanking + lighting glitches on repeated switches). - Inspector material lists auto-refresh: SceneTreeModel emits materialsChanged() on rebuild (model load / node+entity created) and on selection change; the submesh material dropdown and the material library list re-fetch instead of caching a stale set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 13 minutes and 50 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 adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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 (4)
📝 WalkthroughWalkthroughAdds a ChangesmaterialsChanged Signal and QML Material List Refresh
Texture Preview Memoization and Embedded Texture Support
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 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: 592f2d8ee7
ℹ️ 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".
| auto cached = m_previewPathCache.constFind(texName); | ||
| if (cached != m_previewPathCache.constEnd()) { | ||
| return cached.value(); |
There was a problem hiding this comment.
When two loaded resources share the same texture name but come from different resource groups or origins—a case this patch explicitly tries to support by scanning all groups—the first resolved preview is reused for every later material with that basename. The cache is never cleared on loadMaterial()/model changes, and embedded previews only rewrite the generated <name>_embedded.png inside computeTexturePreviewPath(), which is skipped on a cache hit, so selecting another FBX with its own diffuse.png can show or export stale pixels from the earlier asset. Key the cache by resource group/origin or clear it whenever the current material/resource context changes.
Useful? React with 👍 / 👎.
| } catch (...) { | ||
| } | ||
|
|
||
| // NOTE: we deliberately do NOT call texPtr->convertToImage() |
There was a problem hiding this comment.
For textures whose origin is a .tim file, the code above intentionally avoids returning the origin because QML cannot display TIM, but this new path now skips the previous convertToImage() PNG generation unless the texture happens to be in EmbeddedTextureCache. User-selected/imported TIM textures are loaded through PS1TIM::loadTimToOgreImage() and have no embedded-cache entry, so they now fall through to an empty or raw .tim path and the preview/Save Texture As flow regresses. Keep a safe CPU-side TIM conversion path instead of removing the fallback for all non-embedded textures.
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 (1)
src/MaterialEditorQML.cpp (1)
1098-1162: ⚡ Quick winAdd a breadcrumb for the texture rebinding operation.
This path is a user-facing texture switch plus RTSS/material regeneration, but it has no
ui.actionbreadcrumb at the operation boundary.🔎 Proposed fix
if (!name.isEmpty() && name != "*Select a texture*") { + SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), + QStringLiteral("Set material texture: %1").arg(name)); + // The on-screen mesh renders via RTSS (the viewport uses theAs per coding guidelines, “Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb() with categories: 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tools, 'file.import'/'file.export' for I/O”.
🤖 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 1098 - 1162, Add a Sentry breadcrumb at the beginning of the texture rebinding operation to track user-facing texture switches. At the start of the block where `!name.isEmpty() && name != "*Select a texture*"` is checked, call SentryReporter::addBreadcrumb() with category 'ui.action' and a descriptive message that includes the texture name being applied. This breadcrumb should be added before the call to ensureTextureInMaterialGroup() to capture the entire operation including the RTSS/material regeneration that follows through getCurrentTextureUnit(), setTextureName(), and the material compilation steps.Source: Coding guidelines
🤖 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 `@qml/SceneTreeNode.qml`:
- Around line 293-299: The Connections block with onMaterialsChanged in
SceneTreeNode causes each node instance to fetch availableMaterials()
redundantly when the signal fires, creating fan-out performance issues in large
trees. Move the material refresh logic (matFilter.allMaterials assignment and
treeModel.availableMaterials() call) from the per-node Connections to a single
parent or controller-level handler, or implement conditional logic to only
execute this fetch when an active submesh dropdown requires the materials data.
This centralizes the refresh point and eliminates redundant calls across all
tree nodes.
In `@qml/TexturePropertiesPanel.qml`:
- Around line 216-221: The texturePreview image control has its source cleared
to an empty string but its visible property may remain true, resulting in a
blank preview pane instead of showing the placeholder. When the source property
is set to empty in the texturePreview Image element, explicitly set the visible
property to false to ensure the placeholder is displayed. Alternatively, bind
the visible property to a condition that checks the image status so that
texturePreview only remains visible when it has a valid loaded image and becomes
hidden when the status is Image.Null.
In `@src/MaterialEditorQML.cpp`:
- Around line 2002-2016: The cache in the texture preview path resolution is too
broad and caches empty results. Modify the cache lookup and insertion to include
resource group information in the cache key alongside texName, so different
asset groups don't share cached previews. Additionally, check if the resolved
path from computeTexturePreviewPath() is empty before calling
m_previewPathCache.insert() to avoid caching transient misses, ensuring only
valid resolutions are cached in m_previewPathCache while allowing subsequent
calls to attempt resolution again.
- Around line 1118-1119: The issue is that updateTextureUnitList() calls
getCurrentPass() which reads from m_passMap, but if a prior compile operation
rebuilt the pass objects, m_passMap may contain stale pointers. Before calling
updateTextureUnitList() and later getCurrentTextureUnit(), first refresh or
rebuild the pass maps to ensure they contain valid pass object pointers. This
prevents dereferencing stale pointers when getCurrentPass() is invoked
internally by updateTextureUnitList().
- Around line 2074-2077: The texName variable can contain path separators or
relative path components like ".." which allows directory traversal and causes
failures when nested directories don't exist. Before using texName in the
filePath() call within the embedded PNG save operation, sanitize it by
extracting only the filename component (basename) and removing any directory
separators or dangerous path components to ensure the file is safely saved
within the intended texture_previews directory.
- Around line 1128-1134: After removing passes with tech->removePass(1) in the
while loop, the m_passMap cache may contain stale references to removed passes.
If the currently selected pass was not pass 0, the subsequent getCurrentPass()
call could return a dangling pointer. Reset the currently selected pass to pass
0 (the surviving pass) and clear or refresh the m_passMap to invalidate the
stale pass references before calling getCurrentPass() for the
setAlphaRejectSettings operation.
---
Nitpick comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 1098-1162: Add a Sentry breadcrumb at the beginning of the texture
rebinding operation to track user-facing texture switches. At the start of the
block where `!name.isEmpty() && name != "*Select a texture*"` is checked, call
SentryReporter::addBreadcrumb() with category 'ui.action' and a descriptive
message that includes the texture name being applied. This breadcrumb should be
added before the call to ensureTextureInMaterialGroup() to capture the entire
operation including the RTSS/material regeneration that follows through
getCurrentTextureUnit(), setTextureName(), and the material compilation steps.
🪄 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: 1651606c-2fa4-42c2-aa78-eb9073ce8816
📒 Files selected for processing (7)
qml/PropertiesPanel.qmlqml/SceneTreeNode.qmlqml/TexturePropertiesPanel.qmlsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/SceneTreeModel.cppsrc/SceneTreeModel.h
…bedded safety, stale pass maps CodeRabbit/Codex review fixes: - setTextureName: rebuild m_techMap via updateTechniqueList() BEFORE updateTextureUnitList() (which reads m_passMap via getCurrentPass()) so a prior recompile can't leave it dereferencing freed Pass objects. In the PS1 TMD branch, after collapsing passes reset m_selectedPassIndex to 0 and refresh the maps, then apply alpha-reject to pass 0 directly instead of via the (now stale) cached pointer. - Preview cache: key by material resource group + texture name (FBX imports reuse basenames across per-asset groups) and never cache empty/transient misses. SD generation clears the whole (group-keyed) cache. - Preview filenames: sanitize to a single safe basename (strip dirs / "..") before writing embedded/TIM previews under texture_previews. - TIM textures: restore a CPU-side PNG preview via PS1TIM::loadTimToOgreImage (the convertToImage path that previously handled this was removed because it faults on GPU-only buffers). - SceneTreeNode: stop fanning out availableMaterials() to every node on materialsChanged — refetch only for the open submesh dropdown, and fetch lazily when a dropdown is opened. - TexturePropertiesPanel: treat Image.Null (empty source) like Image.Error and show the placeholder so clearing the preview can't leave a blank pane. - setTextureName: add the ui.action Sentry breadcrumb for the texture switch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed all review feedback in 6524379:
|
…w-and-material-list
…ty gate No behavior change — addresses the SonarCloud new-code maintainability gate (cognitive complexity 71, deep nesting, init-statement smells) on PR #734: - Decompose computeTexturePreviewPath() into small file-local helpers (findLoadedTexture, timPreviewUrl, embeddedPreviewUrl, previewUrlFromOgreTexture, previewOutPath, fileUrl) — flat control flow, no >3-level nesting. - Split setTextureName()'s post-bind work into collapseTmdMaterialToSinglePass() and regenerateRtssShaders(), and flatten the method with an early return. - Catch the specific Ogre::Exception instead of catch(...), and use an if-init-statement for the preview-cache lookup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
Fixes the Material Editor texture-preview blanking, a crash when rapidly switching texture images, and stale Inspector material lists.
Preview resolution (
getTexturePreviewPath)getByName(name)only searches the default group, so valid textures previewed blank. Now falls back to a group-agnostic scan of the resource map.EmbeddedTextureCache(textures with no on-disk origin, e.g.Boss_diffuse.pnginside Rumba Dancing.fbx).convertToImage()GPU-readback fallback. For textures loaded vialoadImage/loadRawDatathe GPU buffer can't be read back andconvertToImagedereferenced a null internal buffer →EXC_BAD_ACCESS. That is a hardware fault a C++try/catchcannot intercept, so it crashed the app reliably when switching back to an embedded texture.Image.sourcebinding evaluated the getter twice per change andreloadPreview()called it again (3× per switch), each doing GPU + disk work. The preview is now driven imperatively and the resolved path is cached per texture name (invalidated when a generated image reuses a name).Apply path (
setTextureName)TextureUnitState*orphaned; the next switch dereferenced a TUS with a null parent Pass and crashed inTextureUnitState::retrieveTexture()→Pass::getResourceGroup(). Now re-captures live pointers viaupdateTextureUnitList()before use.MSN_SHADERGENscheme; rebinding the TUS texture did not update the already-generated shader, so after a few switches the surface blanked and lighting glitched. NowinvalidateMaterial+validateMaterialin the RTSS scheme andcompile()so the program regenerates against the new binding.Inspector material lists
SceneTreeModel::materialsChanged()emitted on rebuild (model load / node+entity created) and on selection change. The submesh material dropdown and material library list re-fetch instead of caching a stale set.Testing
QtMeshEditortarget,ENABLE_STABLE_DIFFUSION=ON).convertToImageandretrieveTexture/getResourceGroup) no longer fault under rapid back-and-forth texture switching, including repeatedly returning to the embedded FBX texture.Known follow-up
🤖 Generated with Claude Code
Summary by CodeRabbit