Touched on in https://github.com/ppy/osu/discussions/28581.
After a bit more usage of the editor I do agree with this and think that
making the fades a bit more gentle helps a lot.
Closes https://github.com/ppy/osu/issues/28741.
Regressed in a7b066f3ee.
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).
This was reported in https://github.com/ppy/osu/pull/28474, albeit the
code changes proposed there did not fix the issue at all.
See 8b6385f7d0 for demonstration of the
crash scenario. Basically what is happening there is:
- The starting premise is that there is a spinner placement active.
- At this time, a drag selection is started via the timeline.
- Once the drag selection finds at least one suitable object to select,
it mutates `SelectedItems`.
- When selection changes for any reason, the `HitObjectComposer`
decides to switch to the "select" tool, regardless of why
the selection changed.
- Changing the active tool causes the current placement - if any -
to be committed, which mutates the beatmap.
- Back at the drag box selection code, this causes a "collection
modified when enumerating" exception.
The proposed fix here is to eagerly commit active placement - if any -
when drag selection is initiated via the timeline, which avoids this
issue. This also appears to vaguely match stable behaviour and is sort
of consistent with the logic of committing any outstanding changes upon
switching to the selection tool.
Closes https://github.com/ppy/osu/issues/28369.
The reporter of the issue was incorrect; it's not the beat snap grid
that is causing the problem, it's something far stupider than that.
When the current selection changes,
`EditorSelectionHandler.UpdateTernaryStates()` is supposed to update the
state of ternary bindables to reflect the reality of the current
selection. This in turn will fire bindable change callbacks for said
ternary toggles, which heavily use `EditorBeatmap.PerformOnSelection()`.
The thing about that method is that it will attempt to check whether any
changes were actually made to avoid producing empty undo states, *but*
to do this, it must *serialise out the entire beatmap to a stream* and
then *binary equality check that* to determine whether any changes were
actually made:
7b14c77e43/osu.Game/Screens/Edit/EditorChangeHandler.cs (L65-L69)
As goes without saying, this is very expensive and unnecessary, which
leads to stuff like keeping a selection box active while a taiko beatmap
is playing under it dog slow. So to attempt to mitigate that, add
precondition checks to every single ternary callback of this sort to
avoid this serialisation overhead.
And yes, those precondition checks use linq, and that is *still* faster
than not having them.
