Repository navigation
feat(ngs2): implement the custom Reverb module - #10599
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@hugodearaujo @JJenkx @JAB-dev — coordinating the overlap reported on this draft before marking it ready. Proposed order:
@TaymKal @TibixDev — the checker cannot compare #6044/#1384 because they already conflict with main. Manual review finds test categorization in #6044 and the older built-in reverb DSP in #1384. When rebasing, the custom-Reverb regression needs the media category after #6044, and #1384 needs to preserve main's reference-based DSP plus the custom profile here. Please flag any additional semantic overlap. I am preparing a local integration preview; this comment does not claim an agreed order or that the combined branches have passed yet. The draft will stay in place while coordination is pending. |
7f15811 to
bf8242f
Compare
|
Integration preview update for the landing-order proposal above:
The tested resolution is retained locally for the rebase after #10347 lands. |
bf8242f to
022c609
Compare
|
#10347 is now merged. Rebased this PR onto main On the rebased source, Removed the fulfilled #10347 dependency. The fresh overlap scan reports only the external test-registration conflicts with #5548/#9926, plus the already reviewed stale #1384 comparison. Its self-match against the old head of #10599 is this PR before the force-push, not a separate implementation. The resolved combined test-list preview is retained locally; confirmation of the remaining landing order is still pending, so the PR remains draft. |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed updated head 022c609f947fef3fa0bc4f205f7a2aec75980556. The #10347 integration prerequisite is now merged and the body records that its channel folding and mapped dry path remain alongside custom Reverb. git diff --check upstream/main...HEAD passes. I reviewed the rebase and the custom state lifecycle (module creation, parameter activation, PLAY reset, setup reset and sampler completion paths); I found no new standalone source issue in this update.
The 4/4 results per macOS architecture and the combined-preview results are author-reported; I did not independently run these focused production-source fixtures. The PR remains draft while agreement on the test-registration ordering with #5548 and #9926 is pending. Please retain all test registrations when the later branch is rebased. The stale #1384 comparison also needs a fresh check if that PR's source changes are brought into the integration tree.
|
Merged current main The description now reflects the actual ready-for-review state and distinguishes this rerun from the earlier ARM64 results. The existing test-registration landing-order notes remain; no unpublished PitchShift/scheduler work is included. No Linux/Windows or game run claimed. AI-assisted: yes. |
|
@kyon-eth Please remove the new production Voice.cpp include of tests/Ngs2Test.hpp and use the appropriate production public/internal declarations. That fixture brings test helpers, a Require macro and an inline buffers registry into the shipped library. Keep the built-in channel-folding/mapped-dry behavior and custom-Reverb regressions, then rerun ngs2_customreverb, ngs2_custom, ngs2_render and ngs2_core_render on Linux and Windows. The pending test-registration order with #5548/#9926 remains a separate coordination item. |
Register both new Ngs2 tests: this PR's CustomReverb and main's VoiceEvents.
|
A title that stops on this module: BLUE REFLECTION Quartet (PPSA33655). Its bring-up thread #10271 gives the current blocker as "libSceNgs2: the custom Reverb module is not implemented", which is the text This PR's checklist has |
|
Tested this head ( A local probe that only removed the custom-mastering refusal (no implementation) shows what the title creates there: one buffer, one input and four modules, all with buffer id 0 and no option: |
|
Fixed in f108656. Voice.cpp now includes the production public API header, with no test fixture/header in the shipped source. Integrated main fe2b52c, preserving its Gain/routing safeguards, built-in channel folding, mapped dry output, custom-Reverb state and all test registrations. The requested ngs2_customreverb, ngs2_custom, ngs2_render and ngs2_core_render tests pass in the rebuilt macOS x86-64 production-source fixture, alongside the seven related Gain/routing safety cases (11/11). I cannot run Windows/Linux here. The broader fixture’s unrelated ngs2_parse filesystem-adapter failure is not credited as a pass. Thanks for the independent Linux/title report: Project Zero PPSA04333 gets past Reverb and reaches custom mastering instead. The description records that narrower result and BLUE REFLECTION Quartet’s reported blocker, without claiming either title is playable. Maintainers may choose the independent test-registration landing order; later rebases must retain both sets of tests. Could Linux/Windows testers rerun the four requested tests on this updated head? The final main advancement changes no tested implementation source bytes. |
|
Integrated current main 9ae2f7d in 61908b1. The requested Reverb/custom/render/core-render tests and related Gain/routing safety cases pass 11/11 in the rebuilt macOS x86-64 production-source fixture. Conventions, whitespace and privacy checks pass. The latest discussion has no new code-fix request; this update adds no unrelated VAG, PitchShift, Json2 or allocator work. No new Linux/Windows execution or complete-title unblock is claimed. |
|
Title run of this PR's exact head BLUE REFLECTION Quartet (PPSA33655), the title from #10271 that stops on
Also: Trails in the Sky 2nd Chapter (PPSA35169) reaches this PR's module once #11315 handles its PitchShift: comment on #11315. The two PRs conflict in Staging: AI-assisted: yes (Claude Code). |
|
Follow-up with the exact rack that BLUE REFLECTION Quartet (PPSA33655) is refused on, after this PR lets its Reverb rack through. I got it from a private diagnostic print at the start of
So the next step for this title is a two-input custom submixer whose second UserFx module takes buffer 1 as its extra buffer and writes buffer 0. I can't tell from this run whether that is a sidechain or a mix. This is outside this PR's scope; I am only adding it as data for whoever lifts the multi-buffer restriction. AI-assisted: yes (Claude Code). |
|
Independent validation of head
For Trails in the Sky 2nd (#10156) this is the module that aborts after #11315's PitchShift — note #11315 rewrites the same |
# Conflicts: # core/libs/prx/libSceNgs2.native/CMakeLists.txt # core/libs/prx/libSceNgs2.native/src/Custom.cpp # core/libs/prx/libSceNgs2.native/src/Voice.cpp # docs/dev/debt/libSceNgs2.native.md
|
@kyon-eth thanks for merging One thing left from the merge: the debt entry "NGS2 custom module ids" in |
# Conflicts: # core/libs/prx/libSceNgs2.native/src/Voice.cpp
✅ AMD GPU test: passedPR head
Failing on main too (3, not counted against this PR)Validation IDs already present on main (5 IDs in 364 tests)Skipped (3)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Tested on Project Zero: Mask of the Lunar Eclipse (PPSA10278), Windows 11, RTX 4070 SUPER, Release, this PR's head
So this PR removes that stop for this title, but it does not unblock it. The sound was not checked, because the game stops before any audio could be judged. One run each. AI-assisted (Claude Code). |
|
A second title with the same result: Project Zero: Maiden of Black Water (PPSA04334), same PC as my earlier comment (Windows 11, RTX 4070 SUPER, Release). The game is built with
So this PR gets this title past its Reverb rack too, and it then stops at the same next check as Mask of the Lunar Eclipse ( AI-assisted: yes (Claude Code) |
What
Custom racks containing Reverb (
0x1b) currently throw before processing audio. Implement the module for the existing supported one-buffer custom sampler and submixer paths, with independent state per voice and module, both creation sizes, indexed I3DL2 controls, channel exclusion and normalized input folding.Reuse the existing reverb DSP with the custom module's ring sizes, signed wet/dry gains and four-sample room filter. Preserve bypass until parameters are supplied and after setup; clear history on a fresh PLAY while retaining parameters. Sampler completion still ends its voice. Reuse working buffers across grains.
Multiple-buffer routing, custom mastering modules, unsupported rates and NaN controls remain explicit refusals, documented in the library debt file. No complete title unblock is claimed.
Integration status
#10347 has merged. This branch incorporates main
fe2b52c7e4and resolves the shared DSP conflict by retaining input-channel folding and mapped dry output alongside the custom profile, four-sample filter and reusable scratch buffers. It has no outstanding functional dependency on #10347.#5548 and #9926 remain open. Their test-registration conflicts are covered by a separate local integration preview that retains every test and passes 17/17 NGS2 checks on ARM64 and seven focused checks on x86-64. Those branches are not included in this PR. Their authors were asked to confirm that maintainers may choose their order, with the later branch preserving both registrations; confirmation is pending in the coordination thread.
Manual review of #1384/#6044: the former contains the older experimental built-in DSP and must preserve main's reference-based DSP and this custom profile when rebased. The latter changes test categorization; the new regression must receive the media category if it lands first. Their authors have been notified.
This PR is ready for review. Maintainers may choose the landing order; the later branch must preserve both test registrations. Author confirmation on that ordering remains pending. The diff contains only custom-Reverb work.
Tested
Current-main validation (2026-10-27): integrated main
9ae2f7d22ain61908b16ac. The rebuilt macOS x86-64 production-source fixture passes the requested Reverb/custom/render/core-render tests and related Gain/routing safety cases (11/11). Conventions, whitespace and privacy checks pass. No new Linux/Windows execution or complete-title unblock is claimed.Updated onto main
fe2b52c7e4; all four requested rendering/custom checks and seven related Gain/routing safety cases pass in the rebuilt macOS x86-64 production-source fixture (11/11). Earlier ARM64 results are historical.Earlier validation based on main
c9b2660d56: macOS ARM64 and x86-64 (Rosetta): rebuilt the production NGS2 sources using a focused external CMake fixture, the repository's module CMake definitions and pinned local LibAtrac9 dependency. Only unrelated host libc boundaries are adapted; this is not a full application build.ngs2_customreverb,ngs2_custom,ngs2_renderandngs2_core_render: 4/4 pass on each architecture after rebasing, including the merged built-in channel-conversion coverage.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)Review follow-up: production Voice.cpp now uses the public API header, with no test helpers in the shipped library. Integrated main fe2b52c while retaining its Gain/routing safety changes and every test registration. The four requested rendering/custom tests and the seven related Gain/routing safety cases pass in the rebuilt macOS x86-64 production-source fixture (11/11). No new Windows or Linux execution is claimed; the full fixture also has an unrelated filesystem-adapter failure in ngs2_parse.
Independent title evidence: Project Zero: Maiden of Black Water, PPSA04333 passes the previous custom-Reverb refusal on the prior head, then reaches an unsupported custom-mastering module chain. This removes one reported blocker; it does not establish playable gameplay. BLUE REFLECTION Quartet PPSA33655 discussion 10271 reports the same Reverb refusal, but has not been tested with this implementation.
Subsequent independent validation
Native Windows full-build report covers exact published head
61908b16ac: 31/31 NGS2 tests pass on Windows 11. This is the reviewer's execution evidence, not an author Windows run.BLUE REFLECTION Quartet follow-up confirms that the custom-Reverb refusal is removed at this head. The next blocker is an existing two-buffer UserFx submixer configuration. It is outside this one-buffer Reverb change, and no complete-title unblock is claimed. The independent Trails in the Sky 2nd result likewise requires the separate PitchShift PR and subsequent reconciliation.