feat(PS1): PlayStation RSD with Psy-Q PLY/MAT and welded export#454
Conversation
- Add PS1RSD descriptor parse/write, PS1MAT parse/write, Psy-Q PLY import/export\n- RSD export emits PLY+MAT sidecars; PLY export welds corners by quantized pos/normal (+ packed colour when present)\n- Wire .rsd into importer, export filter, CLI format map, Manager extensions\n- Restore TMD editor uniform scale to 10x; update PS1TMD tests\n- Unit tests for RSD/PLY/MAT and CLI extension mapping Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (8)
📝 WalkthroughWalkthroughThis PR adds complete PlayStation 1 (PS1/Psy-Q) RSD asset format support. It introduces three new PS1 format modules (MAT materials, PLY meshes, RSD descriptors), integrates RSD import/export into the main MeshImporterExporter pipeline, and updates file extension recognition throughout the application. ChangesPlayStation RSD Format Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cab78a3385
ℹ️ 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".
| if (!firstTimResource.isEmpty() && rsdMaterialFromScript.isEmpty() && !matPath.isEmpty() && QFileInfo::exists(matPath)) { | ||
| applyTextureMaterialToEntity(const_cast<Ogre::Entity*>(en), | ||
| QStringLiteral("PS1/RSD/") + file.baseName(), | ||
| firstTimResource); |
There was a problem hiding this comment.
This fallback runs for any RSD that has a MAT file and a TIM texture, even when MAT parsing succeeded and face colors were already baked into the imported mesh. In that common case (rsdFaceColors populated), replacing all subentity materials here discards MAT-driven vertex colors and changes the visual result unexpectedly. Gate this fallback on actual MAT-parse failure (or no face colors applied) so valid MAT color data is preserved.
Useful? React with 👍 / 👎.
| if (outFaceColors && sd.colEl && colBase) { | ||
| Ogre::RGBA* cp = nullptr; | ||
| sd.colEl->baseVertexPointerToElement(const_cast<uint8_t*>(colBase + size_t(i0) * sd.colStride), &cp); |
There was a problem hiding this comment.
Face colors are only appended when the source submesh has a diffuse color element, so mixed meshes produce outFaceColors shorter than the exported face count. The importer only applies MAT colors when faceColors->size() == nF, which means a partially colored export causes all MAT colors to be ignored on re-import. Append a default color for uncolored faces (or skip MAT emission unless coverage is complete) to avoid losing color data.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MeshImporterExporter_test.cpp (1)
554-574:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStale assertions in
ExportFileDialogFilter_ContainsAllFormats— this test will fail.The expected filter on line 181 was updated to include
PlayStation RSD (*.rsd), taking the entry count from 19 to 20. That increases the";;"separator count from 18 to 19, but line 554 still asserts18. The spot-check block (lines 556–574) also doesn't include the new RSD entry, so a regression that drops it would not be detected.💚 Proposed fix
- EXPECT_EQ(filter.count(";;"), 18); + EXPECT_EQ(filter.count(";;"), 19); // Spot-check format keys EXPECT_TRUE(filter.contains("3DS (*.3ds)")); EXPECT_TRUE(filter.contains("Assimp Binary (*.assbin)")); ... EXPECT_TRUE(filter.contains("PLY (*.ply)")); + EXPECT_TRUE(filter.contains("PlayStation RSD (*.rsd)")); EXPECT_TRUE(filter.contains("PlayStation TMD (*.tmd)"));🤖 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_test.cpp` around lines 554 - 574, The test ExportFileDialogFilter_ContainsAllFormats has stale assertions after adding "PlayStation RSD (*.rsd)"; update the expected separator count from 18 to 19 in the EXPECT_EQ(filter.count(";;"), ...) and add a spot-check EXPECT_TRUE(filter.contains("PlayStation RSD (*.rsd)")) to the block that verifies format keys so the test reflects the new entry and will catch regressions.
🧹 Nitpick comments (2)
src/PS1/PS1RSD.cpp (1)
150-160: 💤 Low valueOptional: write
NTEXconsistent with what is actually serialized.When
desc.texturescontains empty entries the loop skips them (line 156-157), so the written file can have fewerTEX[...]lines than theNTEX=value indicates. Round-tripping then yields a parsedntexthat disagrees withtextures.size(). Consider derivingntexfrom the count of non-empty entries (or pre-trimming), so writers/readers stay aligned.🤖 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/PS1RSD.cpp` around lines 150 - 160, The NTEX value is computed from desc.ntex or tex.size() but the loop skips empty entries, causing NTEX to disagree with the number of written TEX[...] lines; update the writer in PS1RSD.cpp to compute ntex from the count of non-empty, trimmed entries (e.g., iterate desc.textures, trim each QString and collect/non-empty count) or pre-trim/filter tex into a new list and use its size for ntex, then write TEX[i] only for that filtered list so NTEX matches the actual serialized TEX lines (references: variables desc, tex, ntex and the loop writing "TEX[" << i << "]").src/PS1/PS1PLY.cpp (1)
760-791: 💤 Low valueOptional: avoid decoding the same vertex colour twice per triangle.
c0/c1/c2already hold the raw packed RGBA at lines 760–769, and lines 778–791 re-issuebaseVertexPointerToElementfor the same three corners just to calldecodePackedColour. You can decode directly from the values you already extracted:- if (outFaceColors && sd.colEl && colBase) { - Ogre::RGBA* cp = nullptr; - sd.colEl->baseVertexPointerToElement(const_cast<uint8_t*>(colBase + size_t(i0) * sd.colStride), &cp); - const Ogre::ColourValue cv0 = decodePackedColour(sd.colEl, *cp); - sd.colEl->baseVertexPointerToElement(const_cast<uint8_t*>(colBase + size_t(i1) * sd.colStride), &cp); - const Ogre::ColourValue cv1 = decodePackedColour(sd.colEl, *cp); - sd.colEl->baseVertexPointerToElement(const_cast<uint8_t*>(colBase + size_t(i2) * sd.colStride), &cp); - const Ogre::ColourValue cv2 = decodePackedColour(sd.colEl, *cp); + if (outFaceColors && sd.colEl && colBase) { + const Ogre::ColourValue cv0 = decodePackedColour(sd.colEl, static_cast<Ogre::RGBA>(c0)); + const Ogre::ColourValue cv1 = decodePackedColour(sd.colEl, static_cast<Ogre::RGBA>(c1)); + const Ogre::ColourValue cv2 = decodePackedColour(sd.colEl, static_cast<Ogre::RGBA>(c2));While at it,
outFaceColors->reserve(totalFaces)near line 685 would avoid repeated reallocations.🤖 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/PS1PLY.cpp` around lines 760 - 791, The code redundantly calls sd.colEl->baseVertexPointerToElement three more times to decode colours even though c0/c1/c2 already contain the packed RGBA values; change the outFaceColors block to call decodePackedColour(sd.colEl, c0/ c1/ c2) directly (using the existing c0, c1, c2 variables) instead of re-fetching via baseVertexPointerToElement, and push the averaged QColor as before; additionally, where outFaceColors is created (before triangle loop, e.g. near the reserve area), call outFaceColors->reserve(totalFaces) to avoid repeated reallocations.
🤖 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/MeshImporterExporter.cpp`:
- Around line 1687-1703: MeshImporterExporter::importFileDialogFilter currently
calls Manager::getSingleton()->getValidFileExtention(), which bootstraps the
Manager/Ogre subsystem; change it to query a non-initializing source for the
extensions instead. Add or use a static, non-instantiating accessor on Manager
(e.g. Manager::getValidFileExtensionsStatic() or Manager::validFileExtensions())
that returns the extension string without constructing the singleton, or provide
a free helper that reads a cached/compile-time list; then update
MeshImporterExporter::importFileDialogFilter to call that static/helper function
instead of Manager::getSingleton()->getValidFileExtention(), leaving the rest of
the filter-building code unchanged. Ensure the new accessor is implemented so it
does not touch Ogre/render-system state and adjust any other call sites that
currently call getValidFileExtention() via the singleton.
- Around line 1523-1529: The fallback currently calls
applyTextureMaterialToEntity whenever a TIM and MAT exist even if the MAT parsed
successfully and per-face colours (rsdFaceColors) were applied; change the guard
so the fallback only runs when there is no parsed material and no per-face
colours: check that rsdMaterialFromScript.isEmpty() AND rsdFaceColors is empty
(or PS1MAT::parseMatFile() did not return success) before invoking
applyTextureMaterialToEntity, using the existing symbols firstTimResource,
rsdMaterialFromScript, rsdFaceColors, matPath and applyTextureMaterialToEntity
to locate and update the condition.
In `@src/PS1/PS1MAT.cpp`:
- Around line 40-92: sawHeader is set for any line starting with '@' and the
function currently returns success even when outEntries.size() < expected;
update the parsing to only recognize an explicit "@MAT" header (e.g. change the
check to t.startsWith(QStringLiteral("@MAT")) or compare uppercased t == "@MAT")
when setting sawHeader, and after the material parsing loop validate that
outEntries.size() >= expected — if fewer entries were parsed set outError (e.g.
"Parsed X of expected Y material entries.") and return false; touch symbols:
sawHeader, expected, outEntries, lines, isSkippable, and the material-parsing
block that builds MatEntry/QColor so the logic is enforced in the same function.
In `@src/PS1/PS1PLY.cpp`:
- Around line 380-397: The current Psy-Q vs Blender face-format heuristic (tok,
psyq, v0..v2, n0..n2 in PS1PLY.cpp) can misclassify Blender faces when tok[4] ==
0; change the detection to be deterministic: treat Psy-Q only when tok.size() ==
9 (exact match) and otherwise assume the Blender/RSD ordering, and additionally,
after triangulation where triangle normals are computed, validate the assigned
corner normals (using the existing geometric normal/dot-product check) and if a
clear mismatch is detected, retry with the alternate ordering (swap v1/v2 and
n1/n2) to correct winding/normals.
In `@src/PS1/PS1RSD.cpp`:
- Around line 99-108: The TEX index must be bounded to avoid OOM from a huge
idx; after calling parseTexIndex(key) in the PS1RSD parsing branch (where
parseTexIndex and out.textures are used), add a check against a sensible
constant (e.g. MAX_TEXTURES or 256) and handle out-of-range indices by either
appending the value (same behavior as the negative case) or skipping with a
warning/log; only call out.textures.resize(idx + 1) and assign out.textures[idx]
when 0 <= idx < MAX_TEXTURES to prevent unbounded QStringList allocation.
---
Outside diff comments:
In `@src/MeshImporterExporter_test.cpp`:
- Around line 554-574: The test ExportFileDialogFilter_ContainsAllFormats has
stale assertions after adding "PlayStation RSD (*.rsd)"; update the expected
separator count from 18 to 19 in the EXPECT_EQ(filter.count(";;"), ...) and add
a spot-check EXPECT_TRUE(filter.contains("PlayStation RSD (*.rsd)")) to the
block that verifies format keys so the test reflects the new entry and will
catch regressions.
---
Nitpick comments:
In `@src/PS1/PS1PLY.cpp`:
- Around line 760-791: The code redundantly calls
sd.colEl->baseVertexPointerToElement three more times to decode colours even
though c0/c1/c2 already contain the packed RGBA values; change the outFaceColors
block to call decodePackedColour(sd.colEl, c0/ c1/ c2) directly (using the
existing c0, c1, c2 variables) instead of re-fetching via
baseVertexPointerToElement, and push the averaged QColor as before;
additionally, where outFaceColors is created (before triangle loop, e.g. near
the reserve area), call outFaceColors->reserve(totalFaces) to avoid repeated
reallocations.
In `@src/PS1/PS1RSD.cpp`:
- Around line 150-160: The NTEX value is computed from desc.ntex or tex.size()
but the loop skips empty entries, causing NTEX to disagree with the number of
written TEX[...] lines; update the writer in PS1RSD.cpp to compute ntex from the
count of non-empty, trimmed entries (e.g., iterate desc.textures, trim each
QString and collect/non-empty count) or pre-trim/filter tex into a new list and
use its size for ntex, then write TEX[i] only for that filtered list so NTEX
matches the actual serialized TEX lines (references: variables desc, tex, ntex
and the loop writing "TEX[" << i << "]").
🪄 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: d096b787-a151-4f47-a2c3-cf99df20dd35
📒 Files selected for processing (17)
src/CLIPipeline.cppsrc/CLIPipeline_test.cppsrc/Manager.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter.hsrc/MeshImporterExporter_test.cppsrc/PS1/CMakeLists.txtsrc/PS1/PS1MAT.cppsrc/PS1/PS1MAT.hsrc/PS1/PS1PLY.cppsrc/PS1/PS1PLY.hsrc/PS1/PS1PLY_test.cppsrc/PS1/PS1RSD.cppsrc/PS1/PS1RSD.hsrc/PS1/PS1RSD_test.cppsrc/PS1/PS1TMD.hsrc/PS1/PS1TMD_test.cpp
| QString MeshImporterExporter::importFileDialogFilter() | ||
| { | ||
| const QStringList parts = | ||
| Manager::getSingleton()->getValidFileExtention().split(' ', Qt::SkipEmptyParts); | ||
| QStringList globs; | ||
| globs.reserve(parts.size()); | ||
| for (QString ext : parts) { | ||
| ext = ext.trimmed(); | ||
| if (ext.startsWith('.')) | ||
| globs.append(QLatin1Char('*') + ext); | ||
| } | ||
| const QString allSupported = globs.join(QLatin1Char(' ')); | ||
| return QStringLiteral( | ||
| "All supported (%1);;" | ||
| "PlayStation RSD / TMD / Psy-Q PLY (*.rsd *.tmd *.ply);;" | ||
| "All files (*.*)") | ||
| .arg(allSupported); |
There was a problem hiding this comment.
Avoid bootstrapping Ogre just to build a file-dialog filter.
Line 1690 calls Manager::getSingleton(), which constructs Manager and initializes Ogre/render-system state. Opening the import dialog or unit-testing this helper now has a heavyweight side effect before any file is selected.
🤖 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 1687 - 1703,
MeshImporterExporter::importFileDialogFilter currently calls
Manager::getSingleton()->getValidFileExtention(), which bootstraps the
Manager/Ogre subsystem; change it to query a non-initializing source for the
extensions instead. Add or use a static, non-instantiating accessor on Manager
(e.g. Manager::getValidFileExtensionsStatic() or Manager::validFileExtensions())
that returns the extension string without constructing the singleton, or provide
a free helper that reads a cached/compile-time list; then update
MeshImporterExporter::importFileDialogFilter to call that static/helper function
instead of Manager::getSingleton()->getValidFileExtention(), leaving the rest of
the filter-building code unchanged. Ensure the new accessor is implemented so it
does not touch Ogre/render-system state and adjust any other call sites that
currently call getValidFileExtention() via the singleton.
| if (tok[0] == 0) { | ||
| // Triangle formats seen in the wild: | ||
| // - Psy-Q: 0 v0 v1 v2 0 n0 n1 n2 0 | ||
| // - Blender/RSD exporter: 0 v0 v2 v1 sep n0 n2 n1 end | ||
| if (tok.size() < 9) | ||
| return false; | ||
|
|
||
| int v0 = 0, v1 = 0, v2 = 0; | ||
| int n0 = 0, n1 = 0, n2 = 0; | ||
|
|
||
| const bool psyq = (tok.size() >= 9 && tok[4] == 0 && tok[8] == 0); | ||
| if (psyq) { | ||
| v0 = tok[1]; v1 = tok[2]; v2 = tok[3]; | ||
| n0 = tok[5]; n1 = tok[6]; n2 = tok[7]; | ||
| } else { | ||
| v0 = tok[1]; v2 = tok[2]; v1 = tok[3]; | ||
| n0 = tok[5]; n2 = tok[6]; n1 = tok[7]; | ||
| } |
There was a problem hiding this comment.
Triangle-format heuristic can misclassify legitimate Blender exports.
The Psy-Q-vs-Blender detection relies on tok[4] == 0 && tok[8] == 0, but tok[4] in the Blender/RSD-exporter format is a separator/material-group field whose value is not guaranteed to be non-zero. A face line emitted by Blender with separator/material id 0 will be misread as Psy-Q and the per-corner indices will be wrong (silent geometry corruption — wrong winding, swapped normals).
Consider one of:
- Detecting the flavor once from the file header (e.g.
#PLY Mesh Dataprefix already noted in tests for the Blender exporter) and locking the per-face decoder to that flavor. - Requiring
tok.size() == 9for Psy-Q and falling back deterministically. - Validating geometric consistency (e.g. dot-product check against the triangle normal — already done after triangulation) and re-trying the alternate ordering on a clear mismatch instead of guessing up front.
🤖 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/PS1PLY.cpp` around lines 380 - 397, The current Psy-Q vs Blender
face-format heuristic (tok, psyq, v0..v2, n0..n2 in PS1PLY.cpp) can misclassify
Blender faces when tok[4] == 0; change the detection to be deterministic: treat
Psy-Q only when tok.size() == 9 (exact match) and otherwise assume the
Blender/RSD ordering, and additionally, after triangulation where triangle
normals are computed, validate the assigned corner normals (using the existing
geometric normal/dot-product check) and if a clear mismatch is detected, retry
with the alternate ordering (swap v1/v2 and n1/n2) to correct winding/normals.
Resolve MeshImporterExporter configureCamera/importer conflict (keep world bbox + master try/catch). Fix CI: link PS1MAT, PS1PLY, PS1RSD in MaterialEditorQML test targets (undefined refs from MeshImporterExporter). Co-authored-by: Cursor <cursoragent@cursor.com>
Review follow-up: bound RSD TEX[] slot index; write NTEX from non-empty textures; require @mat header and full MAT entry count; skip RSD texture fallback when vertex colours applied; reuse packed colours in Psy-Q PLY export path. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
Adds PlayStation RSD support: parse/write descriptor files, import geometry via Psy-Q PLY (with optional MAT face colours), and export RSD with PLY + MAT sidecars.
Details
PS1RSD,PS1MAT,PS1PLY(+ unit tests).*.ply+*.matand references them from the.rsd.kTmdEditorUniformScale); tests updated..rsdinto MeshImporterExporter, export dialog filter, CLIPipeline format map, Manager valid extensions.Testing
QtMeshEditorlocally.PS1*_test.cpp,CLIPipeline_test,MeshImporterExporter_test.Made with Cursor
Summary by CodeRabbit
New Features
Tests