Skip to content

Commit 7e2771c

Browse files
bdachpeppy
bdach
and
peppy
authored
Improve usability of sample bank toggles (#36753)
- [x] Depends on #36741 for merge conflict avoidance RFC, cc @OliBomby ## [Adjust behaviour of automatic bank assignment during placement](547f55e) Diatribe time! This is fallout of the discussion about auto bank in #36705. Auto bank in lazer as written before this commit is confused. On stable, auto bank is closer to "no bank", as in "go look up the current sample timing point, get the bank of that, and use that". lazer has no timing points anymore, but people still want auto bank. So what do? Auto bank for normal samples is somewhat sane still. It only works during placement, and will just copy the normal bank of the previous object - if one exists. That said, one *might not* exist, but the resulting object will still have its normal sample created with `editorAutoBank: true`. That is largely cosmetic and without consequences, but this commit fixes that. Auto bank for *addition* samples, however... Hoo boy. - For placed objects, auto bank means "take the normal sample, read its bank, and use that". Simple enough, right? - Hoooooowever. During placement, auto bank before this commit used to mean "look at the *previous object*, check if it has an addition sound and then use its bank, if not use *the previous object's* normal sample and then use its bank" which is a completely different thing with its own implications. Like, say, what happens if the previous object uses the auto addition bank too? What should be copied over? Should it be the notion of "auto bank" in that the addition bank should match the normal bank, or should it be the literal bank that the previous object is using? This change attempts to define this unambiguously. "Auto additions bank" means "the same bank as the normal bank of this object", full stop. ## [Do not touch sample toggle state if there are no selected objects](052cde5) Fixes issue described in #36705 (comment) wherein opening a sample popover will disable addition bank toggles and toggle off all addition samples. --------- Co-authored-by: Dean Herbert <pe@ppy.sh>
1 parent 5174d8b commit 7e2771c

File tree

4 files changed

+216
-27
lines changed

4 files changed

+216
-27
lines changed

‎osu.Game.Tests/Visual/Editing/TestSceneHitObjectSampleAdjustments.cs‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,172 @@ void checkPlacementSampleAdditionBank(string expected) => AddAssert($"Placement
595595
() => EditorBeatmap.PlacementObject.Value.Samples.First(s => s.Name != HitSampleInfo.HIT_NORMAL).Bank, () => Is.EqualTo(expected));
596596
}
597597

