Skip to content

Add All option for disabled audio events - #259

Open
Guffawaffle wants to merge 3 commits into
STFC-Mod:devfrom
Guffawaffle:feature/audio-disable-all
Open

Guffawaffle wants to merge 3 commits into
STFC-Mod:devfrom
Guffawaffle:feature/audio-disable-all

Conversation

@Guffawaffle

@Guffawaffle Guffawaffle commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Adds case-insensitive All to the existing named audio-event filter: [audio].disabled_events = "All". It suppresses playback-start and resume actions while allowing stop, unload and other cleanup actions through. Exact-name filtering remains available; All takes precedence when combined with names.

Following #313, the existing default-enabled [patches].audioeventhooks controls installation independently of tracing/filter choices. Inactive or null-name callbacks call the original unchanged before converting names; filter-processing failures also forward to native behavior. The one existing Fabric detour resolves the complete unique named-string instance signature, reference arguments and Int32 action enum, and checks installation success. Direct event objects and hashed-ID routes are unaffected. This PR is standalone against dev; no notification or settings stack is imported.

Validation: Windows release build, existing compile-time parser/filter policy assertions and all 11 example TOMLs pass. Three independent reviews cover the exact candidate. Static Windows270 evidence covers the selected Fabric overwrite window; exact-artifact audio runtime and supported Mac extent/execution remain unqualified. CI starts on publication without waiting for results.

@Guffawaffle
Guffawaffle marked this pull request as ready for review August 31, 2026 08:59
@Guffawaffle
Guffawaffle marked this pull request as draft August 31, 2026 09:00
@netniV

netniV commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

I'm not 100% convinced on this one. The idea of an All option brings it in sync with Banners, just don't know that we need all the extra that comes with it.

@Guffawaffle

Copy link
Copy Markdown
Contributor Author

@netniV I'll take a look and see what I can trim on it. Thanks for the feedback!

@Guffawaffle

Copy link
Copy Markdown
Contributor Author

@netniV Good call—the first version overbuilt the All option.

I've trimmed it back to a wildcard over the existing named-string event filter. The Event-object/hashed-ID detours, raw Fabric.Event layout mirror, SPUD target resolver, and unrelated string-helper change are gone. The focused delta is now 67 additions / 17 deletions, down from 218 / 35, and it introduces no additional audio detours.

I also narrowed the documentation so it no longer promises universal audio coverage. Windows runtime testing confirmed that All suppresses the named game-audio stream while the independent mod defeat alert remains audible. The PR body now calls out the bounded scope and pending corrected-head/macOS evidence explicitly. Thanks for pushing on the scope here.

Comment thread mods/src/config.cc Outdated
@Guffawaffle

Copy link
Copy Markdown
Contributor Author

Review refresh complete at 8162591 (receipt b8595cf2e318a3fd1b491ead436c87c3ae38148f37b85fc0db1004b07f419576).

The hostile lane found one low-severity parser defect: the wildcard check passed arbitrary config bytes through C toupper. Commit 8162591 replaces it with an exact, length-aware, allocation-free ASCII comparison and adds compile-time checks for mixed case, embedded NUL, and a high byte. The Windows release mods build and diff-check pass; all three correction lanes now report no confirmed defect.

I also refreshed the focused stack range and removed the incorrect suggestion that string-valued alert_* settings accept false. The PR stays draft for exact-head Windows policy evidence plus macOS native hook-fit/runtime validation.

@Guffawaffle
Guffawaffle force-pushed the feature/audio-disable-all branch from cfe2227 to 4183124 Compare September 24, 2026 02:29
@Guffawaffle

Guffawaffle commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor Author

Hook/feature alignment follow-up for #313

Follow-up to #313: keep hook installation independent of feature enablement, with default-enabled compatibility switches under [patches] and feature checks inside the installed hooks.

Config::Load() currently derives installAudioEventHooks from [audio].trace_events, disabled_events, and the new All state. Preserve upstream's independent [patches].audioeventhooks switch instead. All should affect filtering inside the Fabric hook only; keep the stop/unload cleanup behavior. This PR is standalone: do not pull the notification/settings stack into this correction.

Implemented in bc020dc. Existing audioeventhooks now independently controls the single named Fabric route. Inactive/null and processing-failure paths forward native arguments exactly once; All retains start/resume suppression and cleanup precedence. Complete named ABI/reference/Int32 guards and installation result checks are in place without notification/settings scope. Exact Windows build, parser/policy assertions, examples, static Windows fit and three lanes passed. CI is queued; exact audio/Mac qualification and historical maintainer review state remain open.

@Guffawaffle
Guffawaffle marked this pull request as ready for review October 5, 2026 04:24
@Guffawaffle
Guffawaffle requested a review from netniV October 5, 2026 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants