feat(ps1): Phase 4 mesh reconstruction from capture (#422)#601
Open
fernandotonon wants to merge 2 commits into
Open
feat(ps1): Phase 4 mesh reconstruction from capture (#422)#601fernandotonon wants to merge 2 commits into
fernandotonon wants to merge 2 commits into
Conversation
Reconstruct captured primitives into Ogre meshes grouped by matrix and texture, attach PS1Capture nodes to the editor scene, and add Arm/Capture toolbar controls with unit tests for GTE inverse and MeshReconstructor. 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 implements the end-to-end PS1 mesh reconstruction pipeline: capturing frame primitives into thread-safe snapshots, transforming screen-space coordinates to world-space, reconstructing geometry grouped by matrix and texture, attaching Ogre meshes to the live scene, and integrating user actions via the session toolbar. ChangesPS1 Runtime Geometry Extraction and Mesh Attachment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: fdf4fe6ee5
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/PS1/runtime/MeshReconstructor.cpp (2)
91-100: ⚡ Quick winSprite reconstruction uses hardcoded offset and may produce incorrect geometry.
The sprite is created by duplicating vertices and adding a fixed 0.05f Y offset, resulting in a thin strip rather than a proper quad. PS1 sprites typically have width/height data that should determine the second pair of vertices. This hardcoded offset won't scale with sprite dimensions.
Is this intentional placeholder geometry, or should sprite dimensions be extracted from
PrimRecordfields (e.g., width/height stored in unused vertex slots or separate fields)?🤖 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/MeshReconstructor.cpp` around lines 91 - 100, The sprite branch in MeshReconstructor (the PrimKind::Sprite block using prim.vertexCount, vtx(), ReconstructedVertex and acc.addTriangle) uses a fixed 0.05f Y offset to create a quad, which is wrong — replace the hardcoded offset with proper sprite dimensions: read width/height from the PrimRecord (or from the unused vertex slots if your format stores size there), compute the four quad vertices around the sprite center using those width/height values (and respect sprite orientation if available), and then call acc.addTriangle on the two triangles built from those computed vertices instead of duplicating a/b and adding 0.05f. Ensure you use the original vertex position/normal/uv when constructing the new corners so scale and texture mapping stay correct.
125-125: 💤 Low valueRemove unused
subIndexvariable.The variable
subIndexis incremented but never read, resulting in dead code.🧹 Proposed fix
- int subIndex = 0; for (auto matIt = groupsByMatrix.constBegin(); matIt != groupsByMatrix.constEnd(); ++matIt) { for (auto texIt = matIt.value().constBegin(); texIt != matIt.value().constEnd(); ++texIt) { const SubMeshAccumulator &acc = texIt.value(); if (acc.vertices.isEmpty() || acc.indices.size() < 3) continue; ReconstructedSubMesh sub; sub.materialName = acc.materialName; sub.vertices = acc.vertices; sub.indices = acc.indices; result.subMeshes.append(sub); result.vertexCount += acc.vertices.size(); result.triangleCount += acc.indices.size() / 3; - ++subIndex; } }Also applies to: 139-139
🤖 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/MeshReconstructor.cpp` at line 125, Remove the unused dead variable `subIndex` from MeshReconstructor.cpp: delete the declaration "int subIndex = 0;" and any subsequent increments or references to `subIndex` (the occurrences around the current locations ~125 and ~139) so the code no longer contains an incremented-but-never-read variable; ensure no other logic depends on `subIndex` after removal.
🤖 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/runtime/GteInverse_test.cpp`:
- Around line 5-15: Add a deterministic Google Test unit that exercises
GteInverse::screenToModel (not only psxScreenToWorld): construct a non-trivial
input with a non-zero translation/offset and known projection parameters, call
GteInverse::screenToModel, then assert the outputs are finite (use
std::isfinite) and match expected values within a small tolerance; include at
least one case that verifies the inverse projection returns a non-zero model
coordinate and one deterministic comparison (EXPECT_NEAR) to validate
correctness. Reference the GteInverse::screenToModel function and keep the test
in the existing GteInverse_test.cpp file following the naming pattern
TEST(GteInverseTest, ScreenToModelDeterministic). Ensure you set up any required
projection/state the same way the implementation expects before calling
screenToModel.
In `@src/PS1/runtime/GteInverse.cpp`:
- Around line 18-27: The inverse unprojection currently applies only the
rotation part (matrix.rt) to ir[] and then scales to mx/my/mz, omitting the
translation part; update the computation in GteInverse.cpp to incorporate the
matrix translation (e.g., matrix.tr or TR field) into vx,vy,vz before applying
kFixedScale so screenToModel respects RT/TR/OFX/OFY/H. Specifically, after
accumulating the rotation terms (the loop using matrix.rt.m[c][0..2] and ir[c]),
add the translation components (matrix.tr.x/y/z or matrix.TR[0..2]) to vx, vy,
vz (with the correct sign if conventions require) and then compute mx/my/mz as
before using kFixedScale. Ensure references to mx, my, mz, matrix.rt and
matrix.tr are updated consistently.
In `@src/PS1/runtime/MeshReconstructor.h`:
- Around line 14-16: The default normal components in MeshReconstructor (nx, ny,
nz) are incorrectly initialized with ny = 1.0f, which biases lighting; change
the default values so nx, ny, nz are all 0.0f to match the Phase 4 mesh
contract. Locate the variables nx, ny, nz in MeshReconstructor.h and set their
initializers to 0.0f (keeping the same variable names) so the mesh normals are
zeroed by default.
In `@src/PS1/runtime/PS1RipMeshBuilder.cpp`:
- Around line 145-151: When entity creation fails (entity == nullptr) the
function currently returns false without populating errorOut; update the return
path to set an explanatory message in errorOut (e.g., "failed to create entity
for mesh" plus any context like node or mesh id) before returning, and if
resultOut is expected to be used on failure ensure it is either cleared or not
written; modify the block around resultOut/return to assign errorOut->message
(or appropriate field) when entity is null so callers receive a non-empty error.
- Around line 29-33: The material enables lighting while the mesh builder uses a
uniform normal, causing incorrect shading: change the material setup in
PS1RipMeshBuilder (the code that gets Ogre::Pass via
mat->getTechnique(0)->getPass(0) and calls
pass->setLightingEnabled(true)/setVertexColourTracking(...)) to disable lighting
for reconstructed PS1 capture materials (call setLightingEnabled(false)) or
alternatively compute and assign proper per-face/per-vertex normals in the mesh
construction routines so lighting is correct; also wrap all PS1-specific code in
this file (including PS1RipMeshBuilder class/functions and related material
setup) with `#ifdef` ENABLE_PS1_RIP / `#endif` to follow the optional feature
dependency guideline.
In `@src/PS1/runtime/PS1RipSessionWindow.cpp`:
- Around line 56-60: The "Arm Capture" QAction (armCapAct) can remain checked
when PS1RipManager auto-disarms (e.g., on session stop); add a connection from
PS1RipManager's relevant state-change signal (e.g., a signal such as
armedChanged(bool) or sessionStopped()) to update the QAction checked state so
the UI follows the manager. Concretely, after creating armCapAct and connecting
its toggled->m_manager->armCapture, also connect m_manager's disarm/session-stop
signal to a slot or lambda that calls armCapAct->setChecked(false) (or
setChecked(armed) if an armedChanged(bool) exists); apply the same pattern for
the duplicate block around lines 77-82 where another arm action is created.
In `@src/PS1/runtime/PS1RipWorker.cpp`:
- Line 190: Add a Sentry breadcrumb immediately before the emit of the snapshot:
call SentryReporter::addBreadcrumb(...) with metadata including captureId and
primCount (use prims.size() as primCount) right before the emit
frameCaptureReady(captureId, CaptureSnapshot::fromBuffer(*m_captureBuffer),
prims.size()) line in PS1RipWorker.cpp so the capture completion is recorded;
ensure the breadcrumb uses an appropriate category (e.g., "capture" or
"user.action") and includes captureId and primCount as string values.
---
Nitpick comments:
In `@src/PS1/runtime/MeshReconstructor.cpp`:
- Around line 91-100: The sprite branch in MeshReconstructor (the
PrimKind::Sprite block using prim.vertexCount, vtx(), ReconstructedVertex and
acc.addTriangle) uses a fixed 0.05f Y offset to create a quad, which is wrong —
replace the hardcoded offset with proper sprite dimensions: read width/height
from the PrimRecord (or from the unused vertex slots if your format stores size
there), compute the four quad vertices around the sprite center using those
width/height values (and respect sprite orientation if available), and then call
acc.addTriangle on the two triangles built from those computed vertices instead
of duplicating a/b and adding 0.05f. Ensure you use the original vertex
position/normal/uv when constructing the new corners so scale and texture
mapping stay correct.
- Line 125: Remove the unused dead variable `subIndex` from
MeshReconstructor.cpp: delete the declaration "int subIndex = 0;" and any
subsequent increments or references to `subIndex` (the occurrences around the
current locations ~125 and ~139) so the code no longer contains an
incremented-but-never-read variable; ensure no other logic depends on `subIndex`
after removal.
🪄 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: a6d1bace-d5f1-4b46-abc5-71443f96b878
📒 Files selected for processing (19)
src/PS1/CMakeLists.txtsrc/PS1/PS1_RIP_DESIGN.mdsrc/PS1/runtime/CaptureSnapshot.cppsrc/PS1/runtime/CaptureSnapshot.hsrc/PS1/runtime/GteInverse.cppsrc/PS1/runtime/GteInverse.hsrc/PS1/runtime/GteInverse_test.cppsrc/PS1/runtime/MeshReconstructor.cppsrc/PS1/runtime/MeshReconstructor.hsrc/PS1/runtime/MeshReconstructor_test.cppsrc/PS1/runtime/PS1RipManager.cppsrc/PS1/runtime/PS1RipManager.hsrc/PS1/runtime/PS1RipMeshBuilder.cppsrc/PS1/runtime/PS1RipMeshBuilder.hsrc/PS1/runtime/PS1RipSessionWindow.cppsrc/PS1/runtime/PS1RipSessionWindow.hsrc/PS1/runtime/PS1RipWorker.cppsrc/PS1/runtime/PS1RipWorker.htests/CMakeLists.txt
Separate model-space vs screen-space transforms, include TR in GTE inverse, disable lighting on capture materials, and sync Arm Capture UI on stop. Co-authored-by: Cursor <cursoragent@cursor.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CaptureSnapshot,GteInverse,MeshReconstructor, andPS1RipMeshBuilderto turn captured primitives into Ogre meshesmatrixId+ texture key; triangulate quads; vertex color + UV; PS1 Y-down → editor Y-upcaptureFrameimportsPS1Capture_<id>into the main scene when the editor is openps1.rip.mesh.builtTest plan
UnitTests --gtest_filter="MeshReconstructorTest.*:GteInverseTest.*"PS1RipManagerTest.ArmedCaptureAccumulatesPrimitivesENABLE_PS1_RIP=ONPS1Capture_*node in scene treeCloses #422 (epic #412 Phase 4).
Made with Cursor
Summary by CodeRabbit
Release Notes