Add skin mounting flow #30226

smallketchup82 · 2024-10-11T19:06:04Z

This pull request expands on the mounting logic from beatmaps and integrates it with the skin editor, thereby introducing external editing functionality for skins.

I have applied my current knowledge of realm and threading to ensure the code is at the very least presentable. However, I acknowledge that further refinement and evaluation may be necessary. I am open to any feedback and suggestions to improve this implementation.

This is intended to be a stopgap solution until Skin Editor gains support for editing skin files visually.

Here's a video demonstrating the feature:

osu.mounting.v2.mp4

…lay down framework for abstracting logic

The GetFile method in AddFile has a huge overhead, given we're doing this in a loop. Since we clear the files in the skin, we already know there won't be any existing files, so we can skip all of that logic

smallketchup82

Going to leave some comments here for reviewers on why I did certain things and any concerns I have. Apologies if I shouldn't be doing it this way, please correct me for future reference if so.

smallketchup82 · 2024-10-11T19:12:52Z

osu.Game/Database/ModelManager.cs

        /// </summary>
-        public void AddFile(TModel item, Stream contents, string filename, Realm realm)


As for why I touched this

From b7883f1 (#30226)

The GetFile method in AddFile has a huge overhead, given we're doing
this in a loop.

Since we clear the files in the skin, we already know there won't be any
existing files, so we can skip all of that logic

This makes no sense and offers zero safeties.

If the thing has too high overhead, we fix later. Not as part of a huge change. Remove this commit please.

smallketchup82 · 2024-10-11T19:15:13Z

osu.Game/Database/RealmArchiveModelImporter.cs

-            foreach (var realmFile in model.Files)
+            // Detach files from the model to avoid realm contention when copying to the external location.
+            // This is safe as we are not modifying the model in any way.
+            foreach (var realmFile in model.Files.Detach())


I ran into an issue where detaching the skin doesn't actually end up detaching the files, resulting in a Realms.Exceptions.RealmException: Realm accessed from incorrect thread. exception.

I couldn't find a way to fix it apart from this. Fortunately, we only access the realm files here to mount them, so we aren't modifying the files in a way that requires them to be attached.

Two questions:

Where / when is the "skin" being "detached"?

Does detach even... work for SkinInfo? It sure isn't registered in the automapper detach machinery.

osu/osu.Game/Database/RealmObjectExtensions.cs

Lines 171 to 179 in 74675c8

c.CreateMap<RealmKeyBinding, RealmKeyBinding>();

c.CreateMap<BeatmapMetadata, BeatmapMetadata>();

c.CreateMap<BeatmapUserSettings, BeatmapUserSettings>();

c.CreateMap<BeatmapDifficulty, BeatmapDifficulty>();

c.CreateMap<RulesetInfo, RulesetInfo>();

c.CreateMap<ScoreInfo, ScoreInfo>();

c.CreateMap<RealmUser, RealmUser>();

c.CreateMap<RealmFile, RealmFile>();

c.CreateMap<RealmNamedFileUsage, RealmNamedFileUsage>();

Maybe adding SkinInfo in there just fixes this. I dunno.

The skin is being detached here

CurrentSkin.Value.SkinInfo in SkinEditor returns a Live<SkinInfo>. My original idea for fixing the issue with "realm being accessed from an incorrect thread" was to detach the Live<SkinInfo> so that we're only working with a SkinInfo. Though, simply stopping here (without detaching Files) doesn't work and results in the thread error. Adding in the .Detach() to model.Files fixes it.

I got here via trial and error so I don't know for certain. But my best guess is that it likely has to do with RealmNamedFileUsage being in the automapper detach machinery in which you specified. Maybe it's only detaching the Files that ends up fixing the issue, not necessarily the detaching of the Live<SkinInfo>.

At any rate, I guess detaching the Live<SkinInfo> serves only to convert from Live<SkinInfo> to SkinInfo. But a quick test without detaching the Live<SkinInfo> seems to work fine. So maybe that's unnecessary?

smallketchup82 · 2024-10-11T19:18:28Z

osu.Game/Overlays/SkinEditor/SkinEditor.cs

+
+                // Create a new skin instance to ensure the skin is reloaded
+                // If there's a better way to reload the skin, this should be replaced with it.
+                currentSkin.Value = newSkinInfo.CreateInstance(skins);


This is where most of my confidence in the code quality of this PR declines. This was the only way I found that would refresh the skin instantly. I looked through at least 15 different classes, and even tests, to figure out a proper way of reloading skins. Was left empty handed and came up with this which works. The reason I feel unconfident about it is because it feels rather hacky.

smallketchup82 · 2024-10-11T19:22:10Z

osu.Game/Skinning/SkinImporter.cs

+            skinInfoLive.PerformWrite(skinInfo =>
+            {
+                // Not sure if this deletes the files from the storage or just the database.
+                skinInfo.Files.Clear();


As the comment states. Does this also flag the files for deletion? I tried to test it but couldn't conclude whether it does or doesn't. And ModelManager seems to do roughly the same in its DeleteFile method. So I'm a bit confused here.

Don't add comments for this. Ask elsewhere.

And yes, it does. Storage is managed as a separate layer based on file references.

smallketchup82 · 2024-10-11T19:32:24Z

Forgot tests, sorry about that. Will add them in ASAP

peppy · 2024-10-11T20:07:12Z

osu.Game/Database/ModelManager.cs

        /// </summary>
-        public void AddFile(TModel item, Stream contents, string filename, Realm realm)


This makes no sense and offers zero safeties.

If the thing has too high overhead, we fix later. Not as part of a huge change. Remove this commit please.

peppy · 2024-10-11T20:07:43Z

osu.Game/Skinning/SkinImporter.cs

+            skinInfoLive.PerformWrite(skinInfo =>
+            {
+                // Not sure if this deletes the files from the storage or just the database.
+                skinInfo.Files.Clear();


Don't add comments for this. Ask elsewhere.

And yes, it does. Storage is managed as a separate layer based on file references.

peppy · 2024-10-11T20:08:45Z

Forgot tests, sorry about that. Will add them in ASAP

Seems back to front. Like you should be creating the tests to develop the feature. Adding as an afterthought is mostly pointless for something like this? But let's see what you come up with.

This reverts commit b7883f1.

peppy · 2024-10-11T20:54:16Z

I don't like this "simple" approach. Maybe simple for you, but not for a user. And also means you could potentially exit the skin editor or make edits in it while mounted, then overwrite those edits.

There should be a big button for the user to click once done external editing.

smallketchup82 · 2024-10-11T21:33:55Z

I can look into making an overlay equivalent of ExternalEditScreen, though this might take me at least a week given my current schedule.

smallketchup82 · 2024-10-12T17:24:00Z

Sorry, misclick

smallketchup82 · 2024-10-13T00:58:58Z

I've managed to get an overlay component working. It's more or less a port of ExternalEditScreen to an overlay. With a bit of work it can be made compatible with beatmaps, and ExternalEditScreen can be replaced with it, so that the beatmap editor uses the overlay as well.

Here's a video demonstrating what the flow looks like now:

osu.mounting.v2.mp4

Figured this made sense since we're using purple buttons. It doesn't really seem to change anything visually though

peppy · 2024-10-13T17:02:45Z

osu.Game/Overlays/ExternalEditOverlay.cs

@@ -37,7 +37,7 @@ public partial class ExternalEditOverlay : OsuFocusedOverlayContainer
        private FillFlowContainer flow = null!;

        [Cached]
-        private readonly OverlayColourProvider colourProvider = new OverlayColourProvider(OverlayColourScheme.Pink);
+        private readonly OverlayColourProvider colourProvider = new OverlayColourProvider(OverlayColourScheme.Purple);


It should inherit from the skin editor's colours, no?

cdwcgt · 2024-11-05T13:27:00Z

osu.Game/Skinning/SkinImporter.cs

+            skinInfoLive.PerformWrite(skinInfo =>
+            {
+                skinInfo.Files.Clear();
+
+                string[] filesInMountedDirectory = Directory.EnumerateFiles(task.Path, "*.*", SearchOption.AllDirectories).Select(f => Path.GetRelativePath(task.Path, f)).ToArray();
+
+                foreach (string file in filesInMountedDirectory)
+                {
+                    using var stream = File.OpenRead(Path.Combine(task.Path, file));
+
+                    modelManager.AddFile(original, stream, file);
+                }
+            });


I think we need to update the skin‘s name and creator if they were modified externally?

The process should probably match the beatmap reimport one, optimally.

Maybe it can shortcut in the case that ini/json files didn't change, if the full import takes too long (although I'd like to hope it doesn't).

I was hoping that it would update automatically, since changing the skin.ini and creating a new skin instance for the SkinInfo sounds like it should update the skin's name in realm, but it probably isn't because we're reusing the old SkinInfo which would have the old Name property.

I'll probably end up implementing it as peppy suggested. Before processing the other files, I'll hash the two skin.ini's. If there's a change, I'll investigate what changed and reflect them in realm. If nothing was changed, we're good and we can skip that entire process. Though the full import process doesn't take very long and the shortcut might not be necessary.

smallketchup82 added 6 commits October 11, 2024 13:27

Initial implementation

4d09e94

Fix changes not being applied instantly, Improve import performance, …

9de248f

…lay down framework for abstracting logic

Abstract out logic to SkinImporter

a20bd5c

Add a toggle for checking overwriting

b7883f1

The GetFile method in AddFile has a huge overhead, given we're doing this in a loop. Since we clear the files in the skin, we already know there won't be any existing files, so we can skip all of that logic

Make everything translatable

ee16c96

Rename oldskin

2b7eb56

pull-request-size bot added the size/L label Oct 11, 2024

smallketchup82 commented Oct 11, 2024

View reviewed changes

peppy requested changes Oct 11, 2024

View reviewed changes

Revert "Add a toggle for checking overwriting"

0dc77a7

This reverts commit b7883f1.

Merge branch 'master' into skin-mounting

942efce

smallketchup82 added 2 commits October 12, 2024 17:15

Remove a comment

3d16e45

Switch to using an overlay interface for skin mounting

44fe7f2

Switch overlay colour scheme to purple

ca5a74d

Figured this made sense since we're using purple buttons. It doesn't really seem to change anything visually though

smallketchup82 changed the title ~~Add basic skin mounting flow~~ Add skin mounting flow Oct 13, 2024

peppy reviewed Oct 13, 2024

View reviewed changes

Switch colour scheme to blue

15c7a12

peppy self-requested a review October 23, 2024 07:34

cdwcgt reviewed Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add skin mounting flow #30226

Add skin mounting flow #30226

smallketchup82 commented Oct 11, 2024 •

edited

Loading

smallketchup82 left a comment •

edited

Loading

smallketchup82 Oct 11, 2024

peppy Oct 11, 2024

smallketchup82 Oct 11, 2024 •

edited

Loading

bdach Oct 14, 2024

smallketchup82 Oct 14, 2024 •

edited

Loading

smallketchup82 Oct 11, 2024

smallketchup82 Oct 11, 2024

peppy Oct 11, 2024

smallketchup82 commented Oct 11, 2024

peppy Oct 11, 2024

peppy Oct 11, 2024

peppy commented Oct 11, 2024

peppy commented Oct 11, 2024

smallketchup82 commented Oct 11, 2024 •

edited

Loading

smallketchup82 commented Oct 12, 2024

smallketchup82 commented Oct 13, 2024

peppy Oct 13, 2024

cdwcgt Nov 5, 2024

peppy Nov 6, 2024

smallketchup82 Nov 8, 2024 •

edited

Loading

		/// </summary>
		public void AddFile(TModel item, Stream contents, string filename, Realm realm)

	c.CreateMap<RealmKeyBinding, RealmKeyBinding>();
	c.CreateMap<BeatmapMetadata, BeatmapMetadata>();
	c.CreateMap<BeatmapUserSettings, BeatmapUserSettings>();
	c.CreateMap<BeatmapDifficulty, BeatmapDifficulty>();
	c.CreateMap<RulesetInfo, RulesetInfo>();
	c.CreateMap<ScoreInfo, ScoreInfo>();
	c.CreateMap<RealmUser, RealmUser>();
	c.CreateMap<RealmFile, RealmFile>();
	c.CreateMap<RealmNamedFileUsage, RealmNamedFileUsage>();

Add skin mounting flow #30226

Are you sure you want to change the base?

Add skin mounting flow #30226

Conversation

smallketchup82 commented Oct 11, 2024 • edited Loading

smallketchup82 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallketchup82 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallketchup82 Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallketchup82 commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppy commented Oct 11, 2024

peppy commented Oct 11, 2024

smallketchup82 commented Oct 11, 2024 • edited Loading

smallketchup82 commented Oct 12, 2024

smallketchup82 commented Oct 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallketchup82 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

smallketchup82 commented Oct 11, 2024 •

edited

Loading

smallketchup82 left a comment •

edited

Loading

smallketchup82 Oct 11, 2024 •

edited

Loading

smallketchup82 Oct 14, 2024 •

edited

Loading

smallketchup82 commented Oct 11, 2024 •

edited

Loading

smallketchup82 Nov 8, 2024 •

edited

Loading