test(PS1): expand unit coverage for PlayStation importer paths - #535
Conversation
Exercise error paths plus 4/8/16 bpp TIM payloads, MAT D/H variants, RSD TEX edge cases, Psy-Q PLY helpers, and TMD sibling `.tim` binding. 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 (5)
📝 WalkthroughWalkthroughThis pull request expands GoogleTest coverage for PS1 file format handling across five core modules: material (MAT), descriptor (RSD), polygon (PLY), texture (TIM), and mesh (TMD) parsing, writing, and integration. All changes are test-only; no production code is modified. The additions focus on negative-case error paths, edge-case data scenarios, and round-trip validation (parse → write → re-parse). ChangesPS1 Format Handler Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05b27528f6
ℹ️ 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".
| ASSERT_TRUE(PS1TIM::loadTimToOgreImage(path, img, &err)) << err.toStdString(); | ||
| const uint8_t* d = img.getData(); | ||
| auto px = [&](int x, int y) { | ||
| const size_t i = (size_t(y) * 256u + size_t(x)) * 4u; |
There was a problem hiding this comment.
The helper lambda in LoadDecodes8bppIndexedImage uses a hard-coded row stride of 256 pixels, but makeMinimalTim8_IndexedStrip() produces an 8×1 image (wWords=4 in 8bpp mode), so any read beyond (0,0) will compute incorrect offsets and can step outside the decoded buffer. This makes the test fragile and unable to reliably validate indexed decode behavior when expanded; derive the stride from img.getWidth() (or the known test width) instead of 256.
Useful? React with 👍 / 👎.
Ensure BaseMaterial exists before TMD import so per-mesh materials clone correctly. Use decoded image width for TIM pixel indexing per review feedback. 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: 4
🤖 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/PS1/PS1MAT_test.cpp`:
- Around line 423-426: The test reads entries into QVector<PS1MAT::MatEntry> out
via PS1MAT::parseMatFile and then accesses out[0] without checking size; add an
assertion (e.g., ASSERT_EQ/ASSERT_GT) on out.size() or EXPECT_EQ to the expected
number of entries immediately after the parse (before accessing out[0]) so the
test fails with a clear message rather than crashing; update the test that calls
PS1MAT::parseMatFile and references out[0] to first assert the parsed entry
count.
- Around line 354-360: The test Rejects_ParseOpenFailure uses a hardcoded Unix
path; change it to create a platform-neutral non-existent path using
QTemporaryDir: instantiate QTemporaryDir, build a path inside it that you don't
create (e.g., tempDir.path() + QDir::separator() + "no_such_file.mat") and pass
that QString to PS1MAT::parseMatFile while keeping the rest of the expectations;
update the test to reference QTemporaryDir and ensure parseMatFile is called
with the constructed path so the test is deterministic across
Windows/Linux/macOS.
In `@src/PS1/PS1PLY_test.cpp`:
- Line 647: The test writes malformed PLY data to the QFile 'f' but does not
close or flush it before calling importPsyqPly, which can leave the buffer
unwritten; modify the test to flush and close 'f' (e.g., call f.flush() and
f.close() or ensure the QFile is destroyed/closed) immediately after
f.write(...) and before calling importPsyqPly so the importer reads the actual
file contents.
In `@src/PS1/PS1RSD_test.cpp`:
- Around line 103-109: The test PS1RSD.ParseFails_WhenFileMissing currently uses
a hardcoded POSIX path; change it to create a QTemporaryDir (e.g. QTemporaryDir
tmpDir) and build a platform-neutral missing-file path (e.g. tmpDir.path() +
QDir::separator() + "model.rsd") and pass that to PS1RSD::parseRsdFile with the
existing PS1RSD::RsdDescriptor d and err variables; ensure the file does not
exist in that temp dir before calling parseRsdFile so the
EXPECT_FALSE(parseRsdFile(...)) and EXPECT_FALSE(err.isEmpty()) assertions
remain valid.
🪄 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: 190268ad-e178-44a6-b117-b1d93aef06dc
📒 Files selected for processing (5)
src/PS1/PS1MAT_test.cppsrc/PS1/PS1PLY_test.cppsrc/PS1/PS1RSD_test.cppsrc/PS1/PS1TIM_test.cppsrc/PS1/PS1TMD_test.cpp
Use QTemporaryDir nested paths for open-failure tests, close PLY file before import, and assert MAT round-trip entry count before indexing. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
PS1TIM,PS1MAT,PS1RSD,PS1PLY,PS1TMD): invalid inputs, handwritten TIM payloads (4 / 8 / 16 bpp), MAT variants and failure paths, RSD TEX index edge cases,configurePsyqRsdMaterialPassbranches, malformed / missing PLY import, TMD magic / missing-file failures, and sibling.timbinding for textured TMD imports.Test plan
cmake --build build_local --target UnitTestsxvfb-run -a ./build_local/bin/UnitTests --gtest_filter='PS1*'(or equivalent with a working GL/display setup)Made with Cursor
Summary by CodeRabbit