Add skin mounting flow #30226

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

osu.mounting.v2.mp4

Similarly to the analogous option in beatmap editor, this allows arbitrary external modifications to skins. In particular this means that it is now possible to change gameplay assets without exporting and re-emporting the skin.

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.

…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.

osu.Game/Database/ModelManager.cs

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?

self-resolved via 2e0e7ff

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.

osu.Game/Skinning/SkinImporter.cs

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

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

osu.Game/Database/ModelManager.cs

osu.Game/Skinning/SkinImporter.cs

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

osu.Game/Overlays/ExternalEditOverlay.cs

osu.Game/Skinning/SkinImporter.cs

Peppy spoke about using a shortcut and/or hashes to determine if the skin.ini is changed, and if so, then to rename the skin. In my opinion, hashing and doing numerous comparisons is probably less efficient than just syncing the SkinInfo's name during the update. This is an easy solution that does what it needs to.

smallketchup82 · 2024-12-11T15:21:05Z

Since the last review has been addressed, this should be good to go for another round.

bdach · 2024-12-12T03:21:31Z

@smallketchup82 resolved conflicts for you, please double check when able

smallketchup82 · 2024-12-12T13:45:18Z

@bdach Thanks! I didn't notice the merge conflicts. Your resolution looks good to me.

bdach

The settings menu can be opened when the skin is externally mounted. Meaning you can change skin while the skin is externally mounted. Doing this results in fundamentally broken behaviour and it should not even be permissible to enter such a state.

Also disallows using the random skin keybind when the external edit overlay is open. SkinEditor should already be disabling it, but I figured we might as well add this in for redundancy

smallketchup82 · 2024-12-16T16:36:34Z

Thanks for catching that! I've fixed it in 7a5e613. Not sure if that's how or where you wanted it to be done, but it works well and disallows opening the settings menu when the external edit overlay is visible.

bdach · 2025-01-06T09:44:39Z

osu.Game/Overlays/ExternalEditOverlay.cs

I heavily dislike basically the entirety of 7a5e613 and I think the root cause of all that is this class. Why is this an overlay at all? Why is it not a blocking screen like the editor implementation of this is?

Making the external skin edit thing a blocking screen should fix most of the problems that commit is trying to special-case. With the exception of the random skin hotkey, probably - that one I would argue needs better high-level handling, via the current skin bindable getting disabled or something. And even the commit message says the random thing handling may not be necessary at all...?

I created this as an overlay because I initially designed it to work around the skin editor. To me, an overlay on top of another overlay seems simpler than hiding the skin editor overlay, pushing the external edit screen, and then showing the skin editor overlay once that’s done, or any other method that involves screen usage. This was a choice I made for conceptual simplicity. My entire idea was to add the overlay version of the external edit screen for the skin editor, and then move the beatmap editor to use the overlay version in a future PR.

To be blunt, if your only issue is 7a5e613, I can revert that. It was an addition to future-proof the external edit overlay (for example, if you mount a skin outside the context of the skin editor). However, as the commit description states, it’s optional and redundant since the skin editor already performs the same task.

To be blunt, if your only issue is 7a5e613, I can revert that

Uh... how does that work? That commit's fixing #30226 (review) is it not? At least the weird explicit "suppress settings being opened if the external edit overlay is visible" logic would need to remain, would it not?

Basically the fact that OsuGame has to get involved in interactions between these two extremely specific pieces of the game is making the commit really hard to accept for me.

To me, an overlay on top of another overlay seems simpler than hiding the skin editor overlay, pushing the external edit screen, and then showing the skin editor overlay once that’s done, or any other method that involves screen usage

Did you actually try it?

The idea of pushing a screen (if we're assuming the screen will be pushed to the game's main stack) likely isn't going to work well with gameplay, which is one of the common cases for the skin editor.

Well then I don't know what the solution is, but I stand rather unmoved in opposition to OsuGame having to coordinate these two specific pieces deep inside the game. It feels not good. OsuGame is the last resort for stuff like that.

I haven't looked into the PR code yet, but can agree that it definitely should not live in OsuGame.

