TC is a mod that always increases difficulty and is quite similar to HD.
Given we even have diffcalc/pp considerations for it it's time to move
it to the category it belongs.
This doesn't cover any other mods that might need reshuffling too
because TC is the only one that has actual impact on difficulty-based
leaderboards (as in pp) and some people are actively playing it for the
difficulty increase and not just as a fun gimmick.
After a quick search turns out it was difficulty increasing from the
start but was moved to fun in review
https://github.com/ppy/osu/pull/3569#discussion_r300085523 but I don't
really agree with that.
Also as far as I know multimods don't do anything anymore?.. I've put it
into HD multimod for consistency, but can move it to be separate if you
want
Co-authored-by: Dan Balasescu <smoogipoo@smgi.me>
See inline commentary. I don't really have any energy left to provide
anything else, other than maybe a demonstration of how this dies in
framework:
diff --git a/osu.Framework.Tests/Visual/UserInterface/TestSceneScreenStackUnbindOnExit.cs b/osu.Framework.Tests/Visual/UserInterface/TestSceneScreenStackUnbindOnExit.cs
new file mode 100644
index 000000000..c74ce6636
--- /dev/null
+++ b/osu.Framework.Tests/Visual/UserInterface/TestSceneScreenStackUnbindOnExit.cs
@@ -0,0 +1,59 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using NUnit.Framework;
+using osu.Framework.Allocation;
+using osu.Framework.Graphics.UserInterface;
+using osu.Framework.Screens;
+
+namespace osu.Framework.Tests.Visual.UserInterface
+{
+ public partial class TestSceneScreenStackUnbindOnExit : FrameworkTestScene
+ {
+ [Cached]
+ private ScreenStack screenStack = new ScreenStack();
+
+ [Test]
+ public void TestScreenExitUnbindDoesNotInterruptLoadComplete()
+ {
+ AddStep("set up the scenario", () =>
+ {
+ Child = screenStack;
+ screenStack.Push(new Screen());
+ screenStack.Push(new BrokenScreen());
+ });
+ AddUntilStep("wait to get to target screen", () => screenStack.CurrentScreen, Is.InstanceOf<Screen>);
+ }
+
+ private partial class BrokenSlider : BasicSliderBar<float>
+ {
+ [Resolved]
+ private ScreenStack screenStack { get; set; } = null!;
+
+ protected override void LoadComplete()
+ {
+ // exiting the current screen provokes the behaviour of unbinding all bindables in the screen's subtree
+ screenStack.CurrentScreen.Exit();
+
+ // ...but the following calls should still take correct effect inside `SliderBar`
+ // (namely one consisting of propagating `{Min,Max}Value` into `currentNumberInstantaneous`)
+ // so that it doesn't have its internal invariants violated
+ CurrentNumber.MinValue = -10;
+ CurrentNumber.MaxValue = 10;
+
+ // this notably calls `Scheduler.AddOnce(updateValue)` inside, which will happen *in the imminent future, in the same frame as `LoadComplete()` here.
+ // if the above mutations of `{Min,Max}Value` don't correctly propagate inside the slider bar due to an overly eager unbind, this will cause a crash.
+ base.LoadComplete();
+ }
+ }
+
+ private partial class BrokenScreen : Screen
+ {
+ [BackgroundDependencyLoader]
+ private void load()
+ {
+ InternalChild = new BrokenSlider();
+ }
+ }
+ }
+}
I attempted to address what I perceive to be the root issue here which
is that `ScreenStack` is allowed to arbitrarily unbind bindables under
drawables which are by all means still in the scene graph. The attempt
consisted of scheduling the unbind until after children of the screen
stack, but that caused 150 game-side tests to fail, seemingly on
something relevant to bindable leases, so I give up.
To explain on the wordy and probably incoherent title: The patient zero
for this was the following discord report:
https://discord.com/channels/188630481301012481/1097318920991559880/1463029073998774375
After information extraction, it turned out that the user manually used
the "Edit externally..." feature to rename a sample to
`normal-hitnormal1.mp3`, thinking (logically, to be fair), that the 1
bank works the same way that all the others do.
It doesn't, samples in the 1 bank do not have a numerical suffix in the
filename - but the logic retrieving the sample sets to display in the
setup tab was not checking that, leading to confusion wherein a sample
would only "work" in the setup tab but not in the actual editor
composer.
Can be tested with [the beatmap provided by the
user](https://discord.com/channels/188630481301012481/1097318920991559880/1463110516573470774).
This ends up with a bit of an undefined behaviour, but it's already a
bit of an edge case (files missing in the `files` folder that are
references in the database).
First and foremost, let's stop the exception. If we allow it to throw,
it's impossible to exit the skin editor in this state.
Closes https://github.com/ppy/osu/issues/36135.
Reported [on reddit of all
places](https://www.reddit.com/r/osugame/comments/1qhnfdn/every_time_i_make_an_adjustment_to_the_hitcircle/).
The reason that this is happening is as follows:
- Changing a combo colour raises `BeatmapSkinChanged`
- `BeatmapSkinChanged` getting raised triggers the sample chooser
dropdown to repopulate its items (as intended and correctly)
- Setting items of a dropdown can also change its `Current.Value`,
because the relevant logic attempts to ensure that `Current.Value` is
valid in line with the new items
- Therefore the `Current.Value` of the dropdown changes from `null` to
sample set `-1` which corresponds to the "Add new..." item as it's the
only item in the dropdown...
- which is indistinguishable from the sequence of events that happens if
the user manually opens the dropdown and clicks the "Add new..." item.
This change sidesteps this entire car crash by just ensuring that even
beatmaps without custom samples have at least set number 1 initialised
(even if it's empty and has no samples). This means that the initial
value of the dropdown is never `null`, and that every time that the
value changes to the set `-1` it actually is due to user action.
Should have known better than to play these dumb games.