598+
[Test]
599+
public void TestNonAutoBankHotkeysDuringPlacementPersistAfterPlacement()
600+
{
601+
AddStep("Clear all objects", () => EditorBeatmap.Clear());
602+
AddStep("Enter placement mode", () => InputManager.Key(Key.Number2));
603+
AddStep("Move mouse to centre", () => InputManager.MoveMouseTo(Editor.ChildrenOfType<HitObjectComposer>().First().ScreenSpaceDrawQuad.Centre));
604+
605+
AddStep("Move to 3000", () => EditorClock.Seek(3000));
606+
607+
AddStep("Press drum bank shortcut", () =>
608+
{
609+
InputManager.PressKey(Key.ShiftLeft);
610+
InputManager.Key(Key.R);
611+
InputManager.ReleaseKey(Key.ShiftLeft);
612+
});
613+
614+
AddAssert($"Placement sample is {HitSampleInfo.BANK_DRUM}",
615+
() => EditorBeatmap.PlacementObject.Value.Samples.First(s => s.Name == HitSampleInfo.HIT_NORMAL).Bank, () => Is.EqualTo(HitSampleInfo.BANK_DRUM));
616+
617+
AddStep("Press normal addition bank shortcut", () =>
618+
{
619+
InputManager.PressKey(Key.AltLeft);
620+
InputManager.Key(Key.W);
621+
InputManager.ReleaseKey(Key.AltLeft);
622+
});
623+
624+
AddStep("Press finish sample shortcut", () =>
625+
{
626+
InputManager.Key(Key.E);
627+
});
628+
629+
AddAssert($"Placement sample addition is {HitSampleInfo.BANK_NORMAL}",
630+
() => EditorBeatmap.PlacementObject.Value.Samples.First(s => s.Name != HitSampleInfo.HIT_NORMAL).Bank, () => Is.EqualTo(HitSampleInfo.BANK_NORMAL));
631+
632+
AddStep("Finish placement", () => InputManager.Click(MouseButton.Left));
633+
634+
hitObjectHasSamples(0, HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_FINISH);
635+
hitObjectHasSampleNormalBank(0, HitSampleInfo.BANK_DRUM);
636+
hitObjectHasSampleAdditionBank(0, HitSampleInfo.BANK_NORMAL);
637+
hitObjectHasAutoNormalBankFlag(0, false);
638+
hitObjectHasAutoAdditionBankFlag(0, false);
639+
640+
clickSamplePiece(0);
641+
samplePopoverIsOpen();
642+
samplePopoverHasSingleAdditionBank(HitSampleInfo.BANK_NORMAL);
643+
}
644+
645+
[Test]
646+
public void TestAutoAdditionBankHotkeyDuringPlacementPersistsAfterPlacement()
647+
{
648+
AddStep("Clear all objects", () => EditorBeatmap.Clear());
649+
AddStep("Enter placement mode", () => InputManager.Key(Key.Number2));
650+
AddStep("Move mouse to centre", () => InputManager.MoveMouseTo(Editor.ChildrenOfType<HitObjectComposer>().First().ScreenSpaceDrawQuad.Centre));
651+
652+
AddStep("Move to 3000", () => EditorClock.Seek(3000));
653+
654+
AddStep("Press drum bank shortcut", () =>
655+
{
656+
InputManager.PressKey(Key.ShiftLeft);
657+
InputManager.Key(Key.R);
658+
InputManager.ReleaseKey(Key.ShiftLeft);
659+
});
660+
661+
AddAssert($"Placement sample is {HitSampleInfo.BANK_DRUM}",
662+
() => EditorBeatmap.PlacementObject.Value.Samples.First(s => s.Name == HitSampleInfo.HIT_NORMAL).Bank, () => Is.EqualTo(HitSampleInfo.BANK_DRUM));
663+
664+
AddStep("Press normal addition bank shortcut", () =>
665+
{
666+
InputManager.PressKey(Key.AltLeft);
667+
InputManager.Key(Key.W);
668+
InputManager.ReleaseKey(Key.AltLeft);
669+
});
670+
671+
AddStep("Press finish sample shortcut", () =>
672+
{
673+
InputManager.Key(Key.E);
674+
});
675+
676+
AddStep("Press auto addition bank shortcut", () =>
677+
{
678+
InputManager.PressKey(Key.AltLeft);
679+
InputManager.Key(Key.Q);
680+
InputManager.ReleaseKey(Key.AltLeft);
681+
});
682+
683+
AddAssert($"Placement sample addition is {HitSampleInfo.BANK_DRUM}",
684+
() => EditorBeatmap.PlacementObject.Value.Samples.First(s => s.Name != HitSampleInfo.HIT_NORMAL).Bank, () => Is.EqualTo(HitSampleInfo.BANK_DRUM));
685+
686+
AddStep("Finish placement", () => InputManager.Click(MouseButton.Left));
687+
688+
hitObjectHasSamples(0, HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_FINISH);
689+
hitObjectHasSampleNormalBank(0, HitSampleInfo.BANK_DRUM);
690+
hitObjectHasSampleAdditionBank(0, HitSampleInfo.BANK_DRUM);
691+
hitObjectHasAutoNormalBankFlag(0, false);
692+
hitObjectHasAutoAdditionBankFlag(0, true);
693+
694+
clickSamplePiece(0);
695+
samplePopoverIsOpen();
696+
samplePopoverHasSingleAdditionBank(EditorSelectionHandler.HIT_BANK_AUTO);
697+
}
698+
699+
[Test]
700+
public void TestFullAutoBankHotkeyDuringPlacementPersistsAfterPlacement()
701+
{
702+
AddStep("Clear all objects", () => EditorBeatmap.Clear());
703+
AddStep("Enter placement mode", () => InputManager.Key(Key.Number2));
704+
AddStep("Move mouse to centre", () => InputManager.MoveMouseTo(Editor.ChildrenOfType<HitObjectComposer>().First().ScreenSpaceDrawQuad.Centre));
705+
706+
AddStep("Move to 3000", () => EditorClock.Seek(3000));
707+
708+
AddStep("Press auto normal bank shortcut", () =>
709+
{
710+
InputManager.PressKey(Key.ShiftLeft);
711+
InputManager.Key(Key.Q);
712+
InputManager.ReleaseKey(Key.ShiftLeft);
713+
});
714+
715+
AddAssert($"Placement sample is {HitSampleInfo.BANK_NORMAL}",
716+
() => EditorBeatmap.PlacementObject.Value.Samples.First(s => s.Name == HitSampleInfo.HIT_NORMAL).Bank, () => Is.EqualTo(HitSampleInfo.BANK_NORMAL));
717+
718+
AddStep("Press finish sample shortcut", () =>
719+
{
720+
InputManager.Key(Key.E);
721+
});
722+
723+
AddStep("Press auto addition bank shortcut", () =>
724+
{
725+
InputManager.PressKey(Key.AltLeft);
726+
InputManager.Key(Key.Q);
727+
InputManager.ReleaseKey(Key.AltLeft);
728+
});
729+
730+
AddAssert($"Placement sample addition is {HitSampleInfo.BANK_NORMAL}",
731+
() => EditorBeatmap.PlacementObject.Value.Samples.First(s => s.Name != HitSampleInfo.HIT_NORMAL).Bank, () => Is.EqualTo(HitSampleInfo.BANK_NORMAL));
732+
733+
AddStep("Finish placement", () => InputManager.Click(MouseButton.Left));
734+
735+
hitObjectHasSamples(0, HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_FINISH);
736+
hitObjectHasSampleNormalBank(0, HitSampleInfo.BANK_NORMAL);
737+
hitObjectHasSampleAdditionBank(0, HitSampleInfo.BANK_NORMAL);
738+
hitObjectHasAutoNormalBankFlag(0, false); // it's the first object - nothing to inherit bank from
739+
hitObjectHasAutoAdditionBankFlag(0, true);
740+
741+
clickSamplePiece(0);
742+
samplePopoverIsOpen();
743+
samplePopoverHasSingleBank(HitSampleInfo.BANK_NORMAL);
744+
samplePopoverHasSingleAdditionBank(EditorSelectionHandler.HIT_BANK_AUTO);
745+
dismissPopover();
746+
747+
AddStep("Move to 5000", () => EditorClock.Seek(5000));
748+
AddStep("Enter placement mode", () => InputManager.Key(Key.Number2));
749+
AddStep("Move mouse to centre", () => InputManager.MoveMouseTo(Editor.ChildrenOfType<HitObjectComposer>().First().ScreenSpaceDrawQuad.Centre));
750+
AddStep("Finish placement", () => InputManager.Click(MouseButton.Left));
751+
752+
hitObjectHasSamples(1, HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_FINISH); // finish is still implied, continuing from first placement
753+
hitObjectHasSampleNormalBank(1, HitSampleInfo.BANK_NORMAL);
754+
hitObjectHasSampleAdditionBank(1, HitSampleInfo.BANK_NORMAL);
755+
hitObjectHasAutoNormalBankFlag(1, true);
756+
hitObjectHasAutoAdditionBankFlag(1, true);
757+
758+
clickSamplePiece(1);
759+
samplePopoverIsOpen();
760+
samplePopoverHasSingleBank(HitSampleInfo.BANK_NORMAL);
761+
samplePopoverHasSingleAdditionBank(EditorSelectionHandler.HIT_BANK_AUTO);
762+
}
763+
598764
[Test]
599765
public void PopoverForMultipleSelectionChangesAllSamples()
600766
{
@@ -952,6 +1118,14 @@ private void samplePopoverHasIndeterminateBank() => AddUntilStep("sample popover
9521118
return dropdown?.Current.Value == "(multiple)";
9531119
});
9541120

