The "Key Count" metric in mania is very useless since you are already
expected to play maps with a specific Key Count when you are queueing.
This PR inserts the proportion of LNs (Long Notes) in the place of that
metric since it is one of the ways players can gudge their skillsets
(This idea comes from reddit)
Also improved the test suite for other skillsets by making the
architecture more minor ruleset friendly
Addresses https://github.com/ppy/osu/discussions/37568.
---------
Co-authored-by: Dan Balasescu <smoogipoo@smgi.me>
Co-authored-by: Dean Herbert <pe@ppy.sh>
Kao Li Chin (Gao Li Jin)
·
2026-05-08 18:04:29 +09:00
In some cases `SliderPath.GetPathToProgress` used to compute the whole
path when it can be not needed since it can be already stored inside
`calculatedPath` list.
Also some of these use cases will no longer require additional array
wheen only readonly access is all we need.
https://github.com/user-attachments/assets/edfc8d06-4f04-4876-84a5-dfc83a18f160
Of note:
- Supports both native beatmaps and converts
- Supports key mods (changing key mods will trigger song select refilter
when key count grouping is engaged)
- The option to group by keys is only visible when mania ruleset is
active
- If the user selects key count grouping and then switches to another
ruleset, song select will fall back to no grouping, but this change will
not be written back to config. Only the user changing the grouping mode
manually will reflect in config changes. This is done so that key
grouping persists across ruleset changes, and this even survives game
restarts.
---
I've only done some light behaviour testing on this because this feature
needs a lot of subjective shot calls and I don't want to commit too deep
before I get a temperature check on the shot calls I made here.
In particular some performance profiling of
https://github.com/ppy/osu/commit/7de8f70b1dbbdf2e3f13ba10faf25329abf6468d
may be warranted.
Intended to add toggle but forgot.
This also fixes https://github.com/ppy/osu/issues/37012 via a convoluted
refactor of a lot of stuff. The basic overview is:
- Moved all replay overlay concerns out of `HUDOverlay`. We can display
this above everything confidently (i think).
- Split out `ReplayOverlay` and `ReplaySettingsOverlay` so the base
class can handle the visibility, hotkeys and everything that should be
shared with *all* replay overlay components going forward. `Ctrl+H` is
supposed to hide any of these kinds of details, and I'm sure we'll add
more in the future.
- Reorganised some things in `Player` so the new structure would work.
Mainly the overlays which add a black layer during fade out.
1% is not precise enough to push accuracy to high enough levels.
Increasing the precision of the mod will make it more useful for a
larger amount of players who want to push their accuracy to their
absolute limits. This does come with the caveat that it's impossible to
achieve over 99.9% accuracy on many short maps, but I don't think it
really matters if high enough settings act like the Perfect mod on short
enough maps.
Co-authored-by: evill <jlkdsf;ajfklsjd@123.n>
It's been a while.
Notes:
- `SharpCompress` usages changed a bit. Manually adjusted these, mostly
just renames or adjusted parameters.
- nUnit 3 -> 4 migrated using
https://gist.github.com/peppy/07994386d793a117350cb5f24b156585. there's
a mode in this script to update to the newer `Assert.That` syntax but it
requires fixes and couldn't really be bothered.
- DeepEqual nuked as the only usage was on a disabled test. The reason
it's disabled has been merged upstream, but it's failing for other
(realm) reasons which I don't think is worthwhile to investigate for
now.
- This bumps Moq. I think the author is back in a sensible headspace and
the new version has the stupid shit removed, so probably okay? Nice to
be on a level playing field with packages for once in a long time.
- Automapper is silly, but we've discussed this elsewhere.
- `TestRealmKeyBindingStore` failures are a wildcard, but fixed by using
a more standardised testing method. Dunno why, don't care.
---------
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
As mentioned in https://github.com/ppy/osu/discussions/36883.
This has caught me off-guard a few times.
Was a quick one to make this work like it does on stable. It doesn't fit
as well as stable because we have a lot of elements at the top of the
screen, but I think it's better than nothing, as it lets you know you're
in a replay quick obviously.
I don't think we can easily localise strings with formatting in them
yet. Maybe using a `MarkdownContainer` or something?
Closes#13513
Matches stable behavior where hat only plays when the slider tick rate
is a multiple of 2.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
The reproduction scenario for the subscription leak is as follows:
1. Switch to a scrolling ruleset (anything but osu! from the standard
ones).
2. Select a beatmap to edit.
3. Load the composer.
4. Go to timing tab.
5. Change a timing point.
6. Go back to the composer.
At this point, `EditorChangeHandler.OnStateChange` will have multiple of
the same delegate in the invocation list.
<img width="691" height="311" alt="Screenshot 2026-03-05 at 11 15 55"
src="https://github.com/user-attachments/assets/57788341-9573-48f1-b360-f21036891081"
/>
That in turn is caused by the fact that changing a timing point *does*
incur a full reload of the composer via the following flow:
https://github.com/ppy/osu/blob/15b6e28ebe888b1a87574891be1a0db3b04093b7/osu.Game/Rulesets/Edit/ScrollingHitObjectComposer.cs#L145https://github.com/ppy/osu/blob/64a29313a852d50095ae4b7ea8f22fde23aa634f/osu.Game/Screens/Edit/Editor.cs#L1137-L1145
This flow is my "fault"; see https://github.com/ppy/osu/pull/28444. The
reason why a full composer reload is used is not clear to my
recollection at this time, but it is likely because it's just the least
likely to fail. A smarter solution that wouldn't require a full reload
would also entail checking that there exists a safe insertion point that
allows replacing timing points in a way that will reflect everywhere it
must. Including all of the `IScrollingAlgorithm` machinery and such.
In general it is not uncommon in the codebase to not bother to clean up
some event callbacks if it is implicitly or explicitly guaranteed that
both objects bound by the callback will always get disposed in tandem at
the same time. This *was* true with this particular flow to a point,
which was until that full composer reload was implemented.
<details>
<summary>To address the elephant in the room</summary>
Someone will inevitably notice https://github.com/ppy/osu/pull/36824
which was a clanked pull request pointing out this leak. And then
someone will inevitably call this "AI discrimination"! *Gasp!*
So first of all, let me stop you right there. Yes, as far as I am
_personally_ concerned, it is "AI discrimination". I invoke the full
force of the Butlerian Jihad.
The clank army's goal is to eradicate my job and make me work in an
Amazon warehouse instead. Or, if not that, at least my job is to be rid
of all remnants of fun I still get from it and for me to be reduced to
that one guy from the meme "i guess we're doin circles now". You know
the one.
I resent this. You attack me directly. I do not perceive the need to
meet you halfway or be civil.
That said, I have too much respect for the users of this software to
leave reports of potentially real issues unchecked. So I did check, and
it was real. And you know what? Good job to the clanker. It did what it
was designed to do: it parsed a code file, recognised a hole in a
pattern it was designed to recognise, and invoked forms of language
given to it to communicate this to the meatbag that opened that PR.
And here's the thing: my primary issue is with that meatbag that opened
that PR. That meatbag served no functional purpose in any of this. The
meatbag took a hose that spews 90% water and 10% raw sewage at random
intervals and pointed it at my house directly, claiming that they just
want to clean it. At no point did the meatbag appear to have the common
decency to pull out a container, pour some magic liquid out, check if
there's sewage in it, and filter it out if there is any. But no, that
would take *effort* and *thought*, would it not? The *effort* and
*thought* that is required of *me* to *review* the clanker's work?
The PR had no reproduction scenario, and had testing checkboxes that
were presumably meant for *me* to check off. Why is it *my* job to
figure all of this out rather than the submitter meatbag's?
I do *not* have obligations towards spew-hose-pointing meatbags. Point
that hose at your own backyard at your peril.
If you *actually manage* to get the clanker to filter out *all* of the
spew without fail itself, my only win condition is gone. But it is not
yet that time. So at least have the decency to check for the spew
yourself, rather than telling the clanker to put checkboxes in the PR
descriptions telling *me* to check for it.
</details>
- [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 https://github.com/ppy/osu/issues/30293
- Fixes https://osu.ppy.sh/community/forums/topics/2179339?n=1
Aside from fixing the off-by-one error that I mentioned in
https://github.com/ppy/osu/issues/30293#issuecomment-2413801663, this
also:
- Brings back the behaviour wherein if timing points are arranged very
weird and nightcore would play e.g. two first beats in a timing point
back-to-back, the second timing point is silent.
- Brings back the behaviour wherein the finish sample only plays if
`OmitFirstBarLine` on the timing point is disabled.
However:
- This does not bring back the behaviour wherein hat samples only play
if the slider tick rate is even because that only kind of makes sense in
common time, and if common time is mixed with waltz time or other time
signatures, it just gets weird.
- Also stable has zero attempt for compensating for waltz time anyway,
lazer's behaviour is bespoke, so that is not going to match any way you
cut it.
My testing procedure essentially consisted of getting stable to log when
it was playing nightcore samples and cross-checking the first 30sec or
so of https://osu.ppy.sh/beatmapsets/534385#osu/1131956 (check out the
timing of that beatmap, for something ranked it is DEEPLY messed up).
I guess I can add test cases if deemed required but I already wasted
much more time than I would have liked here...
Unsure about this one, but I find the preceding commit to be very
lacking in explaining to the user why the editor don't work. Shining
some things red may help aid understanding.
* Add failing test coverage for layered hit samples not playing in mania when beatmap is converted
Adding the `osu.Game.Rulesets.Osu` reference to the mania test project
is required so that `HitObjectSampleTest` base logic doesn't die on
https://github.com/ppy/osu/blob/f0aeeeea966f06add12cf2bca3dd48dac8573e82/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs#L88-L91
* Fix layered hit sounds not playing on converted beatmaps in mania
Compare
https://github.com/peppy/osu-stable-reference/blob/f9e58b4864a10f801393199e7652b2192c7342c3/osu!/GameplayElements/HitObjects/HitObject.cs#L476-L477.
In case of converted beatmaps, the last condition there
(`BeatmapManager.Current.PlayMode != PlayModes.OsuMania`) fails,
and thus layered hitsounds are allowed to play.
* Add failing test coverage for mania beatmap conversion assigning wrong samples to spinners
* Fix mania beatmap conversion assigning wrong samples to spinners
A spinner is never `IHasRepeats`. It was a dead condition, leading to
the hitobject generating fallback `NodeSamples`, which in particular
feature a silent tail which stable doesn't do.
Noticeably, stable also appears to force the head of the generated hold
note to have no addition sounds:
https://github.com/peppy/osu-stable-reference/blob/f9e58b4864a10f801393199e7652b2192c7342c3/osu!/GameplayElements/HitObjects/Mania/SpinnerMania.cs#L86-L89
* Add failing test coverage for file hit sample not falling back to plain samples if file missing
* Allow `FileHitSampleInfo` to fall back to standard samples if the file is not found (or not allowed to be looked up)
I'm honestly not 100% as to how closely this matches stable because I
reached the point wherein I'd rather not look at stable code anymore, so
as long as this passes tests I'm fine to wait for someone else to report
new breakage.
* Use alternative workaround for lack of osu! ruleset assembly in mania test project
* Fix encode stability test failures
This is a set of model changes which is supposed to facilitate support
for custom sample sets to the beatmap editor that is on par with stable.
It is the minimal set of changes. Because of this, it can probably be
considered "ugly" or however else you want to put it - but before you
say that, I want to try and pre-empt that criticism by explaining where
the problems lie.
Problem #1: duality in sample models
---
There is currently a weird duality of what a `HitObject`'s samples will
be.
- If an object has just been placed in the editor, and not saved /
decoded yet, it will use `HitSampleInfo`.
- If an object has already been encoded to the beatmap at least once, it
will use `ConvertHitObjectParser.LegacyHitSampleInfo`.
As long as that state of affairs remains, `HitSampleInfo` must be able
to represent anything that `LegacyHitSampleInfo` can, if feature parity
is to be achieved.
Problem 2: The 0 & 1 sample banks
---
Custom sample banks of 2 and above are a pretty clean affair. They map to
a suffix on the sample filename, and said samples are allowed to be
looked up from the beatmap skin. `Suffix` already exists in
`HitSampleInfo`.
However, the 1 custom sample bank is evil. It uses *non-suffixed*
samples, *allows lookups from the beatmap skins*, contrary to no bank /
bank 0, which *also* uses non-suffixed samples, but *doesn't* allow them
to be looked up from the beatmap skin.
This is why `HitSampleInfo.UseBeatmapSamples` has been called to
existence - without it there is no way to represent the ability of using
or not using the beatmap skin assets.
As has been stated previously in discussions about this feature, it's
both a *mapping* and a *skinning* concern.
There are many things you could do about either of these problems, but I
am pretty sure tackling either one is going to take *many* more lines of
code than this commit does. Which is why this is the starting point of
negotiation.