- Part of https://github.com/ppy/osu/issues/37818
Access to difficulty info is required for the upcoming multiplier
proposals. All places providing difficulty info intentionally use
`IBeatmapInfo` as the difficulty info exposed to the calculator should
_always_ be pre-mods for our usecase.
There's a couple of quirks:
- The usage in `ScoreProcessor` is a bit troubling to me but I can't see
a way to make it better without refactoring it. Essentially, we don't
have a beatmap until `ApplyBeatmap` is called, but most usages of
`ScoreProcessor` are setting `Mods` prior to `ApplyBeatmap` so there is
a `null` check in the logic for when mods change. Additionally, this
means a new bindable of the beatmap via `ApplyBeatmap` which also feels
a bit dirty. Open to suggestions.
- ~~`BeatmapLeaderboardScore.Tooltip` is using a null-forgiving on the
`BeatmapInfo`, but there's basically no context available on if this is
an issue - the only code path which sets the score is `SetContent` which
has no callers, so it's essentially dead code. Makes sense given it's
Select V1.~~
---------
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
- Part of https://github.com/ppy/osu/issues/37818
During review, I would like to direct particular attention to the
following changes:
## [Migrate song select to new score multiplier
API](https://github.com/ppy/osu/commit/945fd78539da3ae57d1550a5bbfb0f859d153cc4)
This was a confusing change to write because of the way song selects
hook their mod overlays up to global bindables. In particular different
things happen in different circumstances.
- When going through `SongSelect.CreateModOverlay()`, which is called by
the base `SongSelect`, the mod overlay is automatically bound to global
bindables via `SongSelect.on{ArrivingAt,Leaving}Screen()`.
- For multiplayer user mod select overlays, which are bolted on by
subclasses of `SongSelect`, manual hook-up is required.
- As for free mod select overlays, they don't show mod multipliers at
all, and don't have easy access to the ruleset, and thus the hookup is
skipped entirely as redundant.
## [Fix score multiplier registrations being shared between
implementations via superclass static
fields](https://github.com/ppy/osu/commit/ba0a7ad421e0c84c2d8162b6bbdd3a0683f5a6a6)
Revealed by `ScoreMultiplierCalculatorTest` starting to fail due to
interference from `OsuScoreMultiplierCalculator`.
It's not ideal from a performance standpoint but it's the simplest
choice for now. Tricks could be pulled to salvage the static. One is
```csharp
public class ScoreMultiplierCalculator<T>
where T : ScoreMultiplierCalculator<T>
{
}
```
This works because of generics internals; static instance members are
not shared between different specialisations of a generic class. It is
also very unintuitive, so I would rather not. (It trips a ReSharper
inspection too, which would have to be silenced.)
From a performance standpoint this is not ideal, but a significant chunk
of migrated usages already precede the construction of the calculator
via the known-expensive `RulesetInfo.CreateInstance()`, and the paths
that actually construct the calculator do not appear to be that hot. If
need be, this can be handled by actually caching ruleset instances and
their derivative subcomponents.
## [Introduce passing of context to score multiplier
calculator](https://github.com/ppy/osu/pull/37845/changes/9e9242b3221dddacd226f4b3b9c5632d7350e998)
This is required for two reasons:
- The upcoming mod rebalance will require out-of-band supplementary
information that is not available for reading from the mod instances
themselves for calculating the multiplier.
- This context, namely passing of `ScoreInfo`, will be used for
implementing backwards compatibility with old scores and their score
multipliers. This is required because it has turned out under inspection
that all server-side lazer replays recorded until now are missing
`TotalScoreWithoutMods` due to an omission of not sending it across the
wire to spectator server.
Because the score import flow uses replays, filtered through
`LegacyScoreDecoder`, to populate total score in the realm database, it
is basically impossible to ignore scores that are missing
`TotalScoreWithoutMods`, because that will result in bug reports that
the scores do not have the new score multipliers applied.
Thus, passing of `ScoreInfo` will facilitate implementation of
versioning score multipliers, which should result in less breakage than
not doing so.
An example of this is added in 341b2d6e55,
which should handle the case of mania mod multipliers having been
changed without any attempt to facilitate for it in
https://github.com/ppy/osu/pull/30506.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
- Closes https://github.com/ppy/osu/issues/37757
Commit-by-commit reading is recommended. Commits will be split to PRs on
request but I consider this to be the minimal viable functional
increment.
## Done
- This adds a first version of a full storyboard encoder
(a66dc406f498e35d4e0c8f2a462e946a9a1aeccc). I expect there to be hiccups
due to weird corners of the `.osb` format; this is only intended to be
somewhat correct as a start to build upon. Storyboarders are asked to
file issues as necessary.
- Due to the fact that storyboard definitions can reside both in the
`.osu` and the `.osb`, b60698a95c4de1bfeb36fbb159fd5a6028920832 adds the
required storage to be able to tell which storyboard element lives
where, so that it can be decoded properly later.
- In c9d3e04a4135886b5b0943c85f3cc6f4fe99c84c, the storyboard decoder is
weaved into the beatmap decoder to handle the `.osu` part of the
storyboard, via the
`LegacyStoryboardEncoder.Encode{General,Events}ToBeatmap()` methods. For
`.osb`s, `LegacyStoryboardEncoder.EncodeStandaloneStoryboard()` is
intended, but for now is not used outside tests.
- Because of the above, dd1c4e43dc51154cd67860f096712f8b4f229661 removes
`Beatmap.UnhandledEventLines` as no longer required.
- 26ac417ed98a8937c42e5f52c4e15ef065a48902 adds tests. They are mostly
handwritten to ensure basic encode-decode roundtripping. Using existing
storyboards is difficult, see "Known issues" section as to why.
- 5cc542366db7caac38eb0729260d884905a2c0d5 fixes a bug in the storyboard
decoder where the trigger group number was not properly negated on
decode (see inline comment reference to relevant stable code).
## Known issues
- Any and all variables in the `[Variables]` section are inlined into
their usages by `LegacyStoryboardDecoder`, and as such
`LegacyStoryboardEncoder` will end up inlining them and discarding the
`[Variables]` section. As far as I can tell stable will also do this.
- `LegacyStoryboardDecoder` splits all `M` (move) commands into
`MX`/`MY` commands. Therefore, `LegacyStoryboardEncoder` will write out
things in the same split way. I did not put in effort to attempt to
reconcile this, for reasons of part laziness, part not wanting to bloat
this already-large diff.
- Ordering of storyboard samples on decode may not match the order on
decode. I'm crossing fingers this doesn't matter.
- Part of https://github.com/ppy/osu/issues/37818
- Continued from
https://github.com/ppy/osu/pull/37355#issuecomment-4449248107
## Overview
This PR introduces an alternative API, `ScoreMultiplierCalculator`, to
be used going forward for calculating mod multipliers.
The reason for introducing this new API is that it has been requested
that:
- For any two given mods, it should be possible to have the combined mod
multipliers of them in combination be *different* than the product of
the individual mods' multipliers in isolation, i.e. $mult( \\{ A, B \\}
) \neq mult( \\{ A \\} ) \cdot mult( \\{ B \\} )$.
- For an individual mod, it should be possible to have the mod
multipliers depend on a quantity that is *not* the presence of another
mod or the direct value of a setting on the mod.
This capability is being demonstrated in this PR via the
`osu.Game.Tests.Rulesets.Scoring.ScoreMultiplierCalculatorTest` test
fixture.
## Parity with `Mod.ScoreMultiplier`
This PR contains a `ScoreMultiplierCalculator` implementation for each
of the built-in four rulesets.
The abstract `osu.Game.Tests.Rulesets.RulesetScoreMultiplierTest` and
its four derived ruleset-specific test fixtures were written to ensure
that the new implementations do not diverge from the current state of
affairs.
`Mod.ScoreMultiplier` is not removed in this diff to keep size low. It
will be removed as a follow-up.
## Performance
This PR contains a benchmark comparing the current implementation via
`Mod.ScoreMultiplier` and the new `ScoreMultiplierCalculator` API.
Results below.
<details>
| Method | Times | Mods | Mean | Error | StdDev | Gen0 | Allocated |
|---------------------- |------ |---------------------
|--------------:|------------:|------------:|--------:|----------:|
| ViaModScoreMultiplier | 1 | mods (...)tings [27] | 121.171 ns | 1.5284
ns | 1.4297 ns | 0.0782 | 656 B |
| ViaCalculator | 1 | mods (...)tings [27] | 248.509 ns | 1.9313 ns |
1.6127 ns | 0.1364 | 1144 B |
| ViaModScoreMultiplier | 1 | multiple mods | 128.357 ns | 0.4282 ns |
0.4006 ns | 0.0782 | 656 B |
| ViaCalculator | 1 | multiple mods | 252.953 ns | 1.2860 ns | 1.2029 ns
| 0.1364 | 1144 B |
| ViaModScoreMultiplier | 1 | no mods | 3.007 ns | 0.0345 ns | 0.0288 ns
| - | - |
| ViaCalculator | 1 | no mods | 14.802 ns | 0.0616 ns | 0.0576 ns |
0.0134 | 112 B |
| ViaModScoreMultiplier | 1 | single mod | 40.271 ns | 0.1238 ns |
0.1098 ns | 0.0258 | 216 B |
| ViaCalculator | 1 | single mod | 113.033 ns | 0.3140 ns | 0.2937 ns |
0.0842 | 704 B |
| ViaModScoreMultiplier | 1 | single mod 2 | 3.653 ns | 0.0384 ns |
0.0359 ns | 0.0038 | 32 B |
| ViaCalculator | 1 | single mod 2 | 78.172 ns | 0.0680 ns | 0.0603 ns |
0.0621 | 520 B |
| ViaModScoreMultiplier | 10 | mods (...)tings [27] | 1,169.609 ns |
4.3058 ns | 4.0276 ns | 0.7839 | 6560 B |
| ViaCalculator | 10 | mods (...)tings [27] | 2,575.264 ns | 21.2705 ns
| 19.8964 ns | 1.3657 | 11440 B |
| ViaModScoreMultiplier | 10 | multiple mods | 1,171.775 ns | 6.2332 ns
| 5.2050 ns | 0.7839 | 6560 B |
| ViaCalculator | 10 | multiple mods | 2,579.593 ns | 22.1010 ns |
20.6733 ns | 1.3657 | 11440 B |
| ViaModScoreMultiplier | 10 | no mods | 35.943 ns | 0.1665 ns | 0.1476
ns | - | - |
| ViaCalculator | 10 | no mods | 154.980 ns | 0.2381 ns | 0.1988 ns |
0.1338 | 1120 B |
| ViaModScoreMultiplier | 10 | single mod | 404.185 ns | 1.3190 ns |
1.2338 ns | 0.2580 | 2160 B |
| ViaCalculator | 10 | single mod | 1,167.279 ns | 6.1641 ns | 5.7659 ns
| 0.8411 | 7040 B |
| ViaModScoreMultiplier | 10 | single mod 2 | 42.128 ns | 0.2878 ns |
0.2692 ns | 0.0382 | 320 B |
| ViaCalculator | 10 | single mod 2 | 775.435 ns | 2.3318 ns | 2.1811 ns
| 0.6208 | 5200 B |
| ViaModScoreMultiplier | 100 | mods (...)tings [27] | 11,623.346 ns |
51.7174 ns | 43.1863 ns | 7.8430 | 65600 B |
| ViaCalculator | 100 | mods (...)tings [27] | 25,252.987 ns | 44.4352
ns | 39.3906 ns | 13.6719 | 114400 B |
| ViaModScoreMultiplier | 100 | multiple mods | 11,928.536 ns | 35.2079
ns | 32.9334 ns | 7.8430 | 65600 B |
| ViaCalculator | 100 | multiple mods | 25,399.378 ns | 152.4597 ns |
127.3108 ns | 13.6719 | 114400 B |
| ViaModScoreMultiplier | 100 | no mods | 328.158 ns | 0.5827 ns |
0.5165 ns | - | - |
| ViaCalculator | 100 | no mods | 1,517.485 ns | 10.2304 ns | 9.5695 ns
| 1.3390 | 11200 B |
| ViaModScoreMultiplier | 100 | single mod | 3,986.251 ns | 24.2523 ns |
21.4991 ns | 2.5787 | 21600 B |
| ViaCalculator | 100 | single mod | 11,479.514 ns | 23.3738 ns |
20.7203 ns | 8.4076 | 70400 B |
| ViaModScoreMultiplier | 100 | single mod 2 | 385.679 ns | 3.5190 ns |
3.2917 ns | 0.3824 | 3200 B |
| ViaCalculator | 100 | single mod 2 | 7,658.646 ns | 21.8274 ns |
19.3494 ns | 6.2103 | 52000 B |
</details>
While the calculator is obviously slower, in my view it is not
egregiously so. The main overheads both time- and memory-wise are
collection allocations for the dictionary and the set which I consider
to be directly caused by the requested additional complexity and as such
I don't really consider them eliminable.
I have tried and applied some micro-optimisations
(e2469ce338,
cb33abec17), albeit with negligible
effect. I have also tried to key the mods by `Acronym` instead of by
`Type` and the difference was basically nil.
<details>
<summary>patch for keying by acronym</summary>
```diff
diff --git a/osu.Game/Rulesets/Scoring/ScoreMultiplierCalculator.cs b/osu.Game/Rulesets/Scoring/ScoreMultiplierCalculator.cs
index 772f9d178b..7f5907cbda 100644
--- a/osu.Game/Rulesets/Scoring/ScoreMultiplierCalculator.cs
+++ b/osu.Game/Rulesets/Scoring/ScoreMultiplierCalculator.cs
@@ -13,26 +13,26 @@ namespace osu.Game.Rulesets.Scoring
/// </summary>
public class ScoreMultiplierCalculator
{
- private static readonly List<(Type[] mods, Func<Mod[], double> multiplier)> combination_multipliers = [];
- private static readonly Dictionary<Type, Func<Mod, ScoreMultiplierCalculator, double>> single_multipliers_with_context = [];
- private static readonly Dictionary<Type, Func<Mod, double>> single_multipliers = [];
+ private static readonly List<(string[] modAcronyms, Func<Mod[], double> multiplier)> combination_multipliers = [];
+ private static readonly Dictionary<string, Func<Mod, ScoreMultiplierCalculator, double>> single_multipliers_with_context = [];
+ private static readonly Dictionary<string, Func<Mod, double>> single_multipliers = [];
/// <summary>
/// Defines a flat, setting-independent score multiplier for the given <typeparamref name="TMod"/>.
/// </summary>
public static void Single<TMod>(double hasMultiplier)
- where TMod : Mod
+ where TMod : Mod, new()
{
- single_multipliers[typeof(TMod)] = _ => hasMultiplier;
+ single_multipliers[new TMod().Acronym] = _ => hasMultiplier;
}
/// <summary>
/// Defines a setting-dependent score multiplier for the given <typeparamref name="TMod"/>.
/// </summary>
public static void Single<TMod>(Func<TMod, double> hasMultiplier)
- where TMod : Mod
+ where TMod : Mod, new()
{
- single_multipliers[typeof(TMod)] = mod => hasMultiplier.Invoke((TMod)mod);
+ single_multipliers[new TMod().Acronym] = mod => hasMultiplier.Invoke((TMod)mod);
}
/// <summary>
@@ -40,20 +40,20 @@ public static void Single<TMod>(Func<TMod, double> hasMultiplier)
/// The multiplier calculation is given additional context to calculate the multiplier via the <typeparamref name="TContext"/> type instance.
/// </summary>
public static void Single<TMod, TContext>(Func<TMod, TContext, double> hasMultiplier)
- where TMod : Mod
+ where TMod : Mod, new()
where TContext : ScoreMultiplierCalculator
{
- single_multipliers_with_context[typeof(TMod)] = (mod, context) => hasMultiplier.Invoke((TMod)mod, (TContext)context);
+ single_multipliers_with_context[new TMod().Acronym] = (mod, context) => hasMultiplier.Invoke((TMod)mod, (TContext)context);
}
/// <summary>
/// Defines a score multiplier specific to when both <typeparamref name="T1"/> and <typeparamref name="T2"/> mods are present.
/// </summary>
public static void Combination<T1, T2>(Func<T1, T2, double> hasMultiplier)
- where T1 : Mod
- where T2 : Mod
+ where T1 : Mod, new()
+ where T2 : Mod, new()
{
- combination_multipliers.Add(([typeof(T1), typeof(T2)], mods => hasMultiplier((T1)mods[0], (T2)mods[1])));
+ combination_multipliers.Add(([new T1().Acronym, new T2().Acronym], mods => hasMultiplier((T1)mods[0], (T2)mods[1])));
}
/// <summary>
@@ -61,7 +61,7 @@ public static void Combination<T1, T2>(Func<T1, T2, double> hasMultiplier)
/// </summary>
public double CalculateFor(IEnumerable<Mod> mods)
{
- var allModsByType = mods.ToDictionary(m => m.GetType());
+ var allModsByType = mods.ToDictionary(m => m.Acronym);
if (allModsByType.Count == 0)
return 1;
@@ -83,7 +83,7 @@ public double CalculateFor(IEnumerable<Mod> mods)
}
}
- foreach (var modType in remainingModTypes)
+ foreach (string modType in remainingModTypes)
{
if (single_multipliers.TryGetValue(modType, out var multiplier))
result *= multiplier(allModsByType[modType]);
```
</details>
One particular parallel thread that may warrant follow-up is that
`Mod.UsesDefaultConfiguration` is disproportionately expensive due to
calling into regexes via Humanizer internals.
<img width="1517" height="517" alt="Screenshot_2026-05-19_at_10 58 30"
src="https://github.com/user-attachments/assets/68309a8c-74e7-4f96-8ef9-62868eeca337"
/>
Matches stable and significantly improves UX when mapping. In addition,
current behavior makes it too easy to place stacked objects which is
something we should not encourage.
---------
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
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.
James Wilson ·
Bartłomiej Dach ·
Derrick Timmermans ·
Dean Herbert ·
Hivie ·
Kao Li Chin (Gao Li Jin) ·
Andrei Zavatski ·
Denis Titovets ·
Dan Balasescu ·
Austin Moore ·
evill ·
evill ·
Eeli Ogata ·
Arthur Araujo ·
Linus Genz ·