- [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.
Previously CompositeDrawable.CheckChildrenLife() would be run before lifetimeManager.Update() which lead to the new drawables being inserted into the container but not being made alive immediately, leading to the drawable not becoming visibile until the next update loop.
This stems from me looking into `TestSceneFailAnimation` failures
(https://github.com/ppy/osu/runs/48663953318). As it turns out, I should
not have been mad by CI, and rather should have been mad at myself for
failing to read.
`FailedAtJudgement` in fact does not mean "this judgement, and only this
judgement, triggered failure". If any further judgements occur
post-fail, they will also have `FailedAtJudgement` set to true. It is
essentially a *dump* of the state of `HealthProcessor.Failed` prior to
applying the judgement.
https://github.com/ppy/osu/blob/ec21685c2531af3b243f7f0833ffbb340bf3c044/osu.Game/Rulesets/Scoring/HealthProcessor.cs#L49-L57
Because of this, reverting several judgements which occur post-fail
could lead to failed state reverting earlier than intended, and thus
potentially trigger a second fail, thus tripping the `Player` assertion.