Even without the ID resetting logic before `Populate()` getting broken
and annotated with a TODO, this was kinda stupid. Why was purging logic
allowed to run *using a not-yet-completely-validated online ID of the
set*?
This is the other part of hopefully fixing scenarios like
https://osu.ppy.sh/community/forums/topics/2162457?n=8 (hopefully with
no babies poured out in the mean time, but only time will tell).
The end of `Populate()` is too early to do any validation of online IDs,
as population happens in `PostImport()` via `ProcessBeatmap`.
Additionally, this modifies the check to also include the set's ID. This
is one part of curtailing issues like
https://osu.ppy.sh/community/forums/topics/2162457?n=8 - the featured
artist template beatmap in question has a single difficulty with a bogus
positive beatmap set ID and a beatmap ID of 0, and my opinion is that
this sort of situation should for all intents and purposes cause the
beatmap set's ID to be reset.
A few facts of life:
- Guest difficulties are at this point a staple of mapping.
- People are very much used to flinging `.osu`s around (because there's
no better alternative).
- Currently there are two ways to get an `.osu` out of lazer. You can:
- Export the beatmap as "compatibility" to an `.osz`, then
transmogrify the `.osz` to a `.zip`, then extract the `.zip`, then
pluck out the `.osu`. This is the "correct" way to make sure stable
works, but is also stupidly arcane.
- Use "edit externally" to mount the beatmap files to disk, then
copy-paste out the `.osu`. This is the *wrong* way to make sure
stable works, because the mounting process exposes the raw "for
editing" format with features stable doesn't support, but it the
actual easy one.
- Reports about guest difficulties exported from lazer "working wrong on
stable" are prevalent. Probably mostly because of the preceding point.
What this PR does is introduce a *third* method to export an `.osu`,
which is designed to be both the easiest one yet *and* correct. I am
hoping this will curb the complaints until support for direct submission
of guest difficulties is added - which I still hope to see, but it will
be a significant effort *client-side* (the server side has been ready
for years now).
And yes, you will notice that much of the code added in
`LegacyBeatmapExporter` related to manipulation of the path is
copy-pasted from `LegacyExporter`. I don't care enough to invent
protected / abstract / whatever else OOP faff for something that may not
survive review and is mostly a weird semi-temporary wart.
Closes https://github.com/ppy/osu/issues/35807.
The reason this closes the aforementioned issue is as follows:
Taking https://osu.ppy.sh/beatmapsets/1236180#osu/4650477 as the
example, we have:
```
minBeatLength = 342.857142857143
maxBeatLength = 419.58041958042003
mostCommonBeatLength = 342.85700000000003
```
Note that `mostCommonBeatLength < minBeatLength` here.
Taking the inverse of that to compute BPM, we get
```
minBpm = 174.99999999999991
maxBpm = 142.99999999999986
mostCommonBpm = 175.00007291669704
```
which without DT present doesn't do anything bad, but when DT is
engaged (and thus BPM is multiplied by 1.5), midpoint rounding causes
the min BPM to become 262, and the most common BPM to become 263.
* 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
Is this lazy? Sure it is. Friends and blocks do the same thing, though,
and I'm not overthinking this any more than I already have.
Being smarter here would likely mean being more invasive with respect to
listening in on all outgoing API requests and silently updating
favourites on that basis. Which is "smart" but also complicated.
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.
* SSV2 : Replace Mark as Played with Remove from Played if already played
* Remove checks of BeatmapInfo.LastPlayed for DateTimeOffset.MinValue
* Make FooterButtonOptions use a RealmLive<BeatmapInfo> and act on review comments
* FIXUP: Detach BeatmapInfo before passing it to FooterButtonOptions.Popover
---------
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
Alban Cabannes-Michel
·
2025-10-16 14:59:16 +02:00
Easiest way to make this work without rewriting the layout logic.
I think it makes sense to have the button still exist there but not be
usable on certain screens.
This was not fatal but with this change it should recover in a slightly
better way.
Also incidentally fixes the cache refetch potentially being attempted
twice by each of the two background population tasks that use it.
This change refactors `GetAdjustedDisplayDifficulty()` and
`GetBeatmapAttributesToDisplay()` in two ways:
- Both methods now accept `IBeatmapInfo` instead of
`IBeatmapDifficultyInfo`. This is done in order to make mania key
count display to work, wherein `IBeatmapDifficultyInfo` is not enough
to calculate the final key count.
- `GetAdjustedDisplayDifficulty()` now applies all
`IApplicableToDifficulty` mods itself. I did this after noticing that
every real consumer of this method had to do that themselves for very
little reason.
The way that this works is that it plugs into the online request to
retrieve the beatmap set that the client is already performing, and
stores user tag data to the local realm database.
This means that for now user tags will only populate for beatmaps that
the user has displayed on song select which is obviously subpar. I plan
to follow this change up by adding user tag state dumps to `online.db`
and using that data for initial tag population to make the majority case
(ranked beatmaps) work.
Note that several decisions were made here that are potential discussion
points:
- `RealmPopulatingOnlineLookupSource` is set up such that it can be the
middle man / redirection point for similar flows that we need and we
are currently missing, such as storing guest difficulty information,
or storing the user's current best score on a beatmap (handy for rank
achieved sorting / filtering / etc.)
- The user tags are stored in `BeatmapMetadata` which breaks the
longstanding assumption that you can arbitrarily pull out a metadata
instance from any of the beatmaps in a set and get essentially the
same object back.
I've attempted to constrain this some by not adding user tags to
the `IBeatmapMetadataInfo` interface through which `BeatmapSetInfo`
exposes metadata further, but I warn in advance that this is
a temporary state of affairs and I will make it worse in the future
when `BeatmapMetadata.Author` becomes `Authors` plural in order to
support guest mapper display (and direct guest difficulty submission).
- The syntax for searching via user tags is chosen to mostly match web -
it's `tag=`, with support for all of the string matching modes song
select already has (bare word for substring, `""` quotes for phrase
isolated by whitespace, `""!` for exact full match).