Repository navigation
fix(ngs2): reset the custom filter selected by the parameter index - #11353
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the changed custom-filter dispatch and the new regression case. The reset path in Ngs2SetFilter uses param.index, so preserving index when zeroing the translated sampler-filter parameter is necessary; the test exercises the regression by keeping filter 0 muted while resetting filter 1. The source diff is consistent with that behavior.
The PR description reports all 16 ngs2_* tests passing on Linux. The hosted checks on this PR did not run the Build test jobs (they are marked skipped), so that result is from the author’s local run rather than CI. I did not independently run the suite in this review.
|
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.
|
|
I manually compared the current heads of #11353 ( |
|
Current main is |
f3803b2 to
ce9e80e
Compare
|
Rebased onto current |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head ce9e80e3cf664bb87169578b61d4e92f954bdfb2 against current main 8100e480f7d55e87afb5376b8f784bf17df0f75c.
The reset branch preserves the selected filter index and the sampler-filter location/type needed for reset dispatch. The new regression mutes filter 0, resets filter 1, and verifies that filter 0 remains muted; current main has no edits to the touched implementation or test paths, and the merge preview is clean. Current-main conventions and git diff --check pass.
The author reports all 16 ngs2_* tests pass on this rebased head. I did not independently run them; hosted eligibility, conventions, labels and report checks pass, while the product/build test jobs are skipped by path policy.
AI-assisted review.
MarcZX10
left a comment
There was a problem hiding this comment.
Follow-up on my approval of exact head ce9e80e3cf664bb87169578b61d4e92f954bdfb2: current upstream main is now 63 commits beyond merge base c3db979cd33a65a0a651f1e7ba7958137a961413. The branch merges cleanly, and my current-head conventions/diff checks pass. The reported 16-test run was on c3db; since then main has changed other NGS2 implementation, test and debt paths, while all product test jobs are skipped by path policy. Please rebase onto current main and rerun the focused NGS2 suite on that exact head. I cannot independently execute the test binary on this host because its CPU preflight requires unsupported instructions.
The current pairwise merge preview with #11354 is clean, so I found no integration conflict between these two heads.
|
Current-main follow-up ( |
|
Rebased onto current |
ce9e80e to
8f2266c
Compare
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 8f2266c against 035ac96. No actionable findings. The reset path zero-initializes its translated filter parameter, then restores the selected index and location before dispatch. The regression creates two filters, mutes filter 0, resets filter 1, and verifies filter 0 remains muted—directly exercising the index-selection bug. I built ngs2_custom_tests in Release with WinLibs GCC 15.2.0 and ran focused CTest: 1 passed, 0 skipped, 0 failed. The exact-head Actions run passed eligibility only; Linux, Windows and the other product jobs were skipped. This verifies the affected test target locally, but is not a run of the full NGS2 suite or console firmware.
8f2266c to
9adfdfa
Compare
|
Rebased onto current |
9adfdfa to
7a4ce8e
Compare
|
Rebased onto current |
7a4ce8e to
278b438
Compare
|
Rebased again onto current |
278b438 to
327cea3
Compare
|
Rebased onto current The earlier check failure came from a stale CMake configuration that did not include the |
|
Rebased automatically onto current |
327cea3 to
67b607c
Compare
|
Rebased automatically onto current |
67b607c to
450348c
Compare
|
Rebased automatically onto current |
450348c to
bcb6ffc
Compare
|
Rebased automatically onto current |
bcb6ffc to
3e1d4e8
Compare
|
Rebased automatically onto current |
4c624a9 to
2e97d98
Compare
|
Rebased automatically onto current |
2e97d98 to
924a52d
Compare
|
Rebased automatically onto current |
924a52d to
605fc8c
Compare
|
Rebased automatically onto current |
605fc8c to
1ed5daa
Compare
|
Windows result for head Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, The tree also carried the open PRs #11724, #11644, #11639, #11636, #11635, #11630, #11347, #11221, #10925, #11706, #11693, #11686, #11682, all merged cleanly, so the run is a joint one; the tests of this PR are AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
|
Rebased automatically onto current |
1ed5daa to
f2255a3
Compare
|
Rebased automatically onto current |
f2255a3 to
dfc5c7c
Compare
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
What
Reset the custom filter selected by the parameter index. When
typeis 0, the code built a zeroedNgs2SamplerVoiceFilterParam, which also clearedindex, soNgs2SetFilteralways reset filter 0. It now keeps the parameter's index and location before asking the filter engine to turn that filter off.Regression:
TestCustomFilterResetIndexcreates two filter modules, mutes filter 0, resets filter 1 and verifies filter 0 still mutes the output. The test fails without this change because filter 0 is reset instead.Tested
Linux, GCC 16.2.1. All 16
ngs2_*tests pass.TestCustomFilterResetIndexpasses with the fix and fails on the previous code.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)