Skip to content

fix(libSceNgs2.native): accept a reverb voice with different input and output channel counts - #10347

Merged
oneandonlydean merged 3 commits into
boykopovar:mainfrom
hugodearaujo:fix/ngs2-reverb-channel-conversion
Oct 24, 2026
Merged

oneandonlydean merged 3 commits into
boykopovar:mainfrom
hugodearaujo:fix/ngs2-reverb-channel-conversion

Conversation

@hugodearaujo

@hugodearaujo hugodearaujo commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

What

SCE_NGS2_REVERB_VOICE_PARAM_SETUP throws NGS2: reverb channel conversion is not implemented when num_input_channels differs from num_output_channels. The Elder Scrolls V: Skyrim (PPSA03747) sets up its reverb voice with 2 inputs and 8 outputs 19 s into boot, so it stops there.

This PR accepts the conversion:

  • A reverb voice keeps its input count apart from its output count (Ngs2Voice::inputChannels; 0 means the output count, for every other voice). Ports into the voice mix in the input layout, through their matrix or the default channel map.
  • The wet path only needs the two counts. Since e15f164 the reverb folds its input to mono with the PS4 library's fixed gains, and runs its comb selection and allpasses per output channel. The fold now uses the input count, and those stages run for each output channel.
  • The dry signal reaches the output layout through the default channel map. That's the part not checked against the PS4 library, and the new debt entry says so. A stereo input keeps its dry signal on the front left and right, and the other outputs carry only the reverb.
  • Both counts must be 1, 2, 6 or 8 and within the rack's max_channels, as the output count already had to be.

@JAB-dev, your reverb was checked against the PS4 library's output. If your setup can render a 2-in, 8-out voice, that would settle the dry path. #1384, a draft from before e15f164, also kept a separate input count, on its own reverb.

Tested

  • Skyrim (PPSA03747 01.001.010), Linux, RX 9070 XT with RADV (Mesa 26.2.5), Release, X pressed every 30 s:
  • ngs2_render and ngs2_core_render, in Ngs2Render.cpp:
    • TestReverbConversion: a 2-in, 6-out voice and a 2-in, 8-out voice (the title's layout) give the same front left and right as a 2-in, 2-out voice on the same input, wet and dry. Their centre, surrounds and backs carry the reverb, and their LFE stays silent. With the wet level at 0, a 6-in, 2-out voice gives L + C * sqrt(2)/2 + Ls/2 and R + C * sqrt(2)/2 + Rs/2.
    • TestReverbSetup: 1 input and 2 outputs is accepted. Input or output counts of 3 or 7 are rejected with the invalid-argument error, as equal counts of 3, 4, 5 and 7 already were.
    • Red: with main's Voice.cpp, both executables abort with NGS2: reverb channel conversion is not implemented.
  • All 13 ngs2_* tests pass. Linux (Arch, kernel 7.2.11), GCC 16.2.1, Release.
  • tools/check_conventions.py passes.
  • Not tested on Windows or macOS.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: PPSA03747 (no Game bring-up discussion yet)
  • GPUs tested: n/a
  • AI-assisted: yes

@github-actions github-actions Bot added unblocks title Names a title ID on the Unblocks line docs prx labels Oct 23, 2026
@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 1384 core/libs/prx/libSceNgs2.native/src/Ngs2Internal.hpp
core/libs/prx/libSceNgs2.native/src/Render.cpp
core/libs/prx/libSceNgs2.native/src/Reverb.cpp
core/libs/prx/libSceNgs2.native/src/Voice.cpp

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the channel-layout path through MixPort, RenderVoice and Ngs2ProcessReverb. The new input count is used for port mixing and the wet mono fold, while output channel state continues to drive the comb/allpass stages; the dry path is remapped before the output channels are processed. The tests cover unequal counts in both directions, input/output validation, wet output distribution and dry downmix, and the author reports the exact Skyrim setup reaching the menu/loading screen. The open #1384 draft is explicitly distinct: it contains the older experimental reverb and itself records that a rework must preserve main’s reference-checked DSP; this PR does preserve that DSP and adds the missing channel conversion. One useful follow-up would be a focused 2-in/8-out render assertion for the exact title layout. The unverified dry mapping is recorded in technical debt. Hosted eligibility, conventions, intake, labels and report checks pass; product jobs are skipped by path policy. I found no blocking issue in this scope.

@oneandonlydean oneandonlydean self-assigned this Oct 23, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

@hugodearaujo Please remove the existing flags-not-implemented throw expectation in TestReverbSetup at Ngs2Render.cpp:769–771. Voice.cpp still treats those flags as unimplemented, so the assertion would fail when support is added. Keep the real channel-count/NaN validation and successful conversion coverage, then rerun the native/core render and remaining ngs2_* tests.

@hugodearaujo

Copy link
Copy Markdown
Contributor Author

@MarcZX10 thanks for the review. The 2-in, 8-out case is in now (1762c1d): TestReverbConversion renders that layout next to the 2-in, 6-out one and checks it the same way. The front left and right match the 2-in, 2-out voice, the other channels carry the reverb, and the LFE stays silent. All 13 ngs2_* tests pass, and the Tested section says so.

@hugodearaujo

Copy link
Copy Markdown
Contributor Author

@oneandonlydean done in 716498d: TestReverbSetup no longer expects the setup flags to throw. The channel-count rejections, the NaN check and the conversion coverage are unchanged. ngs2_render, ngs2_core_render and the other 11 ngs2_* tests pass (Linux, GCC 16.2.1, Release), and tools/check_conventions.py passes.

@oneandonlydean
oneandonlydean merged commit fe0207e into boykopovar:main Oct 24, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs prx unblocks title Names a title ID on the Unblocks line

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants