Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a 'ducking' effect to the currently playing track when changing ruleset #28547

Merged
merged 29 commits into from
Jul 8, 2024

Conversation

nekodex
Copy link
Contributor

@nekodex nekodex commented Jun 21, 2024

A combination volume+filter effect is added to attenuate the currently playing track to allow the ruleset selection samples to be better heard.

Also adds 'sample-choking' logic to prevent overlapping sample playback when rapidly cycling through rulesets.

ruleset-switcher-ducking.mov

The ducking is implemented via new methods added to MusicController, which are generic enough to also replace some AudioFilter usages in the codebase that provide a similar effect.

I originally wrote this as a component ala AudioFilter, but having it coupled with an ITrackStore to do audio adjustments for volume attenuation felt weird, so I instead just implemented them as methods on MusicControler (for now?). After ManagedBass is updated to support the volume effect type (i.e. via peppy/ManagedBass#1 ?), we can do attenuation via a volume effect on the mixer instead of using audio adjustments and that'd allow for a cleaner component.

I also tried adding the ducking effect to other places (f.e. ButtonSystem), but due to audio adjustments being relative/percentage-based, I couldn't get it sounding consistently nice... with music volume at 70% it would sound okay, but at 100% it sounded off, especially with louder songs. I'll look into it again later, but maybe a fixed decibel reduction could work better... but that's for a separate/future PR.

@LittleLilyBun
Copy link

LittleLilyBun commented Jun 22, 2024

It seems the game crashes when switching to a custom ruleset with this PR...

2024-06-21.19-17-05.mp4

I'm not really a developer at all, just looking around, but noticed this issue with this PR and thought I should report it.

Logs: compressed-logs.zip

@bdach bdach self-requested a review June 24, 2024 06:28
osu.Game/Overlays/MusicController.cs Outdated Show resolved Hide resolved
/// <param name="duckVolumeTo">Level to drop volume to (1.0 = 100%).</param>
/// <param name="duckCutoffTo">Cutoff frequency to drop `AudioFilter` to. Use `AudioFilter.MAX_LOWPASS_CUTOFF` to skip filter effect.</param>
/// <param name="easing">Easing for the ducking transition.</param>
public void Duck(int duration = 0, float duckVolumeTo = 0.25f, int duckCutoffTo = 300, Easing easing = Easing.InCubic)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do wonder about this API design. While convenient it's also a bit weak on forcing the caller to clean up after themselves (there's nothing stopping anyone from calling Duck() and then forgetting to call Unduck() later).

The established pattern we sorta have for these is that such transient operations return an InvokeOnDisposal that will call Unduck() when the InvokeOnDisposal is disposed, so rather than do

musicController.Duck();
// later
musicController.Unduck();

you'd do

var duck = musicController.Duck();
// later
duck.Dispose();

Although it wold mean that the unduck parameters would need to be set by Duck() which may be indesirable.

Also food for thought here is that the current API design allows any number of callers to call Duck() and Unduck() in arbitrary order concurrently and the last one will win essentially. With the IDisposable approach MusicController could keep track of who's currently ducking and just not allow anyone else to while that other component is doing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree this API design doesn't feel great, but nothing better came to mind.

Also food for thought here is that the current API design allows any number of callers to call Duck() and Unduck() in arbitrary order concurrently and the last one will win essentially. With the IDisposable approach MusicController could keep track of who's currently ducking and just not allow anyone else to while that other component is doing it.

This was kinda intentional - ideally triggering Duck() and Unduck() repeatedly and arbitrarily should be allowed, with the most recent call taking precedence.

In my mind TimedDuck() should be preferred for most cases anyway (esp. where usages could overlap), with Duck() and Unduck() reserved only for when TimedDuck() can't be used (i.e. when the delay is arbitrary or dependant on the user - like for overlays/popups/etc).

I'll look into InvokeOnDisposal though, enforcing Unduck() after a Duck() is probably worth the clunkiness of passing unduck parameters up front.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was kinda intentional - ideally triggering Duck() and Unduck() repeatedly and arbitrarily should be allowed, with the most recent call taking precedence.

If this was intended to be more of an "immediate" mode API then that is fine. For reference I don't consider this review thread a blocker, just vague musings on API shape at most I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a valid concern though - I found an edge-case where a TimedDuck() is triggered while a Duck() is already in progress, resulting in 'premature' unducking, when it should really remain ducked instead.

osu.Game/Overlays/Dialog/PopupDialogDangerousButton.cs Outdated Show resolved Hide resolved
@peppy peppy self-requested a review July 4, 2024 07:52
Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Initial pass. I will do a more thorough review of the API surface because I definitely have some suggestions there, just don't want to overload a single review.

/// <param name="easing">Easing for the ducking transition.</param>
/// <param name="unduckDuration">Duration of the unducking transition, in ms.</param>
/// <param name="unduckEasing">Easing for the unducking transition.</param>
public IDisposable Duck(int duration = 0, float duckVolumeTo = 0.25f, int? duckCutoffTo = 300, Easing easing = Easing.OutCubic, int unduckDuration = 500, Easing unduckEasing = Easing.InCubic)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I feel like cubic easing is too abrupt in its current form. Can you give a plain In/Out a try? It already feels better to me (also a slight reduction to the unduck delay):

diff --git a/osu.Game/Overlays/MusicController.cs b/osu.Game/Overlays/MusicController.cs
index 8b52c59bae..06845116b1 100644
--- a/osu.Game/Overlays/MusicController.cs
+++ b/osu.Game/Overlays/MusicController.cs
@@ -265,7 +265,7 @@ public void NextTrack(Action onSuccess = null) => Schedule(() =>
         /// <param name="easing">Easing for the ducking transition.</param>
         /// <param name="unduckDuration">Duration of the unducking transition, in ms.</param>
         /// <param name="unduckEasing">Easing for the unducking transition.</param>
-        public IDisposable Duck(int duration = 0, float duckVolumeTo = 0.25f, int? duckCutoffTo = 300, Easing easing = Easing.OutCubic, int unduckDuration = 500, Easing unduckEasing = Easing.InCubic)
+        public IDisposable Duck(int duration = 0, float duckVolumeTo = 0.25f, int? duckCutoffTo = 300, Easing easing = Easing.Out, int unduckDuration = 500, Easing unduckEasing = Easing.In)
         {
             if (audioDuckActive) return null;
 
@@ -292,7 +292,7 @@ public IDisposable Duck(int duration = 0, float duckVolumeTo = 0.25f, int? duckC
         /// <param name="duckCutoffTo">Cutoff frequency to drop `AudioFilter` to. Use `null` to skip filter effect.</param>
         /// <param name="duckDuration">Duration of the ducking transition, in ms.</param>
         /// <param name="duckEasing">Easing for the ducking transition.</param>
-        public void TimedDuck(int delay, int unduckDuration = 500, Easing unduckEasing = Easing.InCubic, float duckVolumeTo = 0.25f, int? duckCutoffTo = 300, int duckDuration = 0, Easing duckEasing = Easing.OutCubic)
+        public void TimedDuck(int delay, int unduckDuration = 500, Easing unduckEasing = Easing.In, float duckVolumeTo = 0.25f, int? duckCutoffTo = 300, int duckDuration = 0, Easing duckEasing = Easing.Out)
         {
             if (audioDuckActive) return;
 
diff --git a/osu.Game/Overlays/Toolbar/ToolbarRulesetSelector.cs b/osu.Game/Overlays/Toolbar/ToolbarRulesetSelector.cs
index 7da6b76aaa..07ce1f69ef 100644
--- a/osu.Game/Overlays/Toolbar/ToolbarRulesetSelector.cs
+++ b/osu.Game/Overlays/Toolbar/ToolbarRulesetSelector.cs
@@ -122,7 +122,7 @@ private void playRulesetSelectionSample(ValueChangedEvent<RulesetInfo> r)
 
             rulesetSelectionChannel[r.NewValue] = channel;
             channel.Play();
-            musicController?.TimedDuck(600);
+            musicController?.TimedDuck(500);
         }
 
         public override bool HandleNonPositionalInput => !Current.Disabled && base.HandleNonPositionalInput;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate on which usage(s) you find to be too abrupt? If it's just the ruleset selector, probably better to change that specific usage rather than the default?

CubicIn/CubicOut matches what the previous implementations (DialogOverlay, etc) used, so they should sound identical to before.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I was testing with the ruleset selector, but I think it might be a better default. I'll check the other usages and loop back.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think non-cubic sounds better for the dialog cases too. There's not much difference since the duration is so short, but it's a little less sharp.

osu.Game/Overlays/MusicController.cs Outdated Show resolved Hide resolved
@peppy peppy self-requested a review July 5, 2024 02:36
@peppy
Copy link
Sponsor Member

peppy commented Jul 5, 2024

I'm hoping that we can simplify the API down for now to follow KISS principles, removing the easing and unduck settings. @nekodex do you have use cases already in mind which would be overriding these (ie. different duck and unduck duration, or different easing curves)?

diff --git a/osu.Game/Collections/ManageCollectionsDialog.cs b/osu.Game/Collections/ManageCollectionsDialog.cs
index 0396fd531c..b60d31ef8d 100644
--- a/osu.Game/Collections/ManageCollectionsDialog.cs
+++ b/osu.Game/Collections/ManageCollectionsDialog.cs
@@ -125,7 +125,7 @@ protected override void Dispose(bool isDisposing)
 
         protected override void PopIn()
         {
-            audioDucker = musicController?.Duck(100, 1f, unduckDuration: 100);
+            audioDucker = musicController?.Duck(100, 1f);
 
             this.FadeIn(enter_duration, Easing.OutQuint);
             this.ScaleTo(0.9f).Then().ScaleTo(1f, enter_duration, Easing.OutQuint);
diff --git a/osu.Game/Overlays/DialogOverlay.cs b/osu.Game/Overlays/DialogOverlay.cs
index 7c52081053..04daabda6c 100644
--- a/osu.Game/Overlays/DialogOverlay.cs
+++ b/osu.Game/Overlays/DialogOverlay.cs
@@ -106,7 +106,7 @@ void dismiss()
 
         protected override void PopIn()
         {
-            audioDucker = musicController.Duck(100, 1f, unduckDuration: 100);
+            audioDucker = musicController.Duck(100, 1f);
         }
 
         protected override void PopOut()
diff --git a/osu.Game/Overlays/MusicController.cs b/osu.Game/Overlays/MusicController.cs
index 54a0436f2a..3e9f801df8 100644
--- a/osu.Game/Overlays/MusicController.cs
+++ b/osu.Game/Overlays/MusicController.cs
@@ -263,13 +263,10 @@ public void NextTrack(Action? onSuccess = null) => Schedule(() =>
         /// <summary>
         /// Attenuates the volume and/or filters the currently playing track.
         /// </summary>
-        /// <param name="duration">Duration of the ducking transition, in ms.</param>
+        /// <param name="duckDuration">Duration of the ducking transition, in ms.</param>
         /// <param name="duckVolumeTo">Level to drop volume to (1.0 = 100%).</param>
         /// <param name="duckCutoffTo">Cutoff frequency to drop `AudioFilter` to. Use `null` to skip filter effect.</param>
-        /// <param name="easing">Easing for the ducking transition.</param>
-        /// <param name="unduckDuration">Duration of the unducking transition, in ms.</param>
-        /// <param name="unduckEasing">Easing for the unducking transition.</param>
-        public IDisposable? Duck(int duration = 0, float duckVolumeTo = 0.25f, int? duckCutoffTo = 300, Easing easing = Easing.OutCubic, int unduckDuration = 500, Easing unduckEasing = Easing.InCubic)
+        public IDisposable? Duck(int duckDuration = 0, float duckVolumeTo = 0.25f, int? duckCutoffTo = 300)
         {
             if (audioDuckActive) return null;
 
@@ -278,31 +275,27 @@ public void NextTrack(Action? onSuccess = null) => Schedule(() =>
             Schedule(() =>
             {
                 if (duckCutoffTo.IsNotNull())
-                    audioDuckFilter?.CutoffTo((int)duckCutoffTo, duration, easing);
+                    audioDuckFilter?.CutoffTo((int)duckCutoffTo, duckDuration, Easing.Out);
 
-                this.TransformBindableTo(audioDuckVolume, duckVolumeTo, duration, easing);
+                this.TransformBindableTo(audioDuckVolume, duckVolumeTo, duckDuration, Easing.Out);
             });
 
-            return new InvokeOnDisposal(() => unduck(unduckDuration, unduckEasing));
+            return new InvokeOnDisposal(() => unduck(duckDuration, Easing.In));
         }
 
         /// <summary>
         /// A convenience method that ducks the currently playing track, then after a delay, unducks it.
         /// </summary>
-        /// <param name="delay">Delay after audio is ducked before unducking begins, in ms.</param>
-        /// <param name="unduckDuration">Duration of the unducking transition, in ms.</param>
-        /// <param name="unduckEasing">Easing for the unducking transition.</param>
+        /// <param name="delayBeforeUnduck">Delay after audio is ducked before unducking begins, in ms.</param>
         /// <param name="duckVolumeTo">Level to drop volume to (1.0 = 100%).</param>
         /// <param name="duckCutoffTo">Cutoff frequency to drop `AudioFilter` to. Use `null` to skip filter effect.</param>
         /// <param name="duckDuration">Duration of the ducking transition, in ms.</param>
-        /// <param name="duckEasing">Easing for the ducking transition.</param>
-        public void DuckMomentarily(int delay, int unduckDuration = 500, Easing unduckEasing = Easing.InCubic, float duckVolumeTo = 0.25f, int? duckCutoffTo = 300, int duckDuration = 0,
-                                    Easing duckEasing = Easing.OutCubic)
+        public void DuckMomentarily(int delayBeforeUnduck, float duckVolumeTo = 0.25f, int? duckCutoffTo = 300, int duckDuration = 0)
         {
             if (audioDuckActive) return;
 
-            Duck(duckDuration, duckVolumeTo, duckCutoffTo, duckEasing);
-            Scheduler.AddDelayed(() => unduck(unduckDuration, unduckEasing), delay);
+            Duck(duckDuration: duckDuration, duckVolumeTo: duckVolumeTo, duckCutoffTo: duckCutoffTo);
+            Scheduler.AddDelayed(() => unduck(duckDuration, Easing.In), delayBeforeUnduck);
         }
 
         private void unduck(int duration, Easing easing)

@nekodex
Copy link
Contributor Author

nekodex commented Jul 5, 2024

I'm hoping that we can simplify the API down for now to follow KISS principles, removing the easing and unduck settings. @nekodex do you have use cases already in mind which would be overriding these (ie. different duck and unduck duration, or different easing curves)?

The ruleset selector has a duck duration of 0 (instant) with a different unduck duration - instant duck to make room for the initial transient of the samples, while having a more gradual unduck to bring the track back in more gently.

Easing settings I'm probably fine with removing for now, it was more forward-thinking to allow for choosing different curves in the future, but I have no specific use cases for it right now (if you're fine with changing dialogs to be non-cubic).

@peppy
Copy link
Sponsor Member

peppy commented Jul 5, 2024

The ruleset selector has a duck duration of 0 (instant) with a different unduck duration - instant duck to make room for the initial transient of the samples, while having a more gradual unduck to bring the track back in more gently.

Hmm, I'm thinking the unduck duration and easing should be locked globally (for momentary ducks), and keeping the parameter as only the duck duration.

@nekodex
Copy link
Contributor Author

nekodex commented Jul 5, 2024

Hmm, I'm thinking the unduck duration and easing should be locked globally (for momentary ducks), and keeping the parameter as only the duck duration.

I get the desire for simplicity, but thinking ahead to future potential usages (for ButtonSystem, etc), I'd definitely prefer to have more control here. Actually, thinking about ButtonSystem makes me think I may miss the ability to have selectable easings... but oh well.

@peppy
Copy link
Sponsor Member

peppy commented Jul 5, 2024

hmm, okay then. i'll refactor the api surface with that in mind (keep full customisation for now).

@peppy
Copy link
Sponsor Member

peppy commented Jul 5, 2024

I've restructured the API but still have a question:

The way this currently works means that if a duck operation is in progress, all further operations will fail. I can see this going very wrong very fast, where say:

  • Two things both want to duck, both try to duck, then the first restored ducking and the game is not ducking when it should be
  • Something like the toolbar ruleset selection is ducking on quick operations, but any duck requests during the duration of the first one will be ignored.

@nekodex what are your thoughts on these? fixing requires first deciding how these cases should be handled (multiplicative ducking? always applying the most extreme ducking?)

@nekodex
Copy link
Contributor Author

nekodex commented Jul 5, 2024

I had considered keeping a list of duck calls and unducking each one respectively on dispose, but that felt somewhat overcomplicated... but maybe that's required for correctness after all.

@nekodex what are your thoughts on these? fixing requires first deciding how these cases should be handled (multiplicative ducking? always applying the most extreme ducking?)

I'm thinking that only applying the most extreme ducking is the correct approach.

The only place I found that currently has overlapping Duck() usages is when triggering a confirmation popup from ManageCollectionsDialog. That dialog is also the only place that allows triggering DuckMomentarily() while a Duck() is in progress - and arguably the ability to do that (clicking Toolbar to change ruleset while the dialog is visible) feels out of place.

In the above scenario, I think it would be weird if the confirmation popup increased the ducking amount when shown (i.e. multiplicative). The ducking parameters of the dialogs are identical, so I'd expect the ducking amount to stay the same?

I also don't think we should make a habit of overlapping Duck() usage, I don't really foresee using it for things other than modal dialogs/overlays anyway?

@peppy
Copy link
Sponsor Member

peppy commented Jul 5, 2024

The other case which already happens with DuckMomentarily in the ruleset selector with fast selection – you can end up getting the duck to end earlier than expected after a few quick ruleset button changes using hotkeys.

@peppy
Copy link
Sponsor Member

peppy commented Jul 5, 2024

@nekodex i've applied changes as proposed (the "deepest" duck is applied, separately for volume and lowpass for the best effect). give it a try and see what you think.

I've also added tests to be able to test this change (dunno how you work without tests 🤷).

peppy
peppy previously approved these changes Jul 5, 2024
@peppy
Copy link
Sponsor Member

peppy commented Jul 5, 2024

@ppy/team-client request a second pair of eyes on this since I made considerable changes.

@frenzibyte frenzibyte self-requested a review July 8, 2024 03:32
Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

I guess it's worth to note that PopupDialogDangerousButton used to gently remove the duck as the progress of holding increases. Now it's removed in this PR and it seems that bringing it back is going to be a nuisance.

Comment on lines 271 to 272
if (duckOperations.Contains(parameters))
throw new ArgumentException("Ducking has already been applied for the provided parameters.", nameof(parameters));
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? It's only guarding the case where the same DuckParameters instance is applied more than once, but is there really an issue with that being the case? The duckOperartions list should be able to correctly hold duplicate entries of the same DuckParameters instance and remove each one correctly based on the number of duck operations disposed, so I don't see what this is for.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

probably okay to remove now, since i changed the structure a bit.

/// The final volume which should be reached during ducking, when 0 is silent and 1 is original volume.
/// Defaults to 25%.
/// </summary>
public float DuckVolumeTo = 0.25f;
Copy link
Member

Choose a reason for hiding this comment

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

Volume fields generally use double type instead of float, I believe mixing both together can result in stupid precision errors if I still remember how floating-type mathematics work.

});
});

AddUntilStep("wait for duck to complete", () => Game.Audio.Tracks.AggregateVolume.Value, () => Is.EqualTo(normalVolume * 0.5f).Within(float.Epsilon));
Copy link
Member

Choose a reason for hiding this comment

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

float.Epsilon is far too precise for this? 0.01 should be sufficient I would think.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

sure, although not sure this changes much?

Copy link
Member

Choose a reason for hiding this comment

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

Just felt weird seeing float.Epsilon used in general, not a show-stopper or something.

@frenzibyte frenzibyte enabled auto-merge July 8, 2024 09:26
@frenzibyte frenzibyte merged commit 2f4e447 into ppy:master Jul 8, 2024
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialogs with dangerous buttons don't have pop in/out and button sounds
6 participants