feat(ps1): TMD mesh + TIM texture import#394
Conversation
Add PlayStation TMD support across UI + CLI, including textured triangle/quad decoding, TIM decoding, and auto-loading sibling .TIM onto per-import TMD materials. Update Material Editor to browse/load TIM textures and show previews. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (13)
📝 WalkthroughWalkthroughAdds PlayStation TMD/TIM import-export and TIM texture support, integrates them into importer/exporter, asset browser, CLI, material editor (including a new MaterialEditorQML::loadTextureFile API and QML wiring), updates CMake/test wiring, and includes extensive unit tests for PS1 formats and texture handling. ChangesPlayStation TMD/TIM + Material Editor Integration
Sequence DiagramssequenceDiagram
participant User
participant Dialog as File Dialog
participant Browser as Asset Browser
participant Importer as Mesh Importer
participant PS1TMD as PS1TMD Module
participant PS1TIM as PS1TIM Module
participant Ogre as Ogre Engine
participant Scene as Scene Node
User->>Dialog: Select .tmd file
Dialog->>Browser: openFile(path)
Browser->>Importer: importer(filePath)
Importer->>PS1TMD: importTmd(filePath)
PS1TMD->>PS1TMD: Parse TMD header & objects
PS1TMD->>PS1TMD: Decode vertices, normals, UVs
PS1TMD->>Ogre: Create Mesh from soup
PS1TMD->>PS1TIM: loadTimToOgreImage(sibling.tim)
PS1TIM->>PS1TIM: Parse TIM, decode RGBA
PS1TIM->>Ogre: Load texture into material
PS1TMD->>Importer: Return MeshPtr
Importer->>Ogre: applyNormalMapsToEntity
Importer->>Scene: Create Entity & Node
Scene->>User: Display TMD mesh
sequenceDiagram
participant User
participant Editor as Material Editor
participant Dialog as File Dialog
participant PS1TIM as PS1TIM Module
participant Ogre as Ogre Engine
User->>Editor: Click Browse for texture
Editor->>Dialog: openFileDialog()
Dialog->>User: User selects .tim
Editor->>Editor: loadTextureFile(path)
Editor->>PS1TIM: loadTimToOgreImage(path)
PS1TIM->>PS1TIM: Parse TIM, decode BGR555→RGBA
PS1TIM->>PS1TIM: Embed into 256×256 canvas
PS1TIM->>Ogre: Load into Ogre::Image
Ogre->>Ogre: Register texture in resource group
Editor->>Ogre: Set material texture unit
Editor->>Editor: getTexturePreviewPath() → PNG conversion
Editor->>User: Display texture preview
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
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: 35c2eb85ab
ℹ️ 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".
| appendFt3(objects[si].prims, static_cast<uint16_t>(i0), static_cast<uint16_t>(i2), static_cast<uint16_t>(i1), | ||
| static_cast<uint16_t>(i0), tv0, tv2, tv1); |
There was a problem hiding this comment.
In PS1TMD::exportEntity, mesh indices are read as 32-bit when needed, but then immediately narrowed with static_cast<uint16_t> when emitting packets. If a submesh uses any index above 65535 (common with large meshes), the index wraps and points at the wrong vertex, silently corrupting exported geometry. The exporter should detect this and fail (or split/reindex) instead of truncating.
Useful? React with 👍 / 👎.
| if (bppMode == 1 && clutW < 256) { | ||
| // Some TIMs store multiple 16-color CLUTs for 4bpp only; 8bpp should be 256. | ||
| if (outError) *outError = "TIM 8bpp CLUT too small"; | ||
| return false; |
There was a problem hiding this comment.
The decoder treats 8bpp palettes as valid only when clutW >= 256, but TIM CLUT data is defined by total entries, so a 16x16 CLUT (256 colors) is valid and should decode. With this check, such files are rejected as "TIM 8bpp CLUT too small" even though they contain a complete palette. Validate against total color count (clutW * clutH) rather than requiring a single 256-wide row.
Useful? React with 👍 / 👎.
| if (Ogre::TextureManager::getSingleton().getByName(texName, group)) { | ||
| setTextureName(QString::fromStdString(texName)); | ||
| return true; |
There was a problem hiding this comment.
This early return makes texture selection collide on filename: if foo.tim is already loaded from one directory, selecting another foo.tim from a different directory reuses the old texture and never loads the new file. That produces incorrect material assignments whenever projects contain same-named textures in different folders. Use a unique resource name (e.g., canonical path/hash) or force reload when the source path differs.
Useful? React with 👍 / 👎.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MaterialEditorQML.cpp (2)
867-881:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't delete the pass the user is editing.
If the selected pass is not pass 0, this loop can remove the pass that owns
textureUnitimmediately aftersetTextureName(). The texture change is then lost, and the selection maps stay stale until a later reload. Collapse the technique before mutating the texture, or copy the texture onto pass 0 and reset the selection first.🤖 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 867 - 881, The code currently sets textureUnit->setTextureName(...) and then collapses technique passes which can delete the very pass owning that texture; to fix, ensure you collapse/remove extra passes before mutating the texture or else copy the texture from the current pass into pass 0 and reset the selection before removing passes. Concretely: in MaterialEditorQML.cpp use getCurrentTechnique() and determine the index of the pass that owns getCurrentTextureUnit(); if you choose to collapse the technique do that prior to calling getCurrentTextureUnit()->setTextureName(...), or if you must modify the current pass first, copy its texture name to pass 0 (via Ogre::Technique/Pass APIs) and reset selection state before calling tech->removePass(1) in the loop so you never remove the pass currently being edited.
978-982:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd uppercase
.TIMto the file filters.PS1 assets commonly use uppercase
.TIM. These name filters are case-sensitive on Linux/macOS, so valid textures will be hidden there even though the loader itself accepts.timcase-insensitively.Suggested fix
- tr("Texture File (*.bmp *.jpg *.jpeg *.gif *.raw *.png *.tga *.dds *.tim)")); + tr("Texture File (*.bmp *.jpg *.jpeg *.gif *.raw *.png *.tga *.dds *.tim *.TIM)"));- "Texture files (*.jpg *.jpeg *.png *.dds *.tga *.bmp *.tim);;All files (*)", + "Texture files (*.jpg *.jpeg *.png *.dds *.tga *.bmp *.tim *.TIM);;All files (*)",As per coding guidelines, "All code must compile and run on Windows, Linux (Ubuntu), and macOS."
Also applies to: 2541-2545
🤖 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 978 - 982, The file dialog texture filter currently lists only a lowercase ".tim" pattern which is case-sensitive on some OSes; update the filter string used in QFileDialog::getOpenFileName (the tr("Texture File (... )") argument) to include an uppercase "*.TIM" entry alongside "*.tim" and make the same change for the other texture-filter occurrences in this file where the same filter string is constructed (the other match at the second texture filter block). Ensure both places include "*.TIM" so uppercase TIM files are visible on Linux/macOS.
🧹 Nitpick comments (1)
src/MaterialEditorQML.cpp (1)
2557-2614: ⚡ Quick winAdd breadcrumbs to the new texture import flow.
loadTextureFile()is now the user-facing TIM import path, but it doesn't record either the action or the file import. That will make failed loads and wrong-texture reports much harder to reconstruct in Sentry.Suggested fix
bool MaterialEditorQML::loadTextureFile(const QString &filePath) { if (filePath.isEmpty()) return false; + + SentryReporter::addBreadcrumb("ui.action", "Material Editor load texture"); + SentryReporter::addBreadcrumb("file.import", QString("Load texture: %1").arg(filePath));As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MaterialEditorQML.cpp` around lines 2557 - 2614, In loadTextureFile(), add Sentry breadcrumbs for the user-facing texture import flow: record the user action and the file import attempts using SentryReporter::addBreadcrumb—e.g. call SentryReporter::addBreadcrumb("ui.action", "loadTextureFile invoked") (or similar descriptive message) right after entry and SentryReporter::addBreadcrumb("file.import", file.filePath().toStdString()) before attempting to addResourceLocation/load the image; also add breadcrumbs for key outcomes (e.g. TIM branch start, TIM load failure, texture already exists, and final success) referencing the function loadTextureFile, the TIM helper PS1TIM::loadTimToOgreImage, and setTextureName so Sentry has context for failures and successes.
🤖 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/AssetBrowserController.cpp`:
- Line 18: The classifier allowlist includes a multi-part extension "mesh.xml"
but code currently uses QFileInfo::suffix() so names like model.mesh.xml resolve
to "xml" and never match; update AssetBrowserController::openFile,
AssetBrowserController::fileTypeForPath, and
AssetBrowserController::refreshFiles to use QFileInfo::completeSuffix() (or
otherwise normalize multi-part extensions to lowercase) when extracting the
extension before looking up the classifier so ".mesh.xml" entries will be
recognized and filtered as mesh.
In `@src/MaterialEditorQML.cpp`:
- Around line 2578-2605: The code currently reuses a cached Ogre texture found
by Ogre::TextureManager::getSingleton().getByName(texName) where texName is
file.fileName(), causing cross-folder collisions; change the logic in
MaterialEditorQML.cpp around texName/getByName so you verify the texture's
origin via the returned resource's getOrigin() (or Resource::getOrigin()) and
compare it to file.path().toStdString() before returning; if origins differ, do
not return the cached texture — instead construct a unique resource name (for
example include file.path() or a directory hash in texName) or force loading
under a distinct name before calling addResourceLocation/initialiseResourceGroup
and loadImage, then call setTextureName with the unique name.
In `@src/MeshImporterExporter.cpp`:
- Around line 1212-1219: Remove the modal QMessageBox::warning call in the .tmd
import branch and make it mirror the other branches by logging the failure and
continuing; specifically, delete the QMessageBox::warning(nullptr,
QStringLiteral("PlayStation TMD"), ...) that executes when mesh is null and
instead emit the same log used by the .mesh/.xml branches (or use
qWarning()/OGRE_LOG) with a message including file.fileName(), then continue;
ensure no modal UI is created so CLI/headless runs are not blocked.
In `@src/PS1/PS1TIM.cpp`:
- Around line 46-66: The encoder/decoder are inconsistent: psxBgr555ToRgba
ignores the STP bit and treats only c==0 as transparent, while rgbaToPsxBgr555
sets STP when alpha<128 but leaves RGB intact; this causes opaque black and
transparent-nonblack to flip transparency on round-trip. Fix by aligning the
encoder rgbaToPsxBgr555 to zero out RGB when treating a pixel as transparent
(when computing stp set because a<128), i.e. set r5,g5,b5=0 when stp==1 so
encoded value becomes 0x8000 (or 0x0000 depending on desired convention) and
will decode consistently with psxBgr555ToRgba; alternatively, if you prefer
decoder-side change, update psxBgr555ToRgba to check bit 15 (STP) and set a=0
when that bit is set (use function psxBgr555ToRgba and rgbaToPsxBgr555 as
reference).
In `@src/PS1/PS1TMD_test.cpp`:
- Around line 490-501: The test opens a QTemporaryFile (variable tmp), writes
and flushes data, but then calls PS1TMD::importTmd(tmp.fileName(), meshName)
while the temporary file is still open which can fail on Windows; fix by calling
tmp.close() after tmp.flush() (and before calling PS1TMD::importTmd) in this
test and mirror the same close() change in the other new import tests that
follow the same open/write/flush pattern so the file handle is released before
importTmd is invoked.
In `@src/PS1/PS1TMD.cpp`:
- Around line 308-326: The bug is caused by shadowing: the outer vertex index
variables named v0/v1/v2 (uint16_t) are shadowed by inner float UV variables,
causing verts[...] to use the floats. Rename the outer index variables (uint16_t
v0/v1/v2) to i0/i1/i2 (and adjust their uses in the bounds check and the
appendTri call) so the code uses verts[i0]/verts[i1]/verts[i2] and
norms[n0]/n1/n2 as intended; update the if-condition (v0 < nVert ...) to use
i0/i1/i2 and change the appendTri arguments currently using verts[v0/v1/v2] to
verts[i0/i2/i1] to match the existing ordering.
---
Outside diff comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 867-881: The code currently sets textureUnit->setTextureName(...)
and then collapses technique passes which can delete the very pass owning that
texture; to fix, ensure you collapse/remove extra passes before mutating the
texture or else copy the texture from the current pass into pass 0 and reset the
selection before removing passes. Concretely: in MaterialEditorQML.cpp use
getCurrentTechnique() and determine the index of the pass that owns
getCurrentTextureUnit(); if you choose to collapse the technique do that prior
to calling getCurrentTextureUnit()->setTextureName(...), or if you must modify
the current pass first, copy its texture name to pass 0 (via
Ogre::Technique/Pass APIs) and reset selection state before calling
tech->removePass(1) in the loop so you never remove the pass currently being
edited.
- Around line 978-982: The file dialog texture filter currently lists only a
lowercase ".tim" pattern which is case-sensitive on some OSes; update the filter
string used in QFileDialog::getOpenFileName (the tr("Texture File (... )")
argument) to include an uppercase "*.TIM" entry alongside "*.tim" and make the
same change for the other texture-filter occurrences in this file where the same
filter string is constructed (the other match at the second texture filter
block). Ensure both places include "*.TIM" so uppercase TIM files are visible on
Linux/macOS.
---
Nitpick comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 2557-2614: In loadTextureFile(), add Sentry breadcrumbs for the
user-facing texture import flow: record the user action and the file import
attempts using SentryReporter::addBreadcrumb—e.g. call
SentryReporter::addBreadcrumb("ui.action", "loadTextureFile invoked") (or
similar descriptive message) right after entry and
SentryReporter::addBreadcrumb("file.import", file.filePath().toStdString())
before attempting to addResourceLocation/load the image; also add breadcrumbs
for key outcomes (e.g. TIM branch start, TIM load failure, texture already
exists, and final success) referencing the function loadTextureFile, the TIM
helper PS1TIM::loadTimToOgreImage, and setTextureName so Sentry has context for
failures and successes.
🪄 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: 099c5c7c-f133-4c98-9dc3-8fecaf76169f
📒 Files selected for processing (23)
qml/TexturePropertiesPanel.qmlsrc/AssetBrowserController.cppsrc/AssetBrowserController_test.cppsrc/CLIPipeline.cppsrc/CLIPipeline_test.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/Manager.cppsrc/Manager_test.cppsrc/MaterialEditorQML.cppsrc/MaterialEditorQML.hsrc/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cppsrc/PS1/CMakeLists.txtsrc/PS1/PS1TIM.cppsrc/PS1/PS1TIM.hsrc/PS1/PS1TIM_test.cppsrc/PS1/PS1TMD.cppsrc/PS1/PS1TMD.hsrc/PS1/PS1TMD_test.cppsrc/PrimitiveObject.cppsrc/WelcomeDialog.cpptests/CMakeLists.txt
| "x", "x3d", "lwo", "lws", "ac", "ms3d", "cob", "scn", "bvh", "irrmesh", "irr", | ||
| "mdl", "md2", "md3", "md5mesh", "smd", "ogex", "b3d", "q3d", "nff", "off", | ||
| "raw", "ter", "hmp", "assbin", "mesh.xml" | ||
| "raw", "ter", "hmp", "assbin", "mesh.xml", "tmd" |
There was a problem hiding this comment.
mesh.xml will never match with the current classifier.
openFile(), fileTypeForPath(), and refreshFiles() all pass QFileInfo::suffix(), so model.mesh.xml is still seen as xml and this new allowlist entry stays dead. Switch those call sites to completeSuffix() (or normalize multi-part extensions before lookup) so .mesh.xml can actually be opened and filtered as a mesh.
Suggested fix
- QString type = classifyExtension(fi.suffix().toLower());
+ QString type = classifyExtension(fi.completeSuffix().toLower());Apply the same change in:
AssetBrowserController::openFile()AssetBrowserController::fileTypeForPath()AssetBrowserController::refreshFiles()
🤖 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/AssetBrowserController.cpp` at line 18, The classifier allowlist includes
a multi-part extension "mesh.xml" but code currently uses QFileInfo::suffix() so
names like model.mesh.xml resolve to "xml" and never match; update
AssetBrowserController::openFile, AssetBrowserController::fileTypeForPath, and
AssetBrowserController::refreshFiles to use QFileInfo::completeSuffix() (or
otherwise normalize multi-part extensions to lowercase) when extracting the
extension before looking up the classifier so ".mesh.xml" entries will be
recognized and filtered as mesh.
| const std::string texName = file.fileName().toStdString(); | ||
| const std::string group = Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME; | ||
|
|
||
| try { | ||
| if (Ogre::TextureManager::getSingleton().getByName(texName, group)) { | ||
| setTextureName(QString::fromStdString(texName)); | ||
| return true; | ||
| } | ||
| } catch (...) { | ||
| } | ||
|
|
||
| try { | ||
| Ogre::ResourceGroupManager::getSingleton().addResourceLocation( | ||
| file.path().toStdString(), "FileSystem", group); | ||
| Ogre::ResourceGroupManager::getSingleton().initialiseResourceGroup(group); | ||
|
|
||
| Ogre::Image image; | ||
| if (file.suffix().compare("tim", Qt::CaseInsensitive) == 0) { | ||
| QString err; | ||
| if (!PS1TIM::loadTimToOgreImage(filePath, image, &err)) { | ||
| emit errorOccurred(QString("Failed to load TIM: %1").arg(err)); | ||
| return false; | ||
| } | ||
| } else { | ||
| image.load(texName, group); | ||
| } | ||
| Ogre::TextureManager::getSingleton().loadImage(texName, group, image); | ||
| setTextureName(QString::fromStdString(texName)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the full context of the texture loading code
cd /dev/shm/repo || exit 1
wc -l src/MaterialEditorQML.cppRepository: fernandotonon/QtMeshEditor
Length of output: 134
🏁 Script executed:
# Read the relevant section with more context
sed -n '2550,2610p' src/MaterialEditorQML.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 2467
🏁 Script executed:
# Search for other texture loading patterns in the codebase
rg "TextureManager::getSingleton" --context=3 -t cppRepository: fernandotonon/QtMeshEditor
Length of output: 13187
🏁 Script executed:
# Look for how texture names/paths are managed
rg "loadImage\|getByName" --context=2 -t cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there's any deduplication by full path
rg "texName\|texture.*name" src/MaterialEditorQML.cpp -B 2 -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
rg "getOrigin()" -B 2 -A 2 -t cppRepository: fernandotonon/QtMeshEditor
Length of output: 3801
🏁 Script executed:
# Check if there's any path-based uniqueness or hashing for texture names
rg "texName.*path\|unique.*tex\|hash.*tex" -i -t cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Look at the exact context where getOrigin is checked
rg "getOrigin\|Check origin" -B 5 -A 3 -t cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Search for comments about texture naming or resource conflicts
rg "cross.*wire\|same.*name\|basename\|resource.*conflict" -i -t cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Find the full context of getTextureFilePath to see how it handles origin checking
rg "getTextureFilePath" -B 10 -A 10 -t cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there are any TODOs, FIXMEs about texture naming conflicts
rg "TODO|FIXME|BUG|HACK" src/MaterialEditorQML.cpp -i -B 1 -A 1Repository: fernandotonon/QtMeshEditor
Length of output: 2461
🏁 Script executed:
# Look for any resource cleanup or removal that might be related
rg "ResourceGroupManager.*remove\|TextureManager.*remove" -B 2 -A 2 -t cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there's any documentation about the texture loading strategy
rg -A 5 "loadTextureFile" src/MaterialEditorQML.cpp | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 255
🏁 Script executed:
# Look at how PS1TMD.cpp loads textures and uses the names
sed -n '1,300p' src/PS1/PS1TMD.cpp | rg -B 5 -A 5 "loadImage|texName"Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there's any mechanism for unique naming or path-based texture identification
rg "ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME" -B 3 -A 3 -t cpp | head -60Repository: fernandotonon/QtMeshEditor
Length of output: 3905
🏁 Script executed:
# Look for any versioning or cleanup of old textures
rg "remove.*texture\|unload.*texture" -i -B 2 -A 2 -t cpp | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if textures are ever removed or if there's cache invalidation logic
rg "remove|unload|clear|invalidate" src/MaterialEditorQML.cpp -i | grep -i "texture\|resource" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 364
🏁 Script executed:
# Look at the complete getTextureFilePath function to see how it determines texture paths
rg -B 20 "getOrigin.*absolute path" src/MaterialEditorQML.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if similar basename-only texture loading is done elsewhere and whether it's acknowledged as a problem
rg "fileName.*toStdString\|basename.*texture" -B 2 -A 2 -t cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Search for any logic that creates unique names for textures based on path or hash
rg "hash|uuid|unique.*name|path.*hash" -i -t cpp src/MaterialEditorQML.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check how texture origin is set when loadImage is called
rg "loadImage" -B 5 -A 5 -t cpp | head -80Repository: fernandotonon/QtMeshEditor
Length of output: 3407
🏁 Script executed:
# Look at how getOrigin is used when textures are looked up
rg "getOrigin" -B 3 -A 3 -t cpp | grep -A 3 -B 3 "texture\|Texture"Repository: fernandotonon/QtMeshEditor
Length of output: 547
🏁 Script executed:
# Check if there's ever a collision or duplicate handling mechanism
rg "exists|getByName.*group" -B 2 -A 2 -t cpp | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 2227
🏁 Script executed:
# See if similar issue is documented anywhere
rg "TIM\|CAR\|import" src/MaterialEditorQML.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Search for any Ogre documentation or comments about loadImage behavior
rg "loadImage\|getByName" -B 10 -A 5 src/MaterialEditorQML.cpp | head -100Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if the existing texture lookup at line 33 has any validation we missed
sed -n '2575,2640p' src/MaterialEditorQML.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 2786
🏁 Script executed:
# Look for any other texture handling code that implements proper unique naming
rg "MaterialProcessor\|PS1TMD" -l -t cpp | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check MaterialProcessor to see if it handles this correctly
rg -B 5 -A 10 "loadTexture.*filename" src/Assimp/MaterialProcessor.cpp | head -80Repository: fernandotonon/QtMeshEditor
Length of output: 724
🏁 Script executed:
# Check git history to see if this has been addressed recently
git log --oneline src/MaterialEditorQML.cpp | head -10Repository: fernandotonon/QtMeshEditor
Length of output: 121
🏁 Script executed:
# Check if there's any version control history about texture naming fixes
git log -p --all -S "getOrigin" src/MaterialEditorQML.cpp | head -100Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Verify the current state - does the code still use basename-only texture keys?
grep -n "texName.*=.*fileName" src/MaterialEditorQML.cppRepository: fernandotonon/QtMeshEditor
Length of output: 138
🏁 Script executed:
# Check if there's any conditional logic that validates texture origin before reuse
sed -n '2590,2620p' src/MaterialEditorQML.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1226
Check texture origin before reusing by basename in resource cache.
The code uses file.fileName() (basename only) as the Ogre texture key. When getByName() finds an existing texture with that basename, it immediately returns true without verifying the source file path. Loading CAR.TIM from folder A, then loading CAR.TIM from folder B will silently reuse the first texture instead of loading the second—causing different TMD imports to bind to incorrect textures. Ogre provides getOrigin() to track resource sources; either incorporate that check before returning cached textures, or use a unique resource name per source file (e.g., including the full path or directory hash).
🤖 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 2578 - 2605, The code currently
reuses a cached Ogre texture found by
Ogre::TextureManager::getSingleton().getByName(texName) where texName is
file.fileName(), causing cross-folder collisions; change the logic in
MaterialEditorQML.cpp around texName/getByName so you verify the texture's
origin via the returned resource's getOrigin() (or Resource::getOrigin()) and
compare it to file.path().toStdString() before returning; if origins differ, do
not return the cached texture — instead construct a unique resource name (for
example include file.path() or a directory hash in texName) or force loading
under a distinct name before calling addResourceLocation/initialiseResourceGroup
and loadImage, then call setTextureName with the unique name.
| if (!mesh) { | ||
| QMessageBox::warning( | ||
| nullptr, | ||
| QStringLiteral("PlayStation TMD"), | ||
| QStringLiteral("Could not import %1 — invalid file or unsupported primitive types.") | ||
| .arg(file.fileName())); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether CLIPipeline calls MeshImporterExporter::importer (the function that now shows the QMessageBox)
rg -nP -C3 'MeshImporterExporter::importer\b|MeshImporterExporter::sceneImporter\b' --type=cpp
# And what kind of QApplication the CLI entrypoint creates
rg -nP -C3 'QCoreApplication|QGuiApplication|QApplication' src/CLIPipeline.cpp src/main.cppRepository: fernandotonon/QtMeshEditor
Length of output: 27458
🏁 Script executed:
# Check the exact context around lines 1212-1219 in MeshImporterExporter.cpp
sed -n '1205,1230p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1322
🏁 Script executed:
# Also check the other failure paths mentioned in the review (lines 1187, 1200, 1286)
echo "=== .mesh failure path around line 1187 ===" && sed -n '1180,1195p' src/MeshImporterExporter.cpp
echo "=== .xml failure path around line 1200 ===" && sed -n '1193,1210p' src/MeshImporterExporter.cpp
echo "=== Assimp failure path around line 1286 ===" && sed -n '1280,1295p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3303
🏁 Script executed:
# Check if there are any TMD import tests that might show how the CLI/headless code path works
rg -n 'tmd|TMD' src/MeshImporterExporter_test.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 568
🏁 Script executed:
# Verify what happens when QMessageBox is used in a CLI context without user interaction
# Check if there are any guards or conditions around QMessageBox usage elsewhere
rg -n 'QMessageBox' src/MeshImporterExporter.cpp src/CLIPipeline.cpp src/mainwindow.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2193
🏁 Script executed:
# Check the context around line 1356 - the export QMessageBox
sed -n '1350,1365p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 729
🏁 Script executed:
# Check if CLIPipeline's QApplication is created before or after MeshImporterExporter::importer calls
grep -n 'new QApplication\|QApplication a\|MeshImporterExporter::importer' src/CLIPipeline.cpp | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 726
🏁 Script executed:
# Get the full context for the .xml path failure to understand what return does
sed -n '1200,1210p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 683
🏁 Script executed:
# Check the function signature to see if return in the middle of the loop exits the whole function
sed -n '1162,1175p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 616
🏁 Script executed:
# Check if there's a test that exercises the .xml failure path to see expected behavior
rg -n 'mesh.xml|importOgreXmlMesh' src/MeshImporterExporter_test.cpp -A 3 -B 3 | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 1530
🏁 Script executed:
# Check if the exporter function is meant to be GUI-only or can be called from CLI
rg -n 'MeshImporterExporter::exporter\b' src/*.cppRepository: fernandotonon/QtMeshEditor
Length of output: 4249
Remove QMessageBox — inconsistent with other import branches and problematic for CLI automation.
The other branches in this loop (.mesh line 1187, .xml line 1200) handle import failure silently with continue; / return; and let Ogre logging surface the error. The new .tmd branch breaks this pattern with a modal QMessageBox::warning(nullptr, ...), which creates two problems:
- Inconsistent error handling. Per-format failures in this function are currently log-only; the GUI surfaces them via the existing toast/logging pipeline, not modal dialogs. This is the only exception.
- CLI blocking. While CLIPipeline does create a
QApplicationbefore callingimporter(), a modal dialog in the CLI pipeline still blocks execution waiting for user interaction that won't come in automated/headless scenarios.
Recommend logging the failure and letting the existing presentation layer handle it, matching the pattern used by other format branches.
🛡️ Suggested replacement
const std::string meshName = (file.baseName() + QStringLiteral("_tmd")).toStdString();
Ogre::MeshPtr mesh = PS1TMD::importTmd(file.filePath(), meshName);
if (!mesh) {
- QMessageBox::warning(
- nullptr,
- QStringLiteral("PlayStation TMD"),
- QStringLiteral("Could not import %1 — invalid file or unsupported primitive types.")
- .arg(file.fileName()));
+ Ogre::LogManager::getSingleton().logMessage(
+ QStringLiteral("PS1TMD: Could not import %1 — invalid file or unsupported primitive types.")
+ .arg(file.fileName()).toStdString(),
+ Ogre::LML_WARNING);
continue;
}📝 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.
| if (!mesh) { | |
| QMessageBox::warning( | |
| nullptr, | |
| QStringLiteral("PlayStation TMD"), | |
| QStringLiteral("Could not import %1 — invalid file or unsupported primitive types.") | |
| .arg(file.fileName())); | |
| continue; | |
| } | |
| if (!mesh) { | |
| Ogre::LogManager::getSingleton().logMessage( | |
| QStringLiteral("PS1TMD: Could not import %1 — invalid file or unsupported primitive types.") | |
| .arg(file.fileName()).toStdString(), | |
| Ogre::LML_WARNING); | |
| continue; | |
| } |
🤖 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/MeshImporterExporter.cpp` around lines 1212 - 1219, Remove the modal
QMessageBox::warning call in the .tmd import branch and make it mirror the other
branches by logging the failure and continuing; specifically, delete the
QMessageBox::warning(nullptr, QStringLiteral("PlayStation TMD"), ...) that
executes when mesh is null and instead emit the same log used by the .mesh/.xml
branches (or use qWarning()/OGRE_LOG) with a message including file.fileName(),
then continue; ensure no modal UI is created so CLI/headless runs are not
blocked.
| static void psxBgr555ToRgba(uint16_t c, uint8_t& r, uint8_t& g, uint8_t& b, uint8_t& a) | ||
| { | ||
| // Bits: 0..4 R, 5..9 G, 10..14 B, 15 STP (semi-transparency flag in GPU) | ||
| const uint8_t rr = uint8_t(c & 0x1F); | ||
| const uint8_t gg = uint8_t((c >> 5) & 0x1F); | ||
| const uint8_t bb = uint8_t((c >> 10) & 0x1F); | ||
| r = uint8_t((rr * 255 + 15) / 31); | ||
| g = uint8_t((gg * 255 + 15) / 31); | ||
| b = uint8_t((bb * 255 + 15) / 31); | ||
| // Convention: 0 is transparent in many TIMs; otherwise opaque. | ||
| a = (c == 0) ? 0 : 255; | ||
| } | ||
|
|
||
| static uint16_t rgbaToPsxBgr555(uint8_t r, uint8_t g, uint8_t b, uint8_t a) | ||
| { | ||
| const uint16_t r5 = uint16_t(r >> 3); | ||
| const uint16_t g5 = uint16_t(g >> 3); | ||
| const uint16_t b5 = uint16_t(b >> 3); | ||
| const uint16_t stp = (a < 128) ? 1u : 0u; | ||
| return uint16_t((stp << 15) | (b5 << 10) | (g5 << 5) | (r5 << 0)); | ||
| } |
There was a problem hiding this comment.
Asymmetric STP/alpha handling can lose pixel data on round-trip.
Encode at line 64 sets STP from alpha < 128 (and preserves the original RGB), but decode at line 56 ignores STP entirely and only treats c == 0 as transparent. Consequences:
- Opaque black
(0,0,0,255)encodes to0x0000, then decodes back as alpha=0 (transparent black) — the original opacity is lost. - A transparent non-black pixel
(255,0,0,0)encodes to0x801F, then decodes back as alpha=255 (opaque red) — the original transparency is lost.
For TIMs authored externally this is a "lossy format" reality, but the encoder you control can be aligned with this decoder by also zeroing the color when alpha is below the threshold (or, alternatively, having the decoder consider the STP bit). Keeping encode/decode consistent will make SaveThenLoadTim16RoundTrip robust against future test inputs that include opaque-black or transparent-non-black pixels.
🔧 One possible alignment (encoder zeroes color when treating pixel as transparent)
static uint16_t rgbaToPsxBgr555(uint8_t r, uint8_t g, uint8_t b, uint8_t a)
{
+ // Decoder uses c == 0 as the transparent signal, so emit 0x0000 for
+ // transparent pixels to keep round-trip consistent.
+ if (a < 128) return 0u;
const uint16_t r5 = uint16_t(r >> 3);
const uint16_t g5 = uint16_t(g >> 3);
const uint16_t b5 = uint16_t(b >> 3);
- const uint16_t stp = (a < 128) ? 1u : 0u;
- return uint16_t((stp << 15) | (b5 << 10) | (g5 << 5) | (r5 << 0));
+ return uint16_t((b5 << 10) | (g5 << 5) | (r5 << 0));
}🤖 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/PS1/PS1TIM.cpp` around lines 46 - 66, The encoder/decoder are
inconsistent: psxBgr555ToRgba ignores the STP bit and treats only c==0 as
transparent, while rgbaToPsxBgr555 sets STP when alpha<128 but leaves RGB
intact; this causes opaque black and transparent-nonblack to flip transparency
on round-trip. Fix by aligning the encoder rgbaToPsxBgr555 to zero out RGB when
treating a pixel as transparent (when computing stp set because a<128), i.e. set
r5,g5,b5=0 when stp==1 so encoded value becomes 0x8000 (or 0x0000 depending on
desired convention) and will decode consistently with psxBgr555ToRgba;
alternatively, if you prefer decoder-side change, update psxBgr555ToRgba to
check bit 15 (STP) and set a=0 when that bit is set (use function
psxBgr555ToRgba and rgbaToPsxBgr555 as reference).
| QTemporaryFile tmp(QDir::tempPath() + "/qtmesh_ps1tmd_25_XXXXXX.tmd"); | ||
| tmp.setAutoRemove(true); | ||
| ASSERT_TRUE(tmp.open()); | ||
| const QByteArray blob = makeMinimal25NoLightTmd(); | ||
| ASSERT_EQ(tmp.write(blob), blob.size()); | ||
| tmp.flush(); | ||
|
|
||
| const std::string meshName = "PS1Tmd25Mesh"; | ||
| if (auto old = Ogre::MeshManager::getSingleton().getByName(meshName)) | ||
| Ogre::MeshManager::getSingleton().remove(old); | ||
|
|
||
| Ogre::MeshPtr mesh = PS1TMD::importTmd(tmp.fileName(), meshName); |
There was a problem hiding this comment.
Close the temporary file before calling importTmd().
These new tests pass an already-open QTemporaryFile to PS1TMD::importTmd(). That tends to work on Unix, but it can fail on Windows because the second open is denied while the writer handle is still live. Please close() after flush() before importing, and mirror that fix in the other new import tests that use the same pattern.
Suggested fix
ASSERT_EQ(tmp.write(blob), blob.size());
tmp.flush();
+ tmp.close();
const std::string meshName = "PS1Tmd25Mesh";As per coding guidelines, "All code must compile and run on Windows, Linux (Ubuntu), and macOS."
📝 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.
| QTemporaryFile tmp(QDir::tempPath() + "/qtmesh_ps1tmd_25_XXXXXX.tmd"); | |
| tmp.setAutoRemove(true); | |
| ASSERT_TRUE(tmp.open()); | |
| const QByteArray blob = makeMinimal25NoLightTmd(); | |
| ASSERT_EQ(tmp.write(blob), blob.size()); | |
| tmp.flush(); | |
| const std::string meshName = "PS1Tmd25Mesh"; | |
| if (auto old = Ogre::MeshManager::getSingleton().getByName(meshName)) | |
| Ogre::MeshManager::getSingleton().remove(old); | |
| Ogre::MeshPtr mesh = PS1TMD::importTmd(tmp.fileName(), meshName); | |
| QTemporaryFile tmp(QDir::tempPath() + "/qtmesh_ps1tmd_25_XXXXXX.tmd"); | |
| tmp.setAutoRemove(true); | |
| ASSERT_TRUE(tmp.open()); | |
| const QByteArray blob = makeMinimal25NoLightTmd(); | |
| ASSERT_EQ(tmp.write(blob), blob.size()); | |
| tmp.flush(); | |
| tmp.close(); | |
| const std::string meshName = "PS1Tmd25Mesh"; | |
| if (auto old = Ogre::MeshManager::getSingleton().getByName(meshName)) | |
| Ogre::MeshManager::getSingleton().remove(old); | |
| Ogre::MeshPtr mesh = PS1TMD::importTmd(tmp.fileName(), meshName); |
🤖 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/PS1/PS1TMD_test.cpp` around lines 490 - 501, The test opens a
QTemporaryFile (variable tmp), writes and flushes data, but then calls
PS1TMD::importTmd(tmp.fileName(), meshName) while the temporary file is still
open which can fail on Windows; fix by calling tmp.close() after tmp.flush()
(and before calling PS1TMD::importTmd) in this test and mirror the same close()
change in the other new import tests that follow the same open/write/flush
pattern so the file handle is released before importTmd is invoked.
| if (mode == 0x34 && flag == 0 && ilen == 6) { | ||
| const uint16_t n0 = readU16le(d + 12); | ||
| const uint16_t v0 = readU16le(d + 14); | ||
| const uint16_t n1 = readU16le(d + 16); | ||
| const uint16_t v1 = readU16le(d + 18); | ||
| const uint16_t n2 = readU16le(d + 20); | ||
| const uint16_t v2 = readU16le(d + 22); | ||
| if (v0 < nVert && v1 < nVert && v2 < nVert && n0 < nNorm && n1 < nNorm && n2 < nNorm) { | ||
| const float u0 = decodePs1TexU(d[0]); | ||
| const float v0 = decodePs1TexV(d[1]); | ||
| const float u1 = decodePs1TexU(d[4]); | ||
| const float v1 = decodePs1TexV(d[5]); | ||
| const float u2 = decodePs1TexU(d[8]); | ||
| const float v2 = decodePs1TexV(d[9]); | ||
| appendTri(out, verts[v0], verts[v2], verts[v1], norms[n0], norms[n2], norms[n1], Ogre::Vector2(u0, v0), | ||
| Ogre::Vector2(u2, v2), Ogre::Vector2(u1, v1), true); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Critical: variable shadowing silently corrupts Mode 0x34 (lit Gouraud-textured triangle) geometry.
Inside the inner if (v0 < nVert && ...) block, the const float v0/v1/v2 UV declarations shadow the outer uint16_t v0/v1/v2 vertex indices. verts[v0], verts[v1], verts[v2] therefore index using the float V-coordinates (typically 0..1), which truncates to 0 — every Mode 0x34 triangle collapses onto verts[0].
The other textured-triangle handlers in this file (0x24, 0x25, 0x35, 0x3d, 0x3c) deliberately use i0/i1/i2 or v0i/v1i/v2i to avoid exactly this collision; Mode 0x34 was missed. Renaming the outer indices restores correctness.
🐛 Proposed fix — rename indices to i0/i1/i2 like the sibling handlers
if (mode == 0x34 && flag == 0 && ilen == 6) {
const uint16_t n0 = readU16le(d + 12);
- const uint16_t v0 = readU16le(d + 14);
+ const uint16_t i0 = readU16le(d + 14);
const uint16_t n1 = readU16le(d + 16);
- const uint16_t v1 = readU16le(d + 18);
+ const uint16_t i1 = readU16le(d + 18);
const uint16_t n2 = readU16le(d + 20);
- const uint16_t v2 = readU16le(d + 22);
- if (v0 < nVert && v1 < nVert && v2 < nVert && n0 < nNorm && n1 < nNorm && n2 < nNorm) {
+ const uint16_t i2 = readU16le(d + 22);
+ if (i0 < nVert && i1 < nVert && i2 < nVert && n0 < nNorm && n1 < nNorm && n2 < nNorm) {
const float u0 = decodePs1TexU(d[0]);
const float v0 = decodePs1TexV(d[1]);
const float u1 = decodePs1TexU(d[4]);
const float v1 = decodePs1TexV(d[5]);
const float u2 = decodePs1TexU(d[8]);
const float v2 = decodePs1TexV(d[9]);
- appendTri(out, verts[v0], verts[v2], verts[v1], norms[n0], norms[n2], norms[n1], Ogre::Vector2(u0, v0),
+ appendTri(out, verts[i0], verts[i2], verts[i1], norms[n0], norms[n2], norms[n1], Ogre::Vector2(u0, v0),
Ogre::Vector2(u2, v2), Ogre::Vector2(u1, v1), true);
}
continue;
}Consider also enabling -Wshadow (or at least -Wshadow=local) in the project's warning flags so this class of bug is flagged at compile time going forward.
🤖 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/PS1/PS1TMD.cpp` around lines 308 - 326, The bug is caused by shadowing:
the outer vertex index variables named v0/v1/v2 (uint16_t) are shadowed by inner
float UV variables, causing verts[...] to use the floats. Rename the outer index
variables (uint16_t v0/v1/v2) to i0/i1/i2 (and adjust their uses in the bounds
check and the appendTri call) so the code uses verts[i0]/verts[i1]/verts[i2] and
norms[n0]/n1/n2 as intended; update the if-condition (v0 < nVert ...) to use
i0/i1/i2 and change the appendTri arguments currently using verts[v0/v1/v2] to
verts[i0/i2/i1] to match the existing ordering.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MaterialEditorQML.cpp (1)
979-1028:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd breadcrumbs for texture selection/import actions.
These are user-facing + file I/O operations, but no breadcrumb is recorded. Please add
SentryReporter::addBreadcrumb(...)for both action intent and import operation.As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations."
Also applies to: 2560-2617
🤖 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 979 - 1028, The selectTexture flow in MaterialEditorQML::selectTexture lacks Sentry breadcrumbs for the user action and the file import steps; add SentryReporter::addBreadcrumb("ui.action", "selectTexture dialog opened") immediately when the function begins (before showing QFileDialog) to record intent, and add SentryReporter::addBreadcrumb("file.import", QString("import texture: %1").arg(file.fileName())) just before/after each import operation: when adding the resource location / initialising the resource group, before calling PS1TIM::loadTimToOgreImage (include the TIM path) and before calling image.load()/Ogre::TextureManager::getSingleton().loadImage (include filePath/fileName); keep messages concise and use the exact symbols MaterialEditorQML::selectTexture, PS1TIM::loadTimToOgreImage, and setTextureName to help locate placement.
♻️ Duplicate comments (2)
src/MaterialEditorQML.cpp (1)
1001-1004:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix cached texture lookup flow and basename collision reuse.
At Line 1001-1004 and Line 2585-2588, cache lookup is treated as exception-driven and keyed only by basename. This can skip loading on cache miss and can also reuse the wrong texture when two folders contain the same filename.
Suggested fix
- try { - if (Ogre::TextureManager::getSingleton().getByName(texName, group)) { - setTextureName(QString::fromStdString(texName)); - return true; - } - } catch (...) { - } + if (auto existing = Ogre::TextureManager::getSingleton().getByName(texName, group)) { + const QString origin = QString::fromStdString(existing->getOrigin()); + if (QFileInfo(origin).absoluteFilePath() == file.absoluteFilePath()) { + setTextureName(QString::fromStdString(texName)); + return true; + } + } + + const QString uniqueTexName = QString("%1__%2") + .arg(file.fileName(), + QString::number(qHash(file.absoluteFilePath()), 16));In Ogre 14.x, does TextureManager::getByName(name, group) return a null resource on miss (instead of throwing), and is Texture::getOrigin intended for source-path validation after loadImage?Also applies to: 2581-2588
🤖 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 1001 - 1004, The texture cache lookup currently uses an exception-driven flow and only the basename as key, causing misses to be skipped and collisions when different folders share filenames; replace the try/catch around Ogre::TextureManager::getSingleton().getByName(...) with a direct call that checks the returned Ogre::TexturePtr for null (getByName returns null on miss), use the file's full absolute path (or another unique per-source key) instead of just file.fileName() to key the cache, and after loading/obtaining a texture validate the source by comparing Texture::getOrigin() (or equivalent) against the expected source path before reusing it; if the pointer is null or origin mismatches, proceed to loadImage/create the texture with a unique name derived from the full path.src/PS1/PS1TMD.cpp (1)
308-326:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMode 0x34 shadowing bug is still present.
The outer
uint16_t v0/v1/v2declared at lines 310/312/314 are shadowed by the innerconst float v0/v1/v2UV declarations at lines 317/319/321, so theverts[v0]/verts[v1]/verts[v2]accesses at line 322 index using V-coordinates (≈0..1, truncated to 0). Every Mode 0x34 (TMD_G_3T, lit Gouraud‑textured triangle) collapses ontoverts[0], exactly as flagged in the prior review. The sibling handlers (0x24,0x25,0x35,0x3c,0x3d) already usei0/i1/i2/v0i/v1i/v2ito avoid the collision; only Mode 0x34 was missed.🐛 Proposed fix — rename outer indices to
i0/i1/i2if (mode == 0x34 && flag == 0 && ilen == 6) { const uint16_t n0 = readU16le(d + 12); - const uint16_t v0 = readU16le(d + 14); + const uint16_t i0 = readU16le(d + 14); const uint16_t n1 = readU16le(d + 16); - const uint16_t v1 = readU16le(d + 18); + const uint16_t i1 = readU16le(d + 18); const uint16_t n2 = readU16le(d + 20); - const uint16_t v2 = readU16le(d + 22); - if (v0 < nVert && v1 < nVert && v2 < nVert && n0 < nNorm && n1 < nNorm && n2 < nNorm) { + const uint16_t i2 = readU16le(d + 22); + if (i0 < nVert && i1 < nVert && i2 < nVert && n0 < nNorm && n1 < nNorm && n2 < nNorm) { const float u0 = decodePs1TexU(d[0]); const float v0 = decodePs1TexV(d[1]); const float u1 = decodePs1TexU(d[4]); const float v1 = decodePs1TexV(d[5]); const float u2 = decodePs1TexU(d[8]); const float v2 = decodePs1TexV(d[9]); - appendTri(out, verts[v0], verts[v2], verts[v1], norms[n0], norms[n2], norms[n1], Ogre::Vector2(u0, v0), + appendTri(out, verts[i0], verts[i2], verts[i1], norms[n0], norms[n2], norms[n1], Ogre::Vector2(u0, v0), Ogre::Vector2(u2, v2), Ogre::Vector2(u1, v1), true); } continue; }🤖 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/PS1/PS1TMD.cpp` around lines 308 - 326, The code in the Mode 0x34 block declares outer vertex indices uint16_t v0/v1/v2 which are shadowed by inner float v0/v1/v2 UVs; rename the outer vertex index variables (e.g. v0/v1/v2 → i0/i1/i2) where they are read (readU16le assignments) and used (bounds checks and the appendTri call) so verts[...] indexes use i0/i1/i2 while the inner float UV variables can remain v0/v1/v2; update all references to the renamed symbols (n0, i0, n1, i1, n2, i2 and their uses in the bounds check and appendTri) to eliminate the shadowing bug.
🧹 Nitpick comments (1)
src/PS1/PS1TMD.cpp (1)
1041-1071: ⚖️ Poor tradeoffOptional: factor the buffer-aliasing dance into a helper.
The four-way alias check between
posSrc / nrmSrc / uvSrc / colSrcto avoid double-locking the sameHardwareVertexBufferworks, but the unlock side at line 1153 mirrors the lock-side conditions verbatim and is genuinely hard to verify by eye (notably the nested!(uvSrc == nrmSrc && nrmSrc != posSrc)clause appears twice). A smalllockUniqueSource(source, baseOut)helper that memoizessource → base*in a tinystd::array<{src, base*}, 4>would let lock and unlock share one source-of-truth and remove the cascadingelse ifchain.Not blocking — current logic is correct as far as I can tell — flagging only because future additions (tangents, second UV set, bone indices) will compound the combinatorics.
Also applies to: 1153-1156
🤖 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/PS1/PS1TMD.cpp` around lines 1041 - 1071, Factor the repeated buffer-aliasing logic into a small helper (e.g., lockUniqueSource or mapBufferBase) that accepts a source identifier (posSrc/nrmSrc/uvSrc/colSrc) and returns/memoizes the corresponding base pointer (posBase/nrmBase/uvBase/colBase) and whether the buffer was actually locked; replace the current cascading else-if chains that set nrmBase/uvBase/colBase with calls to this helper and use the same helper/memo to drive the unlock path so lock/unlock share one canonical source→base mapping and you avoid duplicate complex conditions like !(uvSrc == nrmSrc && nrmSrc != posSrc).
🤖 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/MaterialEditorQML.cpp`:
- Around line 876-883: The code removes passes from the current technique using
getCurrentTechnique() and then immediately calls getCurrentPass(), which can
return a stale/invalid selection if the previously selected pass was removed;
update the logic in the block that calls Ogre::Technique::removePass(1) so that
after pruning passes you refresh the selected pass before using it (e.g.,
reselect a valid pass index or call getCurrentTechnique()->getPass(0) or
otherwise reset whatever selection state getCurrentPass() relies on), then only
call pass->setAlphaRejectSettings(...) on that freshly validated pass; ensure
you reference getCurrentTechnique, removePass, getCurrentPass and
setAlphaRejectSettings when making the change.
In `@src/PS1/PS1TMD.cpp`:
- Around line 1036-1149: The code silently truncates 32-bit indices to uint16
when building TMD primitives: before resizing objects[si].verts/norms and
locking buffers (i.e., where vCount is read), add a guard that checks if vCount
> 65535 and fail fast (log/error/throw or skip this submesh) so we never produce
primitives with truncated indices; references: vCount, objects[si].verts/norms
resize, the index reading that assigns i0/i1/i2, and the primitive writers
appendFt3 / appendG3 / appendG3C — ensure the guard runs before any per-submesh
blob allocation or writing.
---
Outside diff comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 979-1028: The selectTexture flow in
MaterialEditorQML::selectTexture lacks Sentry breadcrumbs for the user action
and the file import steps; add SentryReporter::addBreadcrumb("ui.action",
"selectTexture dialog opened") immediately when the function begins (before
showing QFileDialog) to record intent, and add
SentryReporter::addBreadcrumb("file.import", QString("import texture:
%1").arg(file.fileName())) just before/after each import operation: when adding
the resource location / initialising the resource group, before calling
PS1TIM::loadTimToOgreImage (include the TIM path) and before calling
image.load()/Ogre::TextureManager::getSingleton().loadImage (include
filePath/fileName); keep messages concise and use the exact symbols
MaterialEditorQML::selectTexture, PS1TIM::loadTimToOgreImage, and setTextureName
to help locate placement.
---
Duplicate comments:
In `@src/MaterialEditorQML.cpp`:
- Around line 1001-1004: The texture cache lookup currently uses an
exception-driven flow and only the basename as key, causing misses to be skipped
and collisions when different folders share filenames; replace the try/catch
around Ogre::TextureManager::getSingleton().getByName(...) with a direct call
that checks the returned Ogre::TexturePtr for null (getByName returns null on
miss), use the file's full absolute path (or another unique per-source key)
instead of just file.fileName() to key the cache, and after loading/obtaining a
texture validate the source by comparing Texture::getOrigin() (or equivalent)
against the expected source path before reusing it; if the pointer is null or
origin mismatches, proceed to loadImage/create the texture with a unique name
derived from the full path.
In `@src/PS1/PS1TMD.cpp`:
- Around line 308-326: The code in the Mode 0x34 block declares outer vertex
indices uint16_t v0/v1/v2 which are shadowed by inner float v0/v1/v2 UVs; rename
the outer vertex index variables (e.g. v0/v1/v2 → i0/i1/i2) where they are read
(readU16le assignments) and used (bounds checks and the appendTri call) so
verts[...] indexes use i0/i1/i2 while the inner float UV variables can remain
v0/v1/v2; update all references to the renamed symbols (n0, i0, n1, i1, n2, i2
and their uses in the bounds check and appendTri) to eliminate the shadowing
bug.
---
Nitpick comments:
In `@src/PS1/PS1TMD.cpp`:
- Around line 1041-1071: Factor the repeated buffer-aliasing logic into a small
helper (e.g., lockUniqueSource or mapBufferBase) that accepts a source
identifier (posSrc/nrmSrc/uvSrc/colSrc) and returns/memoizes the corresponding
base pointer (posBase/nrmBase/uvBase/colBase) and whether the buffer was
actually locked; replace the current cascading else-if chains that set
nrmBase/uvBase/colBase with calls to this helper and use the same helper/memo to
drive the unlock path so lock/unlock share one canonical source→base mapping and
you avoid duplicate complex conditions like !(uvSrc == nrmSrc && nrmSrc !=
posSrc).
🪄 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: d8a5b63e-dd70-4ea5-b3ee-3cdad6ec6575
📒 Files selected for processing (2)
src/MaterialEditorQML.cppsrc/PS1/PS1TMD.cpp
| const uint32_t vCount = vd->vertexCount; | ||
| objects[si].verts.resize(size_t(vCount) * 8u, 0); | ||
| objects[si].norms.resize(size_t(vCount) * 8u, 0); | ||
|
|
||
| // Lock each unique source buffer only once (pos/nrm often share a buffer). | ||
| const uint8_t* posBase = static_cast<const uint8_t*>(posBuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| const uint8_t* nrmBase = nullptr; | ||
| const uint8_t* uvBase = nullptr; | ||
| const uint8_t* colBase = nullptr; | ||
|
|
||
| if (nrmSrc == posSrc) { | ||
| nrmBase = posBase; | ||
| } else { | ||
| nrmBase = static_cast<const uint8_t*>(nrmBuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| } | ||
|
|
||
| if (uvBuf) { | ||
| if (uvSrc == posSrc) { | ||
| uvBase = posBase; | ||
| } else if (uvSrc == nrmSrc && nrmSrc != posSrc) { | ||
| uvBase = nrmBase; | ||
| } else { | ||
| uvBase = static_cast<const uint8_t*>(uvBuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| } | ||
| } | ||
| if (colBuf) { | ||
| if (colSrc == posSrc) { | ||
| colBase = posBase; | ||
| } else if (colSrc == nrmSrc && nrmSrc != posSrc) { | ||
| colBase = nrmBase; | ||
| } else if (uvBuf && colSrc == uvSrc && uvSrc != posSrc && !(uvSrc == nrmSrc && nrmSrc != posSrc)) { | ||
| colBase = uvBase; | ||
| } else { | ||
| colBase = static_cast<const uint8_t*>(colBuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| } | ||
| } | ||
|
|
||
| for (uint32_t vi = 0; vi < vCount; ++vi) { | ||
| const uint8_t* prow = posBase + vi * posStride; | ||
| const uint8_t* nrow = nrmBase + vi * nrmStride; | ||
| Ogre::Real* pf = nullptr; | ||
| Ogre::Real* nf = nullptr; | ||
| posEl->baseVertexPointerToElement(const_cast<uint8_t*>(prow), &pf); | ||
| nrmEl->baseVertexPointerToElement(const_cast<uint8_t*>(nrow), &nf); | ||
| // Inverse of import: undo 180° Z then divide by editor scale, then TMD fixed-point. | ||
| const float tmdX = -pf[0] * invEditorScale; | ||
| const float tmdY = -pf[1] * invEditorScale; | ||
| const float tmdZ = pf[2] * invEditorScale; | ||
| const int16_t px = clampI16(static_cast<int>(std::lround(tmdX * invStep))); | ||
| const int16_t py = clampI16(static_cast<int>(std::lround(tmdY * invStep))); | ||
| const int16_t pz = clampI16(static_cast<int>(std::lround(tmdZ * invStep))); | ||
| // Inverse of R_z on normals (same rotation as vertex positions in mesh space). | ||
| const int16_t nx = clampI16(static_cast<int>(std::lround(-nf[0] * 4096.0f))); | ||
| const int16_t ny = clampI16(static_cast<int>(std::lround(-nf[1] * 4096.0f))); | ||
| const int16_t nz = clampI16(static_cast<int>(std::lround(nf[2] * 4096.0f))); | ||
| writeVertex8(px, py, pz, objects[si].verts.data() + vi * 8); | ||
| writeVertex8(nx, ny, nz, objects[si].norms.data() + vi * 8); | ||
| } | ||
|
|
||
| auto ibuf = sm->indexData->indexBuffer; | ||
| const uint8_t* ib = static_cast<const uint8_t*>(ibuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| const bool i32 = ibuf->getType() == Ogre::HardwareIndexBuffer::IT_32BIT; | ||
| const size_t triCount = sm->indexData->indexCount / 3; | ||
| const unsigned ist = sm->indexData->indexStart; | ||
|
|
||
| for (size_t t = 0; t < triCount; ++t) { | ||
| uint32_t i0, i1, i2; | ||
| if (i32) { | ||
| const auto* ip = reinterpret_cast<const uint32_t*>(ib); | ||
| i0 = ip[ist + t * 3 + 0]; | ||
| i1 = ip[ist + t * 3 + 1]; | ||
| i2 = ip[ist + t * 3 + 2]; | ||
| } else { | ||
| const auto* ip = reinterpret_cast<const uint16_t*>(ib); | ||
| i0 = ip[ist + t * 3 + 0]; | ||
| i1 = ip[ist + t * 3 + 1]; | ||
| i2 = ip[ist + t * 3 + 2]; | ||
| } | ||
| if (i0 >= vCount || i1 >= vCount || i2 >= vCount) | ||
| continue; | ||
| if (hasUv && uvEl && uvBase) { | ||
| Ogre::Real* tf = nullptr; | ||
| uvEl->baseVertexPointerToElement(const_cast<uint8_t*>(uvBase + i0 * uvStride), &tf); | ||
| const Ogre::Vector2 tv0(tf[0], tf[1]); | ||
| uvEl->baseVertexPointerToElement(const_cast<uint8_t*>(uvBase + i1 * uvStride), &tf); | ||
| const Ogre::Vector2 tv1(tf[0], tf[1]); | ||
| uvEl->baseVertexPointerToElement(const_cast<uint8_t*>(uvBase + i2 * uvStride), &tf); | ||
| const Ogre::Vector2 tv2(tf[0], tf[1]); | ||
| // Undo import winding swap so .tmd primitive order matches PSX convention on disk. | ||
| appendFt3(objects[si].prims, static_cast<uint16_t>(i0), static_cast<uint16_t>(i2), static_cast<uint16_t>(i1), | ||
| static_cast<uint16_t>(i0), tv0, tv2, tv1); | ||
| } else if (textured) { | ||
| appendFt3(objects[si].prims, static_cast<uint16_t>(i0), static_cast<uint16_t>(i2), static_cast<uint16_t>(i1), | ||
| static_cast<uint16_t>(i0), Ogre::Vector2(0, 0), Ogre::Vector2(0, 0), Ogre::Vector2(0, 0)); | ||
| } else { | ||
| if (colEl && colBase) { | ||
| Ogre::RGBA* c = nullptr; | ||
| colEl->baseVertexPointerToElement(const_cast<uint8_t*>(colBase + i0 * colStride), &c); | ||
| Ogre::ColourValue cv0; cv0.setAsARGB(*c); | ||
| colEl->baseVertexPointerToElement(const_cast<uint8_t*>(colBase + i1 * colStride), &c); | ||
| Ogre::ColourValue cv1; cv1.setAsARGB(*c); | ||
| colEl->baseVertexPointerToElement(const_cast<uint8_t*>(colBase + i2 * colStride), &c); | ||
| Ogre::ColourValue cv2; cv2.setAsARGB(*c); | ||
|
|
||
| appendG3C(objects[si].prims, | ||
| static_cast<uint16_t>(i0), static_cast<uint16_t>(i2), static_cast<uint16_t>(i1), | ||
| static_cast<uint16_t>(i0), static_cast<uint16_t>(i2), static_cast<uint16_t>(i1), | ||
| cv0, cv2, cv1); | ||
| } else { | ||
| appendG3(objects[si].prims, static_cast<uint16_t>(i0), static_cast<uint16_t>(i2), static_cast<uint16_t>(i1), | ||
| static_cast<uint16_t>(i0), static_cast<uint16_t>(i2), static_cast<uint16_t>(i1)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent vertex-index truncation when a submesh has more than 65535 vertices.
PS1 TMD primitive packets encode vertex/normal indices as uint16le, and appendFt3 / appendG3 / appendG3C all take uint16_t arguments. When a source mesh has a submesh with vCount > 65535, the static_cast<uint16_t>(i0/i1/i2) at lines 1125, 1128, 1141, 1145 truncates modulo 65536 and produces a corrupt TMD that points at the wrong vertices — the index‑buffer read at lines 1103‑1112 already widens to uint32_t, so the truncation is silent and survives the i0 >= vCount bounds check. The per‑object vertex blob (line 1037) is sized for the full count, but the primitive stream that references it cannot address it.
Consider rejecting (or splitting) such submeshes early so the on‑disk file stays consistent with the format’s 16‑bit index space.
🛡️ Proposed guard before populating the per-submesh blobs
const uint32_t vCount = vd->vertexCount;
+ if (vCount > 0xFFFFu) {
+ Ogre::LogManager::getSingleton().logMessage(
+ "PS1TMD: submesh exceeds 65535 vertices; PS1 TMD indices are 16-bit — skipping submesh",
+ Ogre::LML_WARNING);
+ continue;
+ }
objects[si].verts.resize(size_t(vCount) * 8u, 0);
objects[si].norms.resize(size_t(vCount) * 8u, 0);🤖 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/PS1/PS1TMD.cpp` around lines 1036 - 1149, The code silently truncates
32-bit indices to uint16 when building TMD primitives: before resizing
objects[si].verts/norms and locking buffers (i.e., where vCount is read), add a
guard that checks if vCount > 65535 and fail fast (log/error/throw or skip this
submesh) so we never produce primitives with truncated indices; references:
vCount, objects[si].verts/norms resize, the index reading that assigns i0/i1/i2,
and the primitive writers appendFt3 / appendG3 / appendG3C — ensure the guard
runs before any per-submesh blob allocation or writing.
Co-authored-by: Cursor <cursoragent@cursor.com>
…ames Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
*.TIM(same basename as the imported*.TMD) onto per-importTMD/<name>materials when UVs exist.Test plan
CAR.TMDand confirmCAR.TIMauto-applies and maps correctly..timfile; confirm it applies and preview updates.GRID.TMDand check mesh appears (follow-up improvements pending).Made with Cursor
Summary by CodeRabbit
New Features
Other