- Closes https://github.com/ppy/osu/issues/37884
- Closes https://github.com/ppy/osu/pull/37890
Due to lack of population of `Storyboard.Beatmap` and
`Storyboard.BeatmapInfo` post-decoding, `LegacyBeatmapExporter` would
completely drop background specifications on exported beatmap packages.
This affects both direct legacy export to file (`.osz`) as well as
beatmap submission.
I will not pretend that the API here is optimal but I do not see very
easy opportunities to curtail misuse. Storyboards can be treated as
either parts of a beatmap or standalone entities, and if a requirement
is added to forcibly provide a beatmap and its info when encoding out a
storyboard, I also foresee a requirement to bypass this later when
design mode is implemented, which would be a return to square one.
There is likely room for cleanup around `Storyboard` to maybe make this
nicer (remove passing of both `Beatmap` and `BeatmapInfo` and just pass
`Beatmap` instead, maybe shuffle some properties from `Beatmap` to
`Storyboard` to remove the requirement of having to bolt the beatmap on
to begin with). I leave voicing opinions on that, and how soon that
should be done, to reviewers. My primary intent at this time is to
hotfix a major issue in a released build.
The external editing feature is not involved in this bug and any
attempts to claim so are misdirections.
- 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.
- No longer holds realm write transaction open while performing sqlite
lookups.
- No longer attempts a write transaction when it will be a noop.
I'll admit that this is maybe working around the actual realm write part
being slow, but as I can't profile the issue locally, the sluggishness
may actually be in sqlite for those users affected (since it's only been
reported for tag population and not difficulty calculation?).
Regardless, this should fix the issue this iteration.
I also adjusted the user messaging to let them know why tag population
is happening, since we've had some questions as to why it's running in
the first place (it only happens once a month, so that's
understandable).
- [x] Depends on https://github.com/ppy/osu/pull/37227.
- Closes https://github.com/ppy/osu/issues/34699.
- Closes https://github.com/ppy/osu/issues/37210.
Note that https://github.com/ppy/osu/pull/36128 also exists and has
valid improvements which can be addressed separately. This is intended
to be something we can act on immediately.
---------
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
The compatibility export was creating .osu files with CRLF line endings
on Windows and LF line endings on Linux/macOS, because StreamWriter
defaults to Environment.NewLine.
This caused the server to detect spurious file changes when a mapper
alternated between platforms, leading to unnecessary wipes of local
scores and noise in the beatmap update history.
Fix by explicitly setting NewLine = "\r\n" on the StreamWriter, ensuring
CRLF is always used regardless of platform.
Closes#36846
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
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>
- Adds sorting and display styles.
- Saves sort/display modes to the config.
- Improves performance, especially on the 2nd+ time opening the overlay.
https://github.com/user-attachments/assets/e32b50d0-58a1-4eef-b18c-988fb497e545
---
Coming off some recent feedback in
https://github.com/ppy/osu/discussions/33426#discussioncomment-13431275,
I decided to take a bit of a detour and get a little bit more
functionality in.
Sorting by rank, although it should technically work, doesn't work right
now. This is because the osu!web API doesn't return user rank on
`/user/` lookups - it's only returned for the friends request. I'm
leaving this open as a discussion topic.
- We can make osu!web return the rank and osu! will require no further
changes to work correctly, or
- We can try to implement additional paths through
`osu-server-spectator` which would blow this PR out of proportion and is
best left for a task of its own.
For simplicity, I've re-implemented this display mostly as its own
component for now, lifting code from `FriendDisplay` which was recently
overhauled. These implementations should eventually be combined somehow
but that's dependent on:
1. Figuring out the styling - friends can display offline users for
which it makes no sense to display the "spectate" button.
2. Figuring out how to handle the different users/presence pathways.
It's mostly a code complexity issue.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
This ends up with a bit of an undefined behaviour, but it's already a
bit of an edge case (files missing in the `files` folder that are
references in the database).
First and foremost, let's stop the exception. If we allow it to throw,
it's impossible to exit the skin editor in this state.
Closes https://github.com/ppy/osu/issues/36135.
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.
Because the detached store exists and has a chance to actually
semi-reliably intercept a beatmap update operation, I decided to try
this. It still uses a bit of a heuristic in that it checks for
transactions that delete and insert one beatmap each, but probably the
best effort thus far?
Notably old song select that was already doing the same thing locally to
itself got a bit broken by this, but with some tweaking that *looks* to
be more or less harmless I managed to get it unbroken. I'm not too
concerned about old song select, mind, mostly just want to keep it
*vaguely* working if I can help it.
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.