Before:
https://github.com/user-attachments/assets/d87bd7e3-37f8-4634-9e6a-5859d5bade57
After:
https://github.com/user-attachments/assets/4de940af-1e30-4266-9aac-5ccd12f38742
---
The title is convoluted but basically I'm angling to close
https://github.com/ppy/osu/issues/36705 with this.
The point is that on current `master`, the keyboard-hotkey-based toggles
on the left of the screen get disabled if you select a range of objects
which contains no addition samples. The report linked above finds this
annoying because it means you basically always need to add an addition
sound *first* and *then* pick a bank.
This is not necessary, and this commit changes the behaviour such that
the bank selection toggles are no longer blocked when you select a range
of objects without additions. Choosing an addition bank when there are
no additions still does nothing to the selected object, *but* adding a
sound *after* that bank preselection will use the preselected bank
rather than auto.
Part of the screen footer refactor.
Once footer content is being managed by `OsuScreen`, the current tests
which simply create the tested overlay and `ScreenFooter` in a container
will no longer work.
This PR refactors them to use `ScreenTestScene` with the setup being
creating a dedicated testing `OsuScreen` which does the bare minimum to
create the tested overlay and necessary components (eg.
`FooterButtonFreeModsV2` for `TestSceneFreeModsOverlay`).
Most of the changes here can be described as
`%s/<...>Overlay/screen.Overlay/g`, with some minor touchups as
necessary, given that we're now testing a more complete flow which
checks more things that were previously not handled by the tests.
## [Move footer to front in
ScreenTestScene](https://github.com/ppy/osu/commit/f8740e0403b3c0badd60d394c737f2aa912a9ed6)
Self-explanatory. Without it the footer would show below the actual
overlay, breaking tests depending on manual input. For the sake of tests
not breaking in CI, both #36718 and this have this included - would
prefer the former to be merged first since it was already reviewed
there.
## `TestSceneModSelectOverlay`
There were a few tests (`TestColumnHidingOnIsValidChange`,
`TestColumnHidingOnTextFilterChange`, and
`TestHidingOverlayClearsTextSearch`) that would create a custom overlay
instance instead of the globally provided one. I've tested both and the
tests run fine with the default overlay, so they're now using that
instead.
## `TestSceneFreeModSelectOverlay`
Updated to use footer v2.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
- [x] Depends on https://github.com/ppy/osu/pull/36741 for merge
conflict avoidance
RFC, cc @OliBomby
## [Adjust behaviour of automatic bank assignment during
placement](https://github.com/ppy/osu/commit/547f55e9b3ded668fe6e1c8865a2d625e64a2f45)
Diatribe time!
This is fallout of the discussion about auto bank in
https://github.com/ppy/osu/issues/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](https://github.com/ppy/osu/commit/052cde5987e48800ec68ab2528c7e0ce3140e6e0)
Fixes issue described in
https://github.com/ppy/osu/issues/36705#issuecomment-3953917163 wherein
opening a sample popover will disable addition bank toggles and toggle
off all addition samples.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
Closes#33395
Copies the bookmarks from `referenceWorkingBeatmap` while creating a new
difficulty from scratch. I adapted the tests in
`TestSceneEditorBeatmapCreation` to include the bookmark checks.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
The test scene doesn't exercise the custom sample playback, but I hope I
can be forgiven for this as setting up a custom editor beatmap just for
this to work is rather cumbersome.
Probably closes https://github.com/ppy/osu/issues/35138. I'm not sure. I
only got the issue to reproduce once, on dev, using a very large
archive that was uploading really slowly, and then never again.
The working theory is that basically handling of `progressSampleChannel`
is quite dodgy and it could possibly, in circumstances unknown, be
allowed to play forevermore after transitioning to failed / canceled
state. Success state does not get this treatment because it has special
logic to set progress to 1.
While I can't reproduce any test failures because of this, I can
reproduce exceptions by simply running the tests with debugger attached.
There's one other similar related test failure in `TestSingleAudioFile`
but that appears to be an o!f issue.
- Use constraints for better assert messages
- Use `Editor.Undo()` rather than manual input manager synthesizing
ctrl-z (ctrl-z is not a platform agnostic binding, see macOS)
Closes https://github.com/ppy/osu/issues/32420.
The failure cause here is that in editor the beatmap version for the
beatmap affected (or... any beatmap, really), is 0 (ZERO). That is
probably a regression from https://github.com/ppy/osu/pull/32315, but
like... can we universally agree that calling that change "a regression"
in any capacity is dumb? Like what was that code *doing* playing dumb
reference games and copying stuff into an arbitrary instance that could
get or not get used later on? And now you have a 50/50 chance of
accessing the *correct* model's field, depending on whether you go via
`BeatmapInfo` or `Beatmap.BeatmapInfo`?
Moving the field to `IBeatmap`, i.e. what is by now - by consensus,
since https://github.com/ppy/osu/pull/28473 - supposed to be the "decoded
and materialised" beatmap, fixes this issue.
I probably should have done this as part of
https://github.com/ppy/osu/pull/28473 but it slipped my mind. Probably
for the better too because this change has rather large chances of
breaking stuff so maybe better to examine it in isolation (via diffcalc
runs or whatever).
For added humour points, you'd say that the field on `BeatmapInfo` was
not `[Ignore]`d, so this is a realm schema change, right? No. As far as
I can tell, it's not. I opened realm studio and `BeatmapVersion` *is not
a listed column` on `Beatmap` models.
I'm also not gonna get into the fact that I think `EditorBeatmap` doing
dumb games with juggling two `BeatmapInfo` references since
https://github.com/ppy/osu/pull/15075 is bad, because I don't think I
have the mental capacity to hotfix this by going down that train of
thought.