Sorry for the delay in response, I've been busy studying for exams.

At least the weird explicit "suppress settings being opened if the external edit overlay is visible" logic would need to remain, would it not?

Yes, that's my bad. You're right here.

Did you actually try it?

On a local branch yes. It suffers the issue that peppy mentioned (opening skin editor then pressing external edit mid gameplay takes you outside of gameplay). This happens even when paused which takes a good amount of usability away from this feature. I can confirm that it otherwise works.

but I stand rather unmoved in opposition to OsuGame having to coordinate these two specific pieces deep inside the game. It feels not good

I have no issue with moving it out of OsuGame, the question I have is where? Should I move this settings panel side? Or overlay side (i.e. DI settings panel to ExternalEditOverlay and disable it within ExternalEditOverlay)? A bit of direction here would be useful.

self-resolved via b82bf22

…ation

You can't hide a drawable outside of update thread.

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.

bdach · 2025-06-30T12:19:32Z

I've done a pass on this to hopefully bring it over the finish line but disclaimer that I am letting tests run on CI and have not attempted to run tests locally. Testing was mostly manual.

If nothing funny falls out of CI I'll request a second pass over the code because I changed much.

bdach · 2025-06-30T12:20:34Z

osu.Game/Database/RealmAccess.cs

+        /// Write changes to realm asynchronously, guaranteeing order of execution.
+        /// </summary>
+        /// <param name="action">The work to run.</param>
+        public Task<T> WriteAsync<T>(Func<Realm, T> action)


This is just an overload copy-pasted from non-generic WriteAsync() and adjusted to work, to support returning data from WriteAsync() calls.

peppy · 2025-07-01T09:04:51Z

osu.Game/OsuGame.cs

@@ -1227,7 +1225,7 @@ protected override void LoadComplete()
            loadComponentSingleFile(beatmapSetOverlay = new BeatmapSetOverlay(), overlayContent.Add, true);
            loadComponentSingleFile(wikiOverlay = new WikiOverlay(), overlayContent.Add, true);
            loadComponentSingleFile(skinEditor = new SkinEditorOverlay(ScreenContainer), overlayContent.Add, true);
-            loadComponentSingleFile(externalEditOverlay = new ExternalEditOverlay(), overlayContent.Add, true);
+            loadComponentSingleFile(new ExternalEditOverlay(), overlayContent.Add, true);


A bit painful to have this at a global level like this.

diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs index 9e524878dc..394917dc62 100644 --- a/osu.Game/OsuGame.cs +++ b/osu.Game/OsuGame.cs @@ -1225,7 +1225,6 @@ protected override void LoadComplete() loadComponentSingleFile(beatmapSetOverlay = new BeatmapSetOverlay(), overlayContent.Add, true); loadComponentSingleFile(wikiOverlay = new WikiOverlay(), overlayContent.Add, true); loadComponentSingleFile(skinEditor = new SkinEditorOverlay(ScreenContainer), overlayContent.Add, true); - loadComponentSingleFile(new ExternalEditOverlay(), overlayContent.Add, true); loadComponentSingleFile(new LoginOverlay { diff --git a/osu.Game/Overlays/SkinEditor/SkinEditorOverlay.cs b/osu.Game/Overlays/SkinEditor/SkinEditorOverlay.cs index 344dcc0d66..8b52c6b43c 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditorOverlay.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditorOverlay.cs @@ -49,9 +49,15 @@ public partial class SkinEditorOverlay : OverlayContainer, IKeyBindingHandler<Gl [Resolved] private IPerformFromScreenRunner? performer { get; set; } + [Resolved] + private IOverlayManager? overlayManager { get; set; } + [Cached] public readonly EditorClipboard Clipboard = new EditorClipboard(); + [Cached] + private readonly ExternalEditOverlay externalEditOverlay = new ExternalEditOverlay(); + [Resolved] private OsuGame game { get; set; } = null!; @@ -86,6 +92,13 @@ private void load(OsuConfigManager config) config.BindWith(OsuSetting.BeatmapSkins, beatmapSkins); } + protected override void LoadComplete() + { + base.LoadComplete(); + + overlayManager?.RegisterBlockingOverlay(externalEditOverlay); + } + public bool OnPressed(KeyBindingPressEvent<GlobalAction> e) { switch (e.Action)

could work? at least in manual testing it looks to still work.

applied with adjustments (disposal of the Disposable that RegisterBlockingOverlay() returns) in 2c22158

peppy

Fine apart from proposed change.

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

osu.Game/Database/ModelManager.cs Outdated Show resolved Hide resolved

osu.Game/Skinning/SkinImporter.cs Show resolved Hide resolved

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

osu.Game/Overlays/ExternalEditOverlay.cs Outdated Show resolved Hide resolved

Switch colour scheme to blue

15c7a12

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

cdwcgt reviewed Nov 5, 2024

View reviewed changes

osu.Game/Skinning/SkinImporter.cs Outdated Show resolved Hide resolved

Gabixel mentioned this pull request Dec 3, 2024

Osu skin name keeps getting longer #30940

Closed

Merge branch 'master' into skin-mounting

09d55ed

bdach added the area:skin-editor label Dec 16, 2024

bdach self-requested a review December 16, 2024 03:02

bdach requested changes Dec 16, 2024

View reviewed changes

Disallow opening settings menu when external edit ovelay is open

7a5e613

Also disallows using the random skin keybind when the external edit overlay is open. SkinEditor should already be disabling it, but I figured we might as well add this in for redundancy

bdach reviewed Jan 6, 2025

View reviewed changes

bdach added 8 commits June 30, 2025 12:53

Merge branch 'master' into skin-mounting

3688509

Move external edit overlay to more proper namespace

a95987a

Eliminate weird parameter passing

bccdf42

Use better method of disallowing skin changes during external edit

b82bf22

Ensure pending changes are saved before initiating external edit oper…

f63dc2d

…ation

Prevent hiding skin editor during external edit operation

17cfa7f

Fix error code paths just completely falling apart

5fb0449

You can't hide a drawable outside of update thread.

Fix dodgy threading

2e0e7ff

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.

bdach force-pushed the skin-mounting branch from 573a4ec to 2e0e7ff Compare June 30, 2025 12:17

bdach previously approved these changes Jun 30, 2025

View reviewed changes

bdach reviewed Jun 30, 2025

View reviewed changes

bdach requested review from peppy and removed request for peppy July 1, 2025 06:53

peppy reviewed Jul 1, 2025

View reviewed changes

Move external skin edit overlay out of OsuGame

2c22158

bdach dismissed their stale review via 2c22158 July 1, 2025 09:24

peppy approved these changes Jul 1, 2025

View reviewed changes

bdach added the notable feature Attach to pull requests which should be highlighted on the changelog. Things users want to read. label Jul 1, 2025

peppy merged commit 6470295 into ppy:master Jul 1, 2025
8 checks passed

	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>();

Uh oh!

Add skin mounting flow #30226

Add skin mounting flow #30226

Uh oh!

Conversation

smallketchup82 commented Oct 11, 2024 • edited by bdach Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smallketchup82 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smallketchup82 Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smallketchup82 Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smallketchup82 commented Oct 11, 2024

Uh oh!

Uh oh!

Uh oh!

peppy commented Oct 11, 2024

Uh oh!

peppy commented Oct 11, 2024

Uh oh!

smallketchup82 commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smallketchup82 commented Oct 12, 2024

Uh oh!

smallketchup82 commented Oct 13, 2024

Uh oh!

Uh oh!

Uh oh!

smallketchup82 commented Dec 11, 2024

Uh oh!

bdach commented Dec 12, 2024

Uh oh!

smallketchup82 commented Dec 12, 2024

Uh oh!

bdach left a comment

Choose a reason for hiding this comment

Uh oh!

smallketchup82 commented Dec 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smallketchup82 Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smallketchup82 Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdach commented Jun 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smallketchup82 commented Oct 11, 2024 •

edited by bdach

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 Jan 6, 2025 •

edited

Loading

smallketchup82 Jan 10, 2025 •

edited

Loading