feat(ps1): CLUT-aware texture decoding for capture meshes (#421)#656
feat(ps1): CLUT-aware texture decoding for capture meshes (#421)#656fernandotonon wants to merge 5 commits into
Conversation
…421) Decode 4/8/15 bpp tiles with MaterialKey caching and UV-bounds merge, upload textures to PS1Rip_Session resource groups with semi-transparency blend modes, and purge or reuse capture-scoped Ogre materials so repeat captures succeed. Hide PS1 materials from GPU thumbnails in Material Editor to avoid UI freezes. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR refactors PS1 texture decoding from tile-centric to a material-centric MaterialKey (TPAGE/CLUT/bitDepth/semiTrans/drawMode), adds safe VRAM/CLUT reads and material-level caching, introduces session-scoped GPU resource building/cleanup in PS1RipMeshBuilder, includes semiTrans in mesh material grouping, and blocks editor edits/previews for generated PS1 rip/paint-pipeline materials. ChangesMaterial-Centric Texture Decoding and GPU Management
Sequence Diagram(s)sequenceDiagram
participant UI
participant TextureDecoder
participant Cache
participant VramSnapshot
participant TextureBuildContext
participant Ogre
UI->>TextureDecoder: request decodeMaterial(vram, MaterialKey, uvBounds)
TextureDecoder->>Cache: cachedMaterial(key)?
alt cache hit (covers)
Cache-->>TextureDecoder: QImage
else cache miss / partial
TextureDecoder->>VramSnapshot: readVramPixel/readClutColor
TextureDecoder->>TextureDecoder: decodeRegion -> QImage
TextureDecoder->>Cache: store CachedMaterial(image,bounds)
end
TextureDecoder-->>UI: QImage
UI->>TextureBuildContext: predecodeCaptureTextures(keys)
TextureBuildContext->>TextureDecoder: decodeMaterial(...) for keys
TextureBuildContext->>Ogre: uploadTextureToOgre(resourceGroup,...)
TextureBuildContext->>Ogre: create/configure Material (applySemiTransBlend)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 docstrings
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c0ea05b44
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/PS1/runtime/TextureDecoder.cpp (1)
112-118: 💤 Low valueUnused parameter
keyinreadClutColor.The
keyparameter is passed but never referenced in the function body. Consider removing it to avoid compiler warnings and reduce confusion.♻️ Suggested fix
-uint16_t TextureDecoder::readClutColor(const VramSnapshot &vram, int clutX, int clutY, int index, - BitDepth bitDepth, const MaterialKey &key) +uint16_t TextureDecoder::readClutColor(const VramSnapshot &vram, int clutX, int clutY, int index, + BitDepth bitDepth) { const int maxIndex = bitDepth == BitDepth::Bpp4 ? 15 : 255; const int clamped = std::clamp(index, 0, maxIndex); return readVramPixel(vram, clutX + clamped, clutY); }Update call sites in
decodeRegionaccordingly.🤖 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/runtime/TextureDecoder.cpp` around lines 112 - 118, Remove the unused parameter key from the readClutColor signature and all corresponding declarations/overloads, then update every call site (notably the calls from decodeRegion) to stop passing that argument; ensure the function signature readClutColor(const VramSnapshot &vram, int clutX, int clutY, int index, BitDepth bitDepth) is used consistently across declarations, definitions and callers.src/PS1/runtime/PS1RipMeshBuilder.cpp (1)
219-224: ⚡ Quick winUse a standard breadcrumb category and keep event name in the message.
The breadcrumb currently uses an event-like category (
ps1.rip.texture.decoded). Please align category with the shared taxonomy (for consistent dashboards/queries) and keep this identifier in the message payload.As per coding guidelines:
Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message) using categories like "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/PS1/runtime/PS1RipMeshBuilder.cpp` around lines 219 - 224, The breadcrumb is using an event-like category; update the SentryReporter::addBreadcrumb call to use a standard category (e.g., "file.import" or another taxonomy-compliant category such as "file.process") and move the event identifier into the message payload so the message still contains "ps1.rip.texture.decoded" along with the existing stats (decodedMaterials, rgbaBytes, ctx->decoder.warnings().size()); keep the same message formatting but replace the first argument with the chosen standard category.src/PS1/runtime/TextureDecoder_test.cpp (1)
153-161: ⚡ Quick winAdd a draw-mode key-separation test alongside
semiTransseparation.You already verify
semiTransimpacts key/hash; adding the same check fordrawModeBitswill protect the material-key contract and catch future cache/grouping regressions.✅ Suggested test addition
+TEST(TextureDecoderTest, MaterialKeySeparatesDrawModeBits) +{ + TextureDecoder::MaterialKey a{}; + TextureDecoder::MaterialKey b{}; + a.drawModeBits = 0; + b.drawModeBits = 1u << 11; + EXPECT_NE(a.drawModeBits, b.drawModeBits); + EXPECT_NE(qHash(a), qHash(b)); +}🤖 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/runtime/TextureDecoder_test.cpp` around lines 153 - 161, Add a second assertion mirroring the semiTrans test to ensure drawModeBits affects the key/hash: in the TextureDecoder::MaterialKey unit test create two MaterialKey instances (e.g., a and b), set a.drawModeBits = 0 and b.drawModeBits = 1 (or different values), assert a.drawModeBits != b.drawModeBits and then EXPECT_NE(qHash(a), qHash(b)); this verifies TextureDecoder::MaterialKey and qHash respect drawModeBits.
🤖 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 210-215: When a PS1 capture material edit is blocked in the
isPs1RipMaterial check inside MaterialEditorQML.cpp, add a Sentry breadcrumb
before emitting the error and returning: call SentryReporter::addBreadcrumb with
a suitable category like "user.action" and a message including the materialName
and action (e.g., "blocked_edit_ps1_material: <materialName>"). Place this
breadcrumb immediately before the emit errorOccurred(...) line in the
isPs1RipMaterial handling so the breadcrumb records the blocked edit attempt.
In `@src/PS1/runtime/MeshReconstructor.cpp`:
- Around line 23-35: The material grouping currently ignores draw-mode bits
causing merges across different draw modes; update textureMaterialName and
textureGroupKey to include the draw-mode bits (the specific bits used by the
renderer) in both the returned QString and the quint64 key (e.g., append/include
a drawMode value alongside tpage, clut, semiTrans), and then update the matching
parser/lookup format in PS1RipMeshBuilder.cpp so it reads/parses the draw-mode
from the material name/key consistently with the new format; modify
textureMaterialName, textureGroupKey, and the parser/lookup in
PS1RipMeshBuilder.cpp to use the same unique symbol (drawMode) so primitives
with different draw modes no longer collapse into one material.
In `@src/PS1/runtime/PS1RipMeshBuilder.cpp`:
- Around line 167-178: The loop in pageImageFromCachedTile writes into the fixed
kPageTexels×kPageTexels page without clamping boundsOnPage, risking
out-of-bounds writes; fix by first intersecting boundsOnPage with
QRect(0,0,kPageTexels,kPageTexels) to get a clippedRect, compute source offsets
(x/y start offsets into tile) from the difference between boundsOnPage and
clippedRect, and then iterate only over clippedRect dimensions using src +
srcXStart/srcYStart and page.scanLine(clippedRect.y()+y) with dst index
clippedRect.x()+x so all writes remain inside the page. Ensure you return early
if the clippedRect is empty and adjust loops to use clipped widths/heights and
the corresponding source offsets.
---
Nitpick comments:
In `@src/PS1/runtime/PS1RipMeshBuilder.cpp`:
- Around line 219-224: The breadcrumb is using an event-like category; update
the SentryReporter::addBreadcrumb call to use a standard category (e.g.,
"file.import" or another taxonomy-compliant category such as "file.process") and
move the event identifier into the message payload so the message still contains
"ps1.rip.texture.decoded" along with the existing stats (decodedMaterials,
rgbaBytes, ctx->decoder.warnings().size()); keep the same message formatting but
replace the first argument with the chosen standard category.
In `@src/PS1/runtime/TextureDecoder_test.cpp`:
- Around line 153-161: Add a second assertion mirroring the semiTrans test to
ensure drawModeBits affects the key/hash: in the TextureDecoder::MaterialKey
unit test create two MaterialKey instances (e.g., a and b), set a.drawModeBits =
0 and b.drawModeBits = 1 (or different values), assert a.drawModeBits !=
b.drawModeBits and then EXPECT_NE(qHash(a), qHash(b)); this verifies
TextureDecoder::MaterialKey and qHash respect drawModeBits.
In `@src/PS1/runtime/TextureDecoder.cpp`:
- Around line 112-118: Remove the unused parameter key from the readClutColor
signature and all corresponding declarations/overloads, then update every call
site (notably the calls from decodeRegion) to stop passing that argument; ensure
the function signature readClutColor(const VramSnapshot &vram, int clutX, int
clutY, int index, BitDepth bitDepth) is used consistently across declarations,
definitions and callers.
🪄 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: 67b672fc-d927-468b-a40d-e1bfcf269c5b
📒 Files selected for processing (8)
src/MaterialEditorQML.cppsrc/PS1/PS1_RIP_DESIGN.mdsrc/PS1/runtime/MeshReconstructor.cppsrc/PS1/runtime/PS1RipMeshBuilder.cppsrc/PS1/runtime/PsxVramColor.hsrc/PS1/runtime/TextureDecoder.cppsrc/PS1/runtime/TextureDecoder.hsrc/PS1/runtime/TextureDecoder_test.cpp
…clip Include GP0 draw-mode bit 11 in texture material keys, purge ps1_unique_ meshes between captures, clamp page blit to 256×256, and add Sentry breadcrumb when blocking PS1 material edits in Material Editor. Co-authored-by: Cursor <cursoragent@cursor.com>
STP uses alpha < 128, so use 127 in the fixture. OOB warnings need a rect that partially overlaps the page, not one fully outside it. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed automated review feedback in
|
Page-local UVs on TPAGE 0 stay inside 1024-wide VRAM; use TPAGE 0x000F (origin x=960) so a 4px-wide strip at u=254 reads past x=1023. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/PS1/runtime/PS1RipMeshBuilder.cpp (1)
58-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegex parses
_dmsuffix but discards the value.The regex captures the draw-mode suffix
(?:_dm([01]))?in group 4, butparseTpageClutMaterialonly extractssemiTransOutfrom group 3 and ignores group 4. This means materials with different draw modes will parse to the samesemiTransvalue, potentially applying incorrect blend modes.🛠️ Proposed fix to extract drawModeBits
bool parseTpageClutMaterial(const QString &name, uint16_t &tpageOut, uint16_t &clutOut, - uint8_t &semiTransOut) + uint8_t &semiTransOut, uint16_t &drawModeOut) { static const QRegularExpression re( QStringLiteral("^PS1Rip_tpage_([0-9a-fA-F]+)_clut_([0-9a-fA-F]+)(?:_st([0-3]))?(?:_dm([01]))?$")); const QRegularExpressionMatch match = re.match(name); if (!match.hasMatch()) return false; tpageOut = static_cast<uint16_t>(match.captured(1).toUInt(nullptr, 16)); clutOut = static_cast<uint16_t>(match.captured(2).toUInt(nullptr, 16)); semiTransOut = match.captured(3).isEmpty() ? 0 : static_cast<uint8_t>(match.captured(3).toUInt()); + drawModeOut = match.captured(4).isEmpty() ? 0 + : static_cast<uint16_t>(match.captured(4).toUInt() << 11); return true; }Then update the call site at Line 264 to use the extracted draw mode if needed for transparency handling.
🤖 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/runtime/PS1RipMeshBuilder.cpp` around lines 58 - 71, parseTpageClutMaterial's regex captures a draw-mode group (_dm([01])) but the code currently ignores it; change the function signature to accept an additional uint8_t& drawModeOut, parse match.captured(4) into drawModeOut (default 0 when empty) alongside tpageOut/clutOut/semiTransOut, and then update the caller(s) that invoke parseTpageClutMaterial so they pass a variable for drawModeOut and use that extracted draw mode bit when deciding transparency/blend behavior (where the previous code assumed only semiTransOut).
🤖 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.
Outside diff comments:
In `@src/PS1/runtime/PS1RipMeshBuilder.cpp`:
- Around line 58-71: parseTpageClutMaterial's regex captures a draw-mode group
(_dm([01])) but the code currently ignores it; change the function signature to
accept an additional uint8_t& drawModeOut, parse match.captured(4) into
drawModeOut (default 0 when empty) alongside tpageOut/clutOut/semiTransOut, and
then update the caller(s) that invoke parseTpageClutMaterial so they pass a
variable for drawModeOut and use that extracted draw mode bit when deciding
transparency/blend behavior (where the previous code assumed only semiTransOut).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9e84331-198b-49f4-ae52-2511326aae4f
📒 Files selected for processing (5)
src/MaterialEditorQML.cppsrc/PS1/runtime/MeshReconstructor.cppsrc/PS1/runtime/MeshReconstructor.hsrc/PS1/runtime/PS1RipMeshBuilder.cppsrc/PS1/runtime/TextureDecoder_test.cpp
parseTpageClutMaterial now extracts _dm0/_dm1 and ensureMaterial uses it when resolving TextureDecoder cache keys so decode matches the name. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed latest CodeRabbit finding (
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/PS1/runtime/PS1RipMeshBuilder.cpp (3)
520-532:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback partial scene state on attach failure.
At Line 525 and Line 628 the scene is already mutated before
createEntity()can fail.attachToScene()then returns with an orphanPS1Capture_*node, andattachCaptureSetToScene()still leaves the freshly built GPU resources alive until the next purge. Please destroy the just-created node(s) and purge the new session resources before returningfalse.Also applies to: 617-640
🤖 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/runtime/PS1RipMeshBuilder.cpp` around lines 520 - 532, The code mutates the scene before calling mgr->createEntity (via buildMeshResources, mgr->addSceneNode and applyEditorPlacement), so when createEntity fails in attachToScene (and similarly around lines 617-640) it leaves orphan PS1Capture_* SceneNode(s) and GPU resources; update attachToScene/attachCaptureSetToScene to rollback on failure by explicitly destroying the newly created SceneNode(s) (the node returned from mgr->addSceneNode / any children created during applyEditorPlacement) and invoking the existing resource cleanup/purge routine for the session (the same purge that normally frees the meshes/textures created by buildMeshResources) before returning false, ensuring the same cleanup path is used for both single-entity failures and the multi-entity path.
510-518: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd breadcrumbs for the attach/purge/build lifecycle.
This flow is user-facing and does significant GPU/resource work, but the only breadcrumb here is the decode summary inside
predecodeCaptureTextures(). Add breadcrumbs for attach start, purge, success, and failure so capture problems are reconstructable in Sentry. As per coding guidelines,Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message) using categories like "ui.action" for toolbar/menu clicks, "ai.tool_call" for MCP tool invocations, "file.import"/"file.export" for I/O operations.Also applies to: 562-604
🤖 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/runtime/PS1RipMeshBuilder.cpp` around lines 510 - 518, Wrap the attach/purge/build sequence with Sentry breadcrumbs: before removing prior nodes call SentryReporter::addBreadcrumb("ui.action", "attach.capture.start"), before purge call addBreadcrumb("file.import", "attach.purge.start"), after successful purge call addBreadcrumb("file.import", "attach.purge.success"), and on any failure paths addBreadcrumb("file.import", "attach.purge.failure") (include error details). Place these calls around removePriorCaptureNodes(mgr), purgePriorCaptureGpuResources(), and before/after predecodeCaptureTextures(textureSource, &texCtx) (use "ai.tool_call" or "file.import" as appropriate for predecode start/success/failure), and replicate the same breadcrumb pattern in the other related block that follows the same attach/purge/build flow.
39-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDestroy
PS1Rip_Session*resource groups during purge
ensureResourceGroup()creates per-session Ogre resource groups namedPS1Rip_Session%1, butpurgePriorCaptureGpuResources()(lines ~320-370) only removes meshes/materials/textures and never callsOgre::ResourceGroupManager::destroyResourceGroup()for those session groups. Add a cleanup step forPS1Rip_Session*groups inside the purge path to prevent resource-group accumulation across repeated captures.🤖 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/runtime/PS1RipMeshBuilder.cpp` around lines 39 - 56, The purge path currently removes meshes/materials/textures but never destroys the per-session Ogre resource groups created by sessionResourceGroup()/ensureResourceGroup(), causing accumulation; update purgePriorCaptureGpuResources() to iterate active/previous session IDs (or scan ResourceGroupManager for groups matching "PS1Rip_Session%d") and call Ogre::ResourceGroupManager::getSingleton().destroyResourceGroup(groupName) for each matching group (ensuring resources are unloaded first), and reference the same session naming used by sessionResourceGroup()/allocateSessionId() so groups like "PS1Rip_Session<id>" are removed during purge.
🤖 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.
Outside diff comments:
In `@src/PS1/runtime/PS1RipMeshBuilder.cpp`:
- Around line 520-532: The code mutates the scene before calling
mgr->createEntity (via buildMeshResources, mgr->addSceneNode and
applyEditorPlacement), so when createEntity fails in attachToScene (and
similarly around lines 617-640) it leaves orphan PS1Capture_* SceneNode(s) and
GPU resources; update attachToScene/attachCaptureSetToScene to rollback on
failure by explicitly destroying the newly created SceneNode(s) (the node
returned from mgr->addSceneNode / any children created during
applyEditorPlacement) and invoking the existing resource cleanup/purge routine
for the session (the same purge that normally frees the meshes/textures created
by buildMeshResources) before returning false, ensuring the same cleanup path is
used for both single-entity failures and the multi-entity path.
- Around line 510-518: Wrap the attach/purge/build sequence with Sentry
breadcrumbs: before removing prior nodes call
SentryReporter::addBreadcrumb("ui.action", "attach.capture.start"), before purge
call addBreadcrumb("file.import", "attach.purge.start"), after successful purge
call addBreadcrumb("file.import", "attach.purge.success"), and on any failure
paths addBreadcrumb("file.import", "attach.purge.failure") (include error
details). Place these calls around removePriorCaptureNodes(mgr),
purgePriorCaptureGpuResources(), and before/after
predecodeCaptureTextures(textureSource, &texCtx) (use "ai.tool_call" or
"file.import" as appropriate for predecode start/success/failure), and replicate
the same breadcrumb pattern in the other related block that follows the same
attach/purge/build flow.
- Around line 39-56: The purge path currently removes meshes/materials/textures
but never destroys the per-session Ogre resource groups created by
sessionResourceGroup()/ensureResourceGroup(), causing accumulation; update
purgePriorCaptureGpuResources() to iterate active/previous session IDs (or scan
ResourceGroupManager for groups matching "PS1Rip_Session%d") and call
Ogre::ResourceGroupManager::getSingleton().destroyResourceGroup(groupName) for
each matching group (ensuring resources are unloaded first), and reference the
same session naming used by sessionResourceGroup()/allocateSessionId() so groups
like "PS1Rip_Session<id>" are removed during purge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4806c8a9-4086-4920-ac37-e65e269990ea
📒 Files selected for processing (1)
src/PS1/runtime/PS1RipMeshBuilder.cpp
|



Summary
TextureDecoderwithMaterialKey(TPAGE, CLUT, bit depth, semi-transparency, draw mode), UV-bounds cache merge, and 4/8/15 bpp + STP/transparent-black handling.PS1RipMeshBuilderto pre-decode capture textures, upload toPS1Rip_Session<N>, apply PS1 blend modes, and scope/purge Ogre materials per capture so repeat captures do not collide.Closes #421.
Test plan
TextureDecoderTestunit tests (15/4/8 bpp, STP, draw-mode black, cache merge, VRAM warnings)PS1Rip_*materials appear in listunit-tests-linuxMade with Cursor
Summary by CodeRabbit
Improvements
Bug Fixes