From d6c1b5d3991a4e9187e85806697b7f3b1e49206b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 5 Jul 2024 08:54:47 +0200 Subject: [PATCH 1/2] Add failing test coverage --- .../TestSceneHitObjectSampleAdjustments.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneHitObjectSampleAdjustments.cs b/osu.Game.Tests/Visual/Editing/TestSceneHitObjectSampleAdjustments.cs index 9988c1cb59a9..558d8dce9488 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneHitObjectSampleAdjustments.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneHitObjectSampleAdjustments.cs @@ -484,6 +484,25 @@ public void TestHotkeysAffectNodeSamples() hitObjectNodeHasSamples(2, 1, HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_WHISTLE); } + [Test] + public void TestSelectingObjectDoesNotMutateSamples() + { + clickSamplePiece(0); + toggleAdditionViaPopover(1); + setAdditionBankViaPopover(HitSampleInfo.BANK_SOFT); + dismissPopover(); + + hitObjectHasSamples(0, HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_FINISH); + hitObjectHasSampleNormalBank(0, HitSampleInfo.BANK_NORMAL); + hitObjectHasSampleAdditionBank(0, HitSampleInfo.BANK_SOFT); + + AddStep("select first object", () => EditorBeatmap.SelectedHitObjects.Add(EditorBeatmap.HitObjects[0])); + + hitObjectHasSamples(0, HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_FINISH); + hitObjectHasSampleNormalBank(0, HitSampleInfo.BANK_NORMAL); + hitObjectHasSampleAdditionBank(0, HitSampleInfo.BANK_SOFT); + } + private void clickSamplePiece(int objectIndex) => AddStep($"click {objectIndex.ToOrdinalWords()} sample piece", () => { var samplePiece = this.ChildrenOfType().Single(piece => piece.HitObject == EditorBeatmap.HitObjects.ElementAt(objectIndex)); From 4c59ec1d94489857b12705a2d195b59aed019f70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 5 Jul 2024 09:10:38 +0200 Subject: [PATCH 2/2] Fix incorrect ternary state computation for bank toggles Closes https://github.com/ppy/osu/issues/28741. Regressed in a7b066f3ee59b9e9f13344ce3af4c5e7cf511e67. The intent of the original change there was to ensure that addition banks being set will put the ternary state toggles in indeterminate state (to at least provide a visual indication that the selection does not use a single bank). This would previously not be the case due to the use of `.All()` in the original condition (a single object/node was considered to have a bank enabled if and only if *all* samples within it used it). However the attempt to fix that via switching to `Any()` was not correct. The logic used in the offending commit operates on extracted `Samples` and `NodeSamples` from the selection, and would consider the ternary toggle: - fully off if none of the samples/node samples contained a sample with the given bank, - indeterminate if the some of the samples/node samples contained a sample with the given bank, - fully on if at least one sample from every samples/node samples contained a sample with the given bank. This is a *two-tiered* process, as in first a *binary* on/off state is extracted from each object's samples/node samples, and *then* a ternary state is extracted from all objects/nodes. This is insufficient to express the *desired* behaviour, which is that the toggle should be: - fully off if *none of the individual samples in the selection* use the given bank, - indeterminate if *at least one individual sample in the selection* uses the given bank, - fully on if *all individual samples in the selection* use the given bank. The second wording is flattened, and no longer tries to consider "nodes" or "objects", it just looks at all of the samples in the selection without concern as to whether they're from separate objects/nodes or not. To explain why this discrepancy caused the bug, consider a single object with a `soft` normal bank and `drum` addition bank. Selecting the object would cause a ternary button state update; as per the incorrect logic, there were two samples on the object and each had its own separate banks, so two ternary toggles would have their state set to `True` (rather than the correct `Indeterminate`), thus triggering a bindable feedback loop that would cause one of these banks to win and actually overwrite the other. Note that the addition indeterminate state computation *still* needs to do the two-tiered process, because there it actually makes sense (for a selection to have an addition fully on rather than indeterminate, *every* object/node *must* contain that addition). --- .../Screens/Edit/Compose/Components/EditorSelectionHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs b/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs index 70c91b16fd3f..a4efe66bf8c5 100644 --- a/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs +++ b/osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs @@ -186,7 +186,7 @@ protected virtual void UpdateTernaryStates() foreach ((string bankName, var bindable) in SelectionBankStates) { - bindable.Value = GetStateFromSelection(samplesInSelection, h => h.Any(s => s.Bank == bankName)); + bindable.Value = GetStateFromSelection(samplesInSelection.SelectMany(s => s), h => h.Bank == bankName); } IEnumerable> enumerateAllSamples(HitObject hitObject)