Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to export beatmaps from editor in a stable-compatible format #24186

Merged
merged 16 commits into from
Jul 23, 2023

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Jul 11, 2023

This is a menu button that lets you explicitly export a beatmap set to osu! stable.
With this feature it becomes a lot more feasible to create beatmaps in Lazer using the full range of features available in Lazer.

osu!_U9q1Aii7XI

It utilizes the slider path conversion algorithms introduced in #20952 to do a lossy conversion of multi-type sliders to bezier-type sliders which are compatible with osu! stable. It also truncates the time values and coordinates of the slider paths.

Also the default 'Export package' option now creates the file extension '.olz' (osu! lazer zip) which is accepted by osu! lazer but not by osu! stable, while 'Export legacy package' creates a '.osz' file which obviously is accepted by both osu! stable and lazer. This makes it less likely to confuse the two formats.

The exporting code is based on the answer provided in #23490. It creates a new BeatmapSetInfo clone and converts all beatmaps inside to legacy format, then gives it gives that Realm object to the exporter. I'm not sure if the converted beatmaps in the Realm file storage get cleaned up afterwards though.

The LegacyBeatmapExporter modifies the file contents read by the LegacyArchiveExporter using an override, so it can convert beatmap files to be legacy compatible just before they get added to the archive.

osu.Game/Database/BeatmapExporter.cs Show resolved Hide resolved
osu.Game/Database/BeatmapExporter.cs Outdated Show resolved Hide resolved
osu.Game/Beatmaps/BeatmapManager.cs Outdated Show resolved Hide resolved
osu.Game/Beatmaps/BeatmapManager.cs Outdated Show resolved Hide resolved
osu.Game/Beatmaps/BeatmapManager.cs Outdated Show resolved Hide resolved
@@ -966,6 +966,7 @@ private void updateLastSavedHash()
{
new EditorMenuItem(WebCommonStrings.ButtonsSave, MenuItemType.Standard, () => Save()),
new EditorMenuItem(EditorStrings.ExportPackage, MenuItemType.Standard, exportBeatmap) { Action = { Disabled = !RuntimeInfo.IsDesktop } },
new EditorMenuItem(EditorStrings.ExportLegacyPackage, MenuItemType.Standard, exportLegacyBeatmap) { Action = { Disabled = !RuntimeInfo.IsDesktop } },
Copy link
Collaborator

@bdach bdach Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, unsure on this being a separate archive type and all. I think outputting one archive, but with like an .osu2 inside for lazer could work better for users, because this right now just kinda feels really ugly.

I guess the issue with that other solution is that if someone exports from lazer, then edits in stable and reimports that after the stable edits, lazer may see the .osu2 file which stable didn't touch, therefore ignoring the changes from stable. Could compensate for it by adding another file which lazer would write, basically something like:

{"osu_hash":"${SHA2_OF_OSU_FILE}","osu2_hash":"${SHA2_OF_OSU2_FILE"}

which helps, because if lazer sees that, and then sees an .osu file that doesn't match the hash, then it can figure out which thing should take precedence.

Dunno, interested in more opinions. The main point I'm driving at is that I'd like to hide as much of this ugliness from users as possible. Which I guess is the exact opposite to your opinion that it should be very explicit.

Copy link
Member

@peppy peppy Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I wanted to do, but I believe @OliBomby had reasons for not wanting to implement things this way (as per your linked discussion). Will wait to hear his view on it these days.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion on this hasn't changed. I think it's better to have an explicit export option for legacy osu! because it will free the lazer file format from the constraints if osu stable, allowing for more changes to be made to the lazer file format without breaking compatibility with stable.

Considering that I predict the lazer file format will change more in the future, it doesnt make sense to use some half-legacy half-lazer Frankenstein's file format. Therefore I think the explicit is the simpler and more elegant solution.

Even if the lazer file format doesn't change, I think this explicit export will be easier to understand for users because it is just a simpler approach. The exported packages wont have any weird doubled .osu2 files and hashes which prompt the mapper to do a 10 hour investigation on what is really going on.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, well, I don't feel that strongly about a separate export option, but if we do end up having it, can we not have it as two top-level menu items and instead do something like

image

(copy up for discussion, obviously)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that. I'll add the submenu

@peppy
Copy link
Member

peppy commented Jul 12, 2023

osz2 as an extension and format actually already exists and is used and supported by stable (internally for BSS submission is the largest use case)...

maybe it doesn't drag and drop import, but to avoid confusion we probably want to use osz3 for now. or a new file extension if some kind.

osu.Game/Database/LegacyBeatmapExporter.cs Outdated Show resolved Hide resolved
osu.Game/Beatmaps/BeatmapManager.cs Outdated Show resolved Hide resolved

using var skinStream = UserFileStorage.GetStream(file.File.GetStoragePath());
using var skinStreamReader = new LineBufferedReader(contentStream);
var beatmapSkin = new LegacySkin(new SkinInfo(), null!)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why's this a plain LegacySkin rather than a LegacyBeatmapSkin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only need the GetConfig implementation from LegacySkin for LegacyBeatmapEncoder to get the correct combo colours. LegacyBeatmapSkin would also work but it has slightly more overhead.

@bdach
Copy link
Collaborator

bdach commented Jul 22, 2023

Tested this a tiny bit today. It appears to be working well for what it's supposed to be, and fixes the two major pain points reported by users (mangled sliders and mangled SVs).

I think I'd be fine to start with this and see how things unfold going forward. But I'm not merging without a second approval as there are a few key decisions made in here (e.g. the direction wrt a separate archive type with a separate extension) which - while I can agree with - I don't feel comfortable making alone.

bdach
bdach previously approved these changes Jul 22, 2023
@bdach bdach requested a review from peppy July 22, 2023 20:30
/// </summary>
public static LocalisableString ExportPackage => new TranslatableString(getKey(@"export_package"), @"Export package");
public static LocalisableString ExportForEditing => new TranslatableString(getKey(@"export_for_editing"), @"For editing (.olz)");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the terminology here. Reasoning is that I don't want to use "new"/"legacy" terminology, and this is the best way I can find to phrase things in a sensible way. Only other I can think of is "Lossless" for "For preservation", but I think "For editing" is most user friendly. It lets a user know that they should always be using olz if possible, because it is the preferred method for sharing for editing purposes.

I can understand if there may be a view that "for editing is confusing because what if i'm editing in stable?", but stable mappers know what format stable supports. Worst case they will try olz and it will fail and they'll understand.

@peppy peppy changed the title Add legacy beatmap export option Add ability to export beatmaps from editor in a stable-compatible format Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants