Addresses https://github.com/ppy/osu/discussions/34705, I suppose.
The cagey tone of that statement is because this change merely papers
over the issue. The issue in question for the user that reported this is
that they have a bunch of very old beatmaps, whose md5 hashes do not
match the online hashes, that need updating. The submission/rank date
population was running every single time for these, and failing every
time, because there is really not much useful that the lookup *can* do.
Because mappers have made `OnlineID` essentially useless for determining
the provenance of a beatmap due to reusing them to "fix" beatmap
submission failures, online IDs have been explicitly disallowed from use
in any sort of beatmap lookup flow. The only things that are allowed to
be used are: md5 of the beatmap, and filename as a fallback for very old
beatmaps / beatmap packs.
If the user has local beatmaps with md5 not matching online, chances are
that any metadata lookups are likely to fail or return bogus data. At
that point my personal feeling is that backpopulation flows should leave
such beatmaps well alone and the user should just go update the beatmap
themselves.
I am aware that updating 124 individual beatmap sets would - in the
current state of things - would probably be a ridiculously onerous thing
to do, and that people have been asking multiple times for a facility to
update all local beatmaps at once, but that discussion is out of scope
at this stage.
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.
Takes changes in https://github.com/ppy/osu/pull/34233 and removes *all*
of the complexity.
Supersedes and closes#34233.
Contains same caveat that we can only display one beatmap once in the
carousel; this will be addressed separately.
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).
The fact that the stuff "just worked" previously due to one load-bearing
detach in a random location is really scary because a lot of this was
just not written the way it is supposed to be.
While usually we'd handle this locally by moving bind operations to
`LoadComponent`, this component was explicitly made to be used in
asynchronous scenarios (to allow cases like song select to coexist with
realm without adding huge compliexities to the classes locally).
So I think it makes sense to hide this as an implementation detail. The
locked segments should all be quite fast to run so I do not see a
performance issue with lock contention here.
Yesterday after the lazer release there was a bit of a spike in the
number of osu-web requests pointed at `/api/v2/beatmaps/lookup`
specifically. The most likely reason for this is that prior to this
commit, the star rating recalculation was fully performed by the
`BeatmapUpdater.Process()` flow.
This process does full metadata lookups, and while it *will* attempt
to use the local `online.db` metadata cache, it *will* also fall back to
API requests if the local metadata fetch fails. While that means that
the local cache likely saved us from a doomsday scenario here, it *also*
is the case that all of that metadata lookup stuff is *entirely
unnecessary* when wanting to just update star ratings.
Therefore, this splits out only the part relevant to star ratings
as a separate background process, so that it can run completely
locally.
Done for two reasons:
- During review it was requested for the logic to be moved out of
`BezierConverter` as `BezierConverter` was intended to produce
"lazer style" sliders with per-control-point curve types,
as a future usability / code layering concern.
- It is also relevant for encode-decode stability. With how the logic
was structured between the Bezier converter and the legacy beatmap
encoder, the encoder would leave behind per-control-point Bezier curve
specs that stable ignored, but subsequent encodes and decodes in lazer
would end up multiplying the doubled-up control points ad nauseam.
Instead, it is sufficient to only specify the curve type for the
head control point as Bezier, not specify any further curve types
later on, and instead just keep the double-up-control-point for new
implicit segment logic which is enough to make stable cooperate
(and also as close to outputting the slider exactly as stable would
have produced it as we've ever been)
The intention here is to make things more testable going forward.
Specifically, to remove the "back-door" entrance into `BeatmapCarousel`
where `BeatmapSets` can be set by tests and bypas/block realm retrieval.