Repository navigation
feat(ngs2): allow the custom UserFx module on custom sampler racks - #14379
Conversation
|
Please drop the new |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The PR body says that index 0 and any index past the module count on the custom sampler rack return INVALID_PARAM_ID, but the code throws std::invalid_argument, which sceNgs2VoiceControl does not turn into a return code, and the new test pins that exception.
What I ran
Exact head 6a4b61d6c29e08aa814c6fe688f3533e26a041d5, base main e67cc818696fd6e6c58ac80adc6550c54aed68e8 (merge-base e6f57307fa7f47165d7fc1bee945abdb2f6816e7; merge preview clean). Environment: Debian 12, Python 3.11.2, git 2.39.5. No build, no compiler, no CTest.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14379
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base e6f57307fa7f47165d7fc1bee945abdb2f6816e7 --head 6a4b61d6c29e08aa814c6fe688f3533e26a041d5 --json
python3 tools/pr_test_quality.py --root <new directory with git archive of the head> --json core/libs/prx/libSceNgs2.native/tests/Ngs2Custom.cppThe precheck's ABI and test-quality helpers could not find their scripts in its isolated head tree, so I ran both helpers by hand as listed. I also read the head versions of Voice.cpp, Custom.cpp and Ngs2Custom.cpp.
Results
| Check | Result |
|---|---|
| Conventions and PR template (precheck) | Passed. |
git diff --check (precheck) |
Passed. |
Live stack and merge preview against main (precheck) |
Clean. |
pr_abi_audit.py (by hand) |
Passed with warnings: 8 symbols, lexical public-declaration-not-found on internal names such as Cleanup, ControlHandler and CountAlloc. The diff adds no exported symbol. |
pr_test_quality.py (by hand) |
One heuristic finding, possible-unregistered-cpp-test on Ngs2Custom.cpp. False positive: the file is registered as ngs2_custom_tests by the ngs2Test loop in core/libs/prx/libSceNgs2.native/CMakeLists.txt, which includes Custom. |
| Source reading: exception path | sceNgs2VoiceControl catches only Ngs2ReturnCode. Index 0 and out-of-range indexes on the sampler path throw std::invalid_argument. See Blocking. |
| Hosted CI (precheck snapshot) | Passed: eligibility, intake, library-portability, report. Pending: conventions, labels, windows. Skipped: linux, register-context, relinker, relinker-asan, write-watch. |
| Build and NGS2 tests | Not run. |
Unedited command output
$ python3 tools/pr_test_quality.py --root <archive> --json core/libs/prx/libSceNgs2.native/tests/Ngs2Custom.cpp
findings: possible-unregistered-cpp-test (core/libs/prx/libSceNgs2.native/tests/Ngs2Custom.cpp, line 1)
$ python3 tools/pr_abi_audit.py ... --json
status: passed_with_warnings; symbols: 8; findings: public-declaration-not-found (Cleanup, ControlHandler, CountAlloc, ...)
Findings
Blocking:
-
The PR body and the code disagree on the value the guest receives. The PR body says that on custom sampler rack
0x4001, "index 0 and any index past the module count returnINVALID_PARAM_ID(0x804A835A)". The code does not return that code. Index 0 hitsif (index == 0) APS5_INVALID_ARG_EX;(Custom.cpp#L384). An index past the module count hits the bounds check inSetCustomUserFxParam(Custom.cpp#L237). Both useAPS5_INVALID_ARG_EX, which throwsstd::invalid_argument(General.hpp#L106).sceNgs2VoiceControlcatches onlyNgs2ReturnCode(Voice.cpp#L1104), so the exception leaves the API and no return code reaches the guest. The new test pins the exception: Ngs2Custom.cpp#L831-L832 useControlThrows<std::invalid_argument>. The same file already returnsNgs2ReturnCode{SCE_NGS2_ERROR_INVALID_PARAM_ID}for out-of-range indexes on the filter path (Custom.cpp#L345 and #L350).If the firmware reading in the PR is right, the emulator's result differs from the console for these two calls. If the exception is what was intended, the PR body and the debt entry are wrong. Smallest fix in the first case, to match the body and the filter path (not compiled):
if (rackId == SCE_NGS2_RACK_ID_CUSTOM_SAMPLER) {
if (index == 0 || index - 1 >= voice.customUserFx.size()) throw Ngs2ReturnCode{SCE_NGS2_ERROR_INVALID_PARAM_ID};
index--;
}Then assert the returned code in Ngs2Custom.cpp instead of ControlThrows<std::invalid_argument>. In the second case, correct the sentence in the PR body and the custom UserFx entry in libSceNgs2.native.md.
Non-blocking: The test-scope points in oneandonlydean's comment of 2026-11-01 11:03 (the non-null option assertion at Ngs2Custom.cpp:833 and the retargeted 0x99 module check) are not repeated here; this review covers only the return-code question above.
Sources
- sceNgs2VoiceControl catches only Ngs2ReturnCode
- Index-0 throw on the sampler path
- Filter path that returns INVALID_PARAM_ID
- Test that expects std::invalid_argument
Not verified
- The NGS2 test binaries (
ngs2_custom,ngs2_userfx) were not built or run here. The author reports a Linux GCC run; it is not independently verified. - The fma-rc 12.70 module reading that the PR cites, including the index offset and the
0x804A835Acode, was not checked. - Windows and macOS were not run.
To get this to a good state
Choose one of the two options under Blocking. For the code option, rerun ngs2_custom and ngs2_userfx on Linux, and the same no-build checks, on the new head.
|
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.
|
|
Addressed both reviews on
Verified on this machine (Linux, GCC 16.2.1):
AI-assisted: yes |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — no blocking findings
The custom UserFx module is now accepted on a custom sampler rack, and the voice-parameter index is shifted by one only on that rack, with index 0 and out-of-range indices rejected with INVALID_PARAM_ID; the code paths are consistent with each other and the test is updated to match, but the index rule itself rests on an external reading of firmware 12.70, which I could not check here.
What I ran
- Exact head
4e5c7a5d759e6435ce633b52bc06a0941d55b5c7, basee67cc81869. - Environment: Linux,
/usr/bin/python33.11.2, no build. - Precheck:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14379 --output-dir /dev/shm/reviewer-run/precheck/14379 - ABI audit by hand (helper absent from the head tree):
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base e67cc81869 --head 4e5c7a5d759e6435ce633b52bc06a0941d55b5c7 --json(exit 0;passed_with_warnings; 8 findings, allpublic-declaration-not-foundfor callbacks defined in the test and module source, not in a header; 8 symbols). - Test-quality helper on
core/libs/prx/libSceNgs2.native/tests/Ngs2Custom.cppfrom a head extract: one literal-path registration heuristic. - Source read at head:
Ngs2CheckCustomRackandNgs2ApplyCustomParamincore/libs/prx/libSceNgs2.native/src/Custom.cpp(around lines 78-88 and 376-392) and the rewritten test cases inNgs2Custom.cpp.
Results
Precheck report, checks table, unedited (`/dev/shm/reviewer-run/precheck/14379/report.md`):
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check e67cc818696fd6e6c58ac80adc6550c54aed68e8...4e5c7a5d759e6435ce633b52bc06a0941d55b5c7 |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base e67cc818696fd6e6c58ac80adc6550c54aed68e8 --head 4e5c7a5d759e6435ce633b52bc06a0941d55b5c7 --title feat(ngs2): allow the custom UserFx module on custom sampler racks --body /dev/shm/reviewer-run/precheck/14379/pr-body.md |
| changed_export_abi_audit | failed | /usr/bin/python3 tools/pr_abi_audit.py --base e67cc818696fd6e6c58ac80adc6550c54aed68e8 --head 4e5c7a5d759e6435ce633b52bc06a0941d55b5c7 --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14379 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14379/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /usr/bin/python3 /dev/shm/reviewer-run/precheck/14379/head-4e5c7a5d759e-4372fcc2/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14379/head-4e5c7a5d759e-4372fcc2 --json core/libs/prx/libSceNgs2.native/src/Custom.cpp core/libs/prx/libSceNgs2.native/tests/Ngs2Custom.cpp docs/dev/debt/libSceNgs2.native.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
Negative control
Not run here. The C++ test needs a build; on base the sampler-rack UserFx cases throw, which is what the updated test expects to change.
Findings
Blocking: none.
Non-blocking:
- The off-by-one index for the sampler rack (the voice parameter index is one higher than the module index) is the PR's reading of the 12.70 module. It cannot be checked here, and the debt entry should name the source.
- The ABI audit's eight findings are the lexical class seen before (callbacks marked
APS5_VABIin test or module source without a header declaration); they are not exports.
Sources
- Rack check at head:
AnyPS5/core/libs/prx/libSceNgs2.native/src/Custom.cpp
Lines 78 to 88 in 4e5c7a5
- Parameter routing at head:
AnyPS5/core/libs/prx/libSceNgs2.native/src/Custom.cpp
Lines 376 to 392 in 4e5c7a5
Not verified
- The firmware 12.70 index semantics cited in the PR were not checked.
- The C++ test was not built or run here.
- Hosted: at this head
conventions,eligibility,labels,library-portabilityandreportsucceeded;linux,register-context,relinker,relinker-asanandwrite-watchwere skipped.
|
Re-verified on Re-ran locally on this head: |
|
@MarcZX10 the return-code and test-scope items are addressed on |
|
Windows result for head Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release,
AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
|
Checked the sampler-rack UserFx index mapping (+1, index 0 and past-the-count return |
What
Accept the custom UserFx module (0x19) on custom sampler racks (0x4001). The rack check allows it, and the voice parameter follows the module as fma-rc reads the 12.70 module: on a custom sampler rack its index is one higher than the module index, so index 0 and any index past the module count return
INVALID_PARAM_ID(0x804A835A). The sampler's single buffer is used; multi-buffer routing remains unimplemented. The existing legacy UserFx callback path is reused, with no option or module state.The test also picks up two current-main requirements: a custom sampler rack must set
max_waveform_blocks, and the legacy UserFx control marks the changed parameter, so the first process call reports flags 3, as the custom submixer case does.Tested
ngs2_custom,ngs2_gain,ngs2_routingandngs2_userfx4/4 passed; the fivengs2_core_custom_safety_*cases 5/5 passed.python3 tools/check_conventions.py --base origin/main: passed.git diff --check: clean.python3 tools/pr_overlap.py --branch feat/ngs2-custom-sampler-userfx --jobs 32: 14 shared files/exports; the comparisons it could not complete are PRs that conflict withmain(feat(ngs2): implement the custom Reverb module #10599 Reverb, fix(ngs2): return documented Delay option errors #12107 Delay, feat(ngs2): copy custom UserFx source to dest across multiple buffers #12673 multi-buffer UserFx and others). None implements this custom-sampler UserFx path. feat(ngs2): copy custom UserFx source to dest across multiple buffers #12673 touches the sameCustom.cppand test but is a different multi-buffer submixer feature, stacked on feat(ngs2): implement the custom Reverb module #10599.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)