1121+
private void samplePopoverHasSingleAdditionBank(string bank) => AddUntilStep($"sample popover has bank {bank}", () =>
1122+
{
1123+
var popover = this.ChildrenOfType<SamplePointPiece.SampleEditPopover>().SingleOrDefault();
1124+
var dropdown = popover?.ChildrenOfType<LabelledDropdown<string>>().ElementAt(1);
1125+
1126+
return dropdown?.Current.Value == bank;
1127+
});
1128+
9551129
private void dismissPopover()
9561130
{
9571131
AddStep("dismiss popover", () => InputManager.Key(Key.Escape));
@@ -1025,6 +1199,18 @@ private void hitObjectHasSampleAdditionBank(int objectIndex, string bank) => Add
10251199
return h.Samples.Where(o => o.Name != HitSampleInfo.HIT_NORMAL).All(o => o.Bank == bank);
10261200
});
10271201

1202+
private void hitObjectHasAutoNormalBankFlag(int objectIndex, bool autoBank) => AddAssert($"{objectIndex.ToOrdinalWords()} has auto normal bank {(autoBank ? "on" : "off")}", () =>
1203+
{
1204+
var h = EditorBeatmap.HitObjects.ElementAt(objectIndex);
1205+
return h.Samples.Where(o => o.Name == HitSampleInfo.HIT_NORMAL).All(o => o.EditorAutoBank == autoBank);
1206+
});
1207+
1208+
private void hitObjectHasAutoAdditionBankFlag(int objectIndex, bool autoBank) => AddAssert($"{objectIndex.ToOrdinalWords()} has auto addition bank {(autoBank ? "on" : "off")}", () =>
1209+
{
1210+
var h = EditorBeatmap.HitObjects.ElementAt(objectIndex);
1211+
return h.Samples.Where(o => o.Name != HitSampleInfo.HIT_NORMAL).All(o => o.EditorAutoBank == autoBank);
1212+
});
1213+
10281214
private void hitObjectNodeHasSamples(int objectIndex, int nodeIndex, params string[] samples) => AddAssert(
10291215
$"{objectIndex.ToOrdinalWords()} object {nodeIndex.ToOrdinalWords()} node has samples {string.Join(',', samples)}", () =>
10301216
{

‎osu.Game.Tests/Visual/Editing/TestScenePlacementBlueprint.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -272,8 +272,8 @@ public void TestAutomaticBankAssignment()
272272
AddAssert("circle has 2 samples", () => EditorBeatmap.HitObjects[1].Samples, () => Has.Count.EqualTo(2));
273273
AddAssert("normal sample has soft bank", () => EditorBeatmap.HitObjects[1].Samples.Single(s => s.Name == HitSampleInfo.HIT_NORMAL).Bank,
274274
() => Is.EqualTo(HitSampleInfo.BANK_SOFT));
275-
AddAssert("clap sample has drum bank", () => EditorBeatmap.HitObjects[1].Samples.Single(s => s.Name == HitSampleInfo.HIT_CLAP).Bank,
276-
() => Is.EqualTo(HitSampleInfo.BANK_DRUM));
275+
AddAssert("clap sample has soft bank", () => EditorBeatmap.HitObjects[1].Samples.Single(s => s.Name == HitSampleInfo.HIT_CLAP).Bank,
276+
() => Is.EqualTo(HitSampleInfo.BANK_SOFT));
277277
AddAssert("circle inherited volume", () => EditorBeatmap.HitObjects[1].Samples.All(s => s.Volume == 70));
278278

279279
AddStep("seek to 1000", () => EditorClock.Seek(1000)); // previous object is the one at time 500, which has no additions

‎osu.Game/Rulesets/Edit/HitObjectPlacementBlueprint.cs‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -113,29 +113,29 @@ public override SnapResult UpdateTimeAndPosition(Vector2 screenSpacePosition, do
113113
var lastHitObject = getPreviousHitObject();
114114
var lastHitNormal = lastHitObject?.Samples?.FirstOrDefault(o => o.Name == HitSampleInfo.HIT_NORMAL);
115115

116-
if (AutomaticAdditionBankAssignment)
117-
{
118-
// Inherit the addition bank from the previous hit object
119-
// If there is no previous addition, inherit from the normal sample
120-
var lastAddition = lastHitObject?.Samples?.FirstOrDefault(o => o.Name != HitSampleInfo.HIT_NORMAL) ?? lastHitNormal;
121-
122-
if (lastAddition != null)
123-
HitObject.Samples = HitObject.Samples.Select(s => s.Name != HitSampleInfo.HIT_NORMAL ? s.With(newBank: lastAddition.Bank) : s).ToList();
124-
}
116+
if (lastHitNormal != null && AutomaticBankAssignment)
117+
// Inherit the bank from the previous hit object
118+
HitObject.Samples = HitObject.Samples.Select(s => s.Name == HitSampleInfo.HIT_NORMAL ? s.With(newBank: lastHitNormal.Bank, newEditorAutoBank: true) : s).ToList();
119+
else
120+
HitObject.Samples = HitObject.Samples.Select(s => s.Name == HitSampleInfo.HIT_NORMAL ? s.With(newEditorAutoBank: false) : s).ToList();
125121

126122
if (lastHitNormal != null)
127123
{
128-
if (AutomaticBankAssignment)
129-
// Inherit the bank from the previous hit object
130-
HitObject.Samples = HitObject.Samples.Select(s => s.Name == HitSampleInfo.HIT_NORMAL ? s.With(newBank: lastHitNormal.Bank) : s).ToList();
131-
132124
// Inherit the volume and sample set info from the previous hit object
133125
HitObject.Samples = HitObject.Samples.Select(s => s.With(
134126
newVolume: lastHitNormal.Volume,
135127
newSuffix: lastHitNormal.Suffix,
136128
newUseBeatmapSamples: lastHitNormal.UseBeatmapSamples)).ToList();
137129
}
138130

131+
if (AutomaticAdditionBankAssignment)
132+
{
133+
string bank = HitObject.Samples.FirstOrDefault(s => s.Name == HitSampleInfo.HIT_NORMAL)?.Bank ?? HitSampleInfo.BANK_SOFT;
134+
HitObject.Samples = HitObject.Samples.Select(s => s.Name != HitSampleInfo.HIT_NORMAL ? s.With(newBank: bank, newEditorAutoBank: true) : s).ToList();
135+
}
136+
else
137+
HitObject.Samples = HitObject.Samples.Select(s => s.Name != HitSampleInfo.HIT_NORMAL ? s.With(newEditorAutoBank: false) : s).ToList();
138+
139139
if (HitObject is IHasRepeats hasRepeats)
140140
{
141141
// Make sure all the node samples are identical to the hit object's samples

‎osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -297,22 +297,25 @@ protected virtual void UpdateTernaryStates()
297297

298298
var samplesInSelection = SelectedItems.SelectMany(enumerateAllSamples).ToArray();
299299

300-
foreach ((string sampleName, var bindable) in SelectionSampleStates)
300+
if (samplesInSelection.Length > 0)
301301
{
302-
bindable.Value = GetStateFromSelection(samplesInSelection, h => h.Any(s => s.Name == sampleName));
303-
}
302+
foreach ((string sampleName, var bindable) in SelectionSampleStates)
303+
{
304+
bindable.Value = GetStateFromSelection(samplesInSelection, h => h.Any(s => s.Name == sampleName));
305+
}
304306

305-
foreach ((string bankName, var bindable) in SelectionBankStates)
306-
{
307-
bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name == HitSampleInfo.HIT_NORMAL), h => h.Bank == bankName);
308-
}
307+
foreach ((string bankName, var bindable) in SelectionBankStates)
308+
{
309+
bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name == HitSampleInfo.HIT_NORMAL), h => h.Bank == bankName);
310+
}
309311

310-
SelectionAdditionBanksEnabled.Value = samplesInSelection.SelectMany(s => s).Any(o => o.Name != HitSampleInfo.HIT_NORMAL);
312+
SelectionAdditionBanksEnabled.Value = samplesInSelection.SelectMany(s => s).Any(o => o.Name != HitSampleInfo.HIT_NORMAL);
311313

312-
foreach ((string bankName, var bindable) in SelectionAdditionBankStates)
313-
{
314-
bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name != HitSampleInfo.HIT_NORMAL),
315-
h => (bankName != HIT_BANK_AUTO && h.Bank == bankName && !h.EditorAutoBank) || (bankName == HIT_BANK_AUTO && h.EditorAutoBank));
314+
foreach ((string bankName, var bindable) in SelectionAdditionBankStates)
315+
{
316+
bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s).Where(o => o.Name != HitSampleInfo.HIT_NORMAL),
317+
h => (bankName != HIT_BANK_AUTO && h.Bank == bankName && !h.EditorAutoBank) || (bankName == HIT_BANK_AUTO && h.EditorAutoBank));
318+
}
316319
}
317320
}
318321

0 commit comments

Comments
 (0)