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 edit beatmap content externally #28800

Merged
merged 19 commits into from
Jul 11, 2024
Merged

Conversation

peppy
Copy link
Member

@peppy peppy commented Jul 10, 2024

At a low level, this will work for all realm models (and can be reused easily for skins). I'll look at implementing for the skin editor separately though.

osu.2024-07-10.at.09.20.55.mp4

Closes #18902

await inStream.CopyToAsync(outStream).ConfigureAwait(false);
}

return new ExternalEditOperation<TModel>(this, model, mountedPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Q:
Would it be better to have fixed mounted path? It will let user easier to load the project like vscode.
Also, Lazer can only edit one beatmap one time, so there's no need to give the folder a hash?

Copy link
Collaborator

@bdach bdach Jul 10, 2024

Choose a reason for hiding this comment

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

Except filesystems are not as durable as you think they are and reusing a path leads to many issues like "oh this folder isn't writable because someone broke something" "oh this directory is being used by something else and therefore cannot be written to" "oh files got left over here and now random stuff is going to make it in".

I agree with the choice of not using a fixed path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... I'll have a think about this one. I can see the benefits for having it fixed, but as @bdach touches on it would need extra error handling.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should be considered as a follow-up effort if ever.

If we're to allow this, we'd probably not want to delete the files when re-importing so the user can keep an external editor open or something. It has a lot of considerations and a lot of opportunity for breakage.

Would probably say that it's a lower priority and time is best spent elsewhere now that we have the initial support in, unless we get a huge number of complaints from users that what has been implemented doesn't meet their needs.

osu.Game/Screens/Edit/ExternalEditScreen.cs Outdated Show resolved Hide resolved
osu.Game/Database/ExternalEditOperation.cs Show resolved Hide resolved
osu.Game/Screens/Edit/ExternalEditScreen.cs Outdated Show resolved Hide resolved
@bdach
Copy link
Collaborator

bdach commented Jul 10, 2024

Beatmaps that are edited through this flow do not receive "locally modified" status and instead appear to have none.

2024-07-10.12-54-54.mp4

I also got to a state once where this started getting thrown:

2024-07-10 10:44:15 [error]: :
2024-07-10 10:44:15 [error]: NUnit.Framework.AssertionException: :
2024-07-10 10:44:15 [error]: at osu.Framework.Logging.ThrowingTraceListener.Fail(String message1, String message2)
2024-07-10 10:44:15 [error]: at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage)
2024-07-10 10:44:15 [error]: at System.Diagnostics.Debug.Fail(String message, String detailMessage)
2024-07-10 10:44:15 [error]: at osu.Game.Database.ModelManager`1.<>c__DisplayClass12_0.<performFileOperation>b__0(Realm realm) in D:\Open Source\osu\osu.Game\Database\ModelManager.cs:line 56
2024-07-10 10:44:15 [error]: at osu.Game.Database.RealmExtensions.Write(Realm realm, Action`1 function) in D:\Open Source\osu\osu.Game\Database\RealmExtensions.cs:line 56
2024-07-10 10:44:15 [error]: at osu.Game.Database.ModelManager`1.performFileOperation(TModel item, Action`1 operation) in D:\Open Source\osu\osu.Game\Database\ModelManager.cs:line 53
2024-07-10 10:44:15 [error]: at osu.Game.Database.ModelManager`1.DeleteFile(TModel item, RealmNamedFileUsage file) in D:\Open Source\osu\osu.Game\Database\ModelManager.cs:line 37
2024-07-10 10:44:15 [error]: at osu.Game.Beatmaps.BeatmapManager.<>c__DisplayClass40_0.<save>b__0(Realm r) in D:\Open Source\osu\osu.Game\Beatmaps\BeatmapManager.cs:line 471
2024-07-10 10:44:15 [error]: at osu.Game.Database.RealmExtensions.Write(Realm realm, Action`1 function) in D:\Open Source\osu\osu.Game\Database\RealmExtensions.cs:line 56
2024-07-10 10:44:15 [error]: at osu.Game.Database.RealmAccess.Write(Action`1 action) in D:\Open Source\osu\osu.Game\Database\RealmAccess.cs:line 494
2024-07-10 10:44:15 [error]: at osu.Game.Beatmaps.BeatmapManager.save(BeatmapInfo beatmapInfo, IBeatmap beatmapContent, ISkin beatmapSkin, Boolean transferCollections) in D:\Open Source\osu\osu.Game\Beatmaps\BeatmapManager.cs:line 454
2024-07-10 10:44:15 [error]: at osu.Game.Beatmaps.BeatmapManager.Save(BeatmapInfo beatmapInfo, IBeatmap beatmapContent, ISkin beatmapSkin) in D:\Open Source\osu\osu.Game\Beatmaps\BeatmapManager.cs:line 305
2024-07-10 10:44:15 [error]: at osu.Game.Screens.Edit.Editor.Save() in D:\Open Source\osu\osu.Game\Screens\Edit\Editor.cs:line 535

Not sure what I did precisely to trigger it, I was doing a lot of exiting and re-entering the mounting screen as it was doing its things to see how it reacts. Full log here, maybe it'll help: 1720608222.runtime.log

@peppy
Copy link
Member Author

peppy commented Jul 10, 2024

Not sure what I did precisely to trigger it, I was doing a lot of exiting and re-entering the mounting screen as it was doing its things to see how it reacts.

I feel this may be fixed by b6741ee.. exiting the screen wasn't properly being blocked until completion.

osu.Game/Screens/Edit/Editor.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Edit/Editor.cs Outdated Show resolved Hide resolved
@peppy
Copy link
Member Author

peppy commented Jul 11, 2024

I've applied all feedback, added error handling and (hopefully) simplified things a bit.

@bdach bdach self-requested a review July 11, 2024 07:14
@bdach
Copy link
Collaborator

bdach commented Jul 11, 2024

I've applied follow-up changes to address some remaining concerns of mine, please check them.

I'm sure we'll get some reports about some things in this flow not quite working but I'm not willing to go fish myself so let's get this in and see what happens.

@peppy
Copy link
Member Author

peppy commented Jul 11, 2024

Changes look fine 👍 .

@bdach bdach merged commit f1fa34e into ppy:master Jul 11, 2024
11 of 17 checks passed
@peppy peppy deleted the file-mounting-v3 branch July 11, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Better file management
4 participants