Skip to content

feat(ps1): CLUT-aware texture decoding for capture meshes (#421)#656

Open
fernandotonon wants to merge 5 commits into
masterfrom
feat/ps1-texture-decode-421
Open

feat(ps1): CLUT-aware texture decoding for capture meshes (#421)#656
fernandotonon wants to merge 5 commits into
masterfrom
feat/ps1-texture-decode-421

Conversation

@fernandotonon
Copy link
Copy Markdown
Owner

@fernandotonon fernandotonon commented May 22, 2026

Summary

  • Extends TextureDecoder with MaterialKey (TPAGE, CLUT, bit depth, semi-transparency, draw mode), UV-bounds cache merge, and 4/8/15 bpp + STP/transparent-black handling.
  • Wires PS1RipMeshBuilder to pre-decode capture textures, upload to PS1Rip_Session<N>, apply PS1 blend modes, and scope/purge Ogre materials per capture so repeat captures do not collide.
  • Material Editor lists PS1 capture materials but skips GPU thumbnails (prevents freeze); editing remains read-only.

Closes #421.

Test plan

  • TextureDecoderTest unit tests (15/4/8 bpp, STP, draw-mode black, cache merge, VRAM warnings)
  • Capture frame twice in PS1 Rip session — second capture succeeds, textured mesh visible
  • Open Material Editor — no UI freeze; PS1Rip_* materials appear in list
  • CI: Windows, macOS, Linux builds + unit-tests-linux

Made with Cursor

Summary by CodeRabbit

  • Improvements

    • More accurate PS1 mesh & texture reconstruction with improved transparency handling and finer material grouping
    • Faster, cached material-level decoding and session-scoped GPU resource handling to reduce UI stalls and rebuilds
  • Bug Fixes

    • Prevent editing/previewing unsupported PS1/paint-pipeline materials; show an error and skip previews to avoid freezes
    • Better out-of-range VRAM/read warnings and more reliable transparency semantics in decoded textures

Review Change Stack

…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>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

📝 Walkthrough

Walkthrough

This 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.

Changes

Material-Centric Texture Decoding and GPU Management

Layer / File(s) Summary
MaterialKey contract and transparency helpers
src/PS1/runtime/TextureDecoder.h, src/PS1/runtime/PsxVramColor.h, src/PS1/runtime/TextureDecoder.cpp
Introduces TextureDecoder::MaterialKey (TPAGE, CLUT coords, bitDepth, semiTrans, drawModeBits) and adds drawModeMasksZeroAsTransparent() plus helpers mapping tpage/clut into the key.
TextureDecoder decode pipeline and caching
src/PS1/runtime/TextureDecoder.cpp, src/PS1/runtime/TextureDecoder.h
Adds safe VRAM reads (readVramPixel), CLUT lookup, decodeRegion (15/8/4bpp) applying material-specific transparency rules, material-level cache CachedMaterial { QImage, boundsOnPage }, and decodeMaterial with bounds-intersection/merge, warnings, and stats.
PS1RipMeshBuilder session and texture pipeline
src/PS1/runtime/PS1RipMeshBuilder.cpp
Adds session id/resource-group utilities, TextureBuildContext caching, predecodeCaptureTextures, ensureMaterial (configures passes including semi-trans blends), texture upload into explicit resourceGroup, purgePriorCaptureGpuResources(), and refactors attachToScene/attachCaptureSetToScene to use this pipeline.
Mesh Reconstruction semiTrans Integration
src/PS1/runtime/MeshReconstructor.cpp, src/PS1/runtime/MeshReconstructor.h
Adds textureMaterialName(...) and textureGroupKey(...) that include semiTrans and draw-mode bit so primitives with different semi-transparency/draw-mode don't merge.
Material Editor PS1 Rip Blocking
src/MaterialEditorQML.cpp
Adds predicates to detect PS1 rip (PS1Rip_/PS1Rip/) and paint-pipeline materials (QMEPaintMaskOverlay_, QMEPaint_, TexturePaint/); blocks load/edit of PS1 rip materials with an error breadcrumb; skips previews and filters paint-pipeline materials appropriately.
Design documentation and tests
src/PS1/PS1_RIP_DESIGN.md, src/PS1/runtime/TextureDecoder_test.cpp
Updates design doc and converts/extends unit tests to material-key-based decoding covering 15/8/4bpp, CLUT indices, STP/draw-mode transparency semantics, cache dedupe, bounds warnings, and MaterialKey semiTrans separation.

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)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

"🐰 I nibble tiles from VRAM bright,
I stitch them into meshes in the night,
Semi-trans whispers, textures take flight,
Session-scope snug, previews tucked from sight."

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.90% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: CLUT-aware texture decoding for PS1 capture meshes, matching the core technical objective across multiple file changes.
Description check ✅ Passed The description covers Summary, Technical Details with bullets for Features/Bugfixes sections, includes test plan, and correctly references issue #421.
Linked Issues check ✅ Passed All major coding requirements from #421 are met: MaterialKey-based decoding with TPAGE/CLUT/bit-depth/STP/draw-mode support, RGBA output, cache deduplication, Ogre material/texture emission to scoped resource groups, STP blend modes, VRAM warnings, and unit test coverage across bit depths and edge cases.
Out of Scope Changes check ✅ Passed All changes directly support the #421 objectives: TextureDecoder refactoring for MaterialKey decoding, MeshReconstructor updates for texture grouping, PS1RipMeshBuilder session-scoped texture/material lifecycle, MaterialEditorQML filtering for PS1 materials, PsxVramColor transparency helper, and corresponding test coverage—no extraneous modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ps1-texture-decode-421

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/PS1/runtime/MeshReconstructor.cpp Outdated
Comment thread src/PS1/runtime/PS1RipMeshBuilder.cpp Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
src/PS1/runtime/TextureDecoder.cpp (1)

112-118: 💤 Low value

Unused parameter key in readClutColor.

The key parameter 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 decodeRegion accordingly.

🤖 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 win

Use 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 win

Add a draw-mode key-separation test alongside semiTrans separation.

You already verify semiTrans impacts key/hash; adding the same check for drawModeBits will 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

📥 Commits

Reviewing files that changed from the base of the PR and between 77afebc and 0c0ea05.

📒 Files selected for processing (8)
  • src/MaterialEditorQML.cpp
  • src/PS1/PS1_RIP_DESIGN.md
  • src/PS1/runtime/MeshReconstructor.cpp
  • src/PS1/runtime/PS1RipMeshBuilder.cpp
  • src/PS1/runtime/PsxVramColor.h
  • src/PS1/runtime/TextureDecoder.cpp
  • src/PS1/runtime/TextureDecoder.h
  • src/PS1/runtime/TextureDecoder_test.cpp

Comment thread src/MaterialEditorQML.cpp
Comment thread src/PS1/runtime/MeshReconstructor.cpp Outdated
Comment thread src/PS1/runtime/PS1RipMeshBuilder.cpp
fernandotonon and others added 2 commits May 22, 2026 00:42
…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>
@fernandotonon
Copy link
Copy Markdown
Owner Author

Addressed automated review feedback in 9031f36 and CI test fixes in 0a94e3f:

  • Draw-mode grouping (P1): textureMaterialName / textureGroupKey now include _dm0/_dm1 (GP0 bit 11); decoder MaterialKey already keyed on drawModeBits.
  • Mesh purge (P2): purgePriorCaptureGpuResources also removes ps1_unique_* meshes.
  • Bounds clip: pageImageFromCachedTile intersects tile blit with the 256×256 page rect.
  • Sentry: breadcrumb on blocked PS1 material edit in Material Editor.
  • CI: StpBitSetsSemiTransparentAlpha uses alpha 127 (stp is a < 128); OOB test uses partial page overlap QRect(254,0,4,1).

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>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Regex parses _dm suffix but discards the value.

The regex captures the draw-mode suffix (?:_dm([01]))? in group 4, but parseTpageClutMaterial only extracts semiTransOut from group 3 and ignores group 4. This means materials with different draw modes will parse to the same semiTrans value, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0c0ea05 and 679c94c.

📒 Files selected for processing (5)
  • src/MaterialEditorQML.cpp
  • src/PS1/runtime/MeshReconstructor.cpp
  • src/PS1/runtime/MeshReconstructor.h
  • src/PS1/runtime/PS1RipMeshBuilder.cpp
  • src/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>
@fernandotonon
Copy link
Copy Markdown
Owner Author

Addressed latest CodeRabbit finding (06becb4):

  • parseTpageClutMaterial now parses the _dm([01]) suffix into drawModeBitOut.
  • ensureMaterial rebuilds or corrects the TextureDecoder::MaterialKey from that bit so cached RGBA tiles match the material name’s draw-mode variant (GP0 bit 11 / transparent-black).

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Rollback 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 orphan PS1Capture_* node, and attachCaptureSetToScene() 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 returning false.

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 win

Add 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 win

Destroy PS1Rip_Session* resource groups during purge

ensureResourceGroup() creates per-session Ogre resource groups named PS1Rip_Session%1, but purgePriorCaptureGpuResources() (lines ~320-370) only removes meshes/materials/textures and never calls Ogre::ResourceGroupManager::destroyResourceGroup() for those session groups. Add a cleanup step for PS1Rip_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

📥 Commits

Reviewing files that changed from the base of the PR and between 679c94c and 06becb4.

📒 Files selected for processing (1)
  • src/PS1/runtime/PS1RipMeshBuilder.cpp

@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PS1: CLUT-aware texture decoding (4bpp/8bpp/15bpp → RGBA)

1 participant