From b2af49c1021d904a61e679835bd3cf2073bee170 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 4 Jul 2024 18:04:38 +0900 Subject: [PATCH 1/3] Fix classic fallback not having a transformer (and only add if required) --- .../Skinning/BeatmapSkinProvidingContainer.cs | 34 +++++++++++-------- .../Skinning/RulesetSkinProvidingContainer.cs | 20 +++++++---- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs b/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs index 4486c8a9f0..94c7a3aac6 100644 --- a/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs +++ b/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -17,9 +15,9 @@ namespace osu.Game.Skinning /// public partial class BeatmapSkinProvidingContainer : SkinProvidingContainer { - private Bindable beatmapSkins; - private Bindable beatmapColours; - private Bindable beatmapHitsounds; + private Bindable beatmapSkins = null!; + private Bindable beatmapColours = null!; + private Bindable beatmapHitsounds = null!; protected override bool AllowConfigurationLookup { @@ -68,11 +66,15 @@ namespace osu.Game.Skinning } private readonly ISkin skin; + private readonly ISkin? classicFallback; - public BeatmapSkinProvidingContainer(ISkin skin) + private Bindable currentSkin = null!; + + public BeatmapSkinProvidingContainer(ISkin skin, ISkin? classicFallback = null) : base(skin) { this.skin = skin; + this.classicFallback = classicFallback; } protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) @@ -93,15 +95,19 @@ namespace osu.Game.Skinning beatmapColours.BindValueChanged(_ => TriggerSourceChanged()); beatmapHitsounds.BindValueChanged(_ => TriggerSourceChanged()); - // If the beatmap skin looks to have skinnable resources, add the default classic skin as a fallback opportunity. - if (skin is LegacySkinTransformer legacySkin && legacySkin.IsProvidingLegacyResources) + currentSkin = skins.CurrentSkin.GetBoundCopy(); + currentSkin.BindValueChanged(_ => { - SetSources(new[] - { - skin, - skins.DefaultClassicSkin - }); - } + bool userSkinIsLegacy = skins.CurrentSkin.Value is LegacySkin; + bool beatmapProvidingResources = skin is LegacySkinTransformer legacySkin && legacySkin.IsProvidingLegacyResources; + + // If the beatmap skin looks to have skinnable resources and the user's skin choice is not a legacy skin, + // add the default classic skin as a fallback opportunity. + if (!userSkinIsLegacy && beatmapProvidingResources && classicFallback != null) + SetSources(new[] { skin, classicFallback }); + else + SetSources(new[] { skin }); + }, true); } } } diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 07e238243b..d736f4cdb5 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -28,25 +28,33 @@ namespace osu.Game.Skinning protected readonly Ruleset Ruleset; protected readonly IBeatmap Beatmap; + [CanBeNull] + private readonly ISkin beatmapSkin; + /// /// This container already re-exposes all parent sources in a ruleset-usable form. /// Therefore disallow falling back to any parent any further. /// protected override bool AllowFallingBackToParent => false; - protected override Container Content { get; } + protected override Container Content { get; } = new Container + { + RelativeSizeAxes = Axes.Both, + }; public RulesetSkinProvidingContainer(Ruleset ruleset, IBeatmap beatmap, [CanBeNull] ISkin beatmapSkin) { Ruleset = ruleset; Beatmap = beatmap; + this.beatmapSkin = beatmapSkin; + } - InternalChild = new BeatmapSkinProvidingContainer(GetRulesetTransformedSkin(beatmapSkin)) + [BackgroundDependencyLoader] + private void load(SkinManager skinManager) + { + InternalChild = new BeatmapSkinProvidingContainer(GetRulesetTransformedSkin(beatmapSkin), GetRulesetTransformedSkin(skinManager.DefaultClassicSkin)) { - Child = Content = new Container - { - RelativeSizeAxes = Axes.Both, - } + Child = Content, }; } From e3c8bee7d0ec7afd1a4087fd5c68cb67aac28049 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 4 Jul 2024 19:45:41 +0900 Subject: [PATCH 2/3] Fix nullability failure --- osu.Game/Skinning/SkinProvidingContainer.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index acb15da80e..9aff187c9c 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -7,6 +7,7 @@ using System.Linq; using osu.Framework.Allocation; using osu.Framework.Audio.Sample; using osu.Framework.Bindables; +using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Extensions.TypeExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -201,7 +202,10 @@ namespace osu.Game.Skinning source.SourceChanged -= TriggerSourceChanged; } - skinSources = sources.Select(skin => (skin, new DisableableSkinSource(skin, this))).ToArray(); + skinSources = sources + // Shouldn't be required after NRT is applied to all calling sources. + .Where(skin => skin.IsNotNull()) + .Select(skin => (skin, new DisableableSkinSource(skin, this))).ToArray(); foreach (var skin in skinSources) { From f201cc3feaf9af6bed621d06c761595a9b1eb36e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 5 Jul 2024 10:09:06 +0900 Subject: [PATCH 3/3] Expand explanation in inline comment --- osu.Game/Skinning/BeatmapSkinProvidingContainer.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs b/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs index 94c7a3aac6..41fa7fcc66 100644 --- a/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs +++ b/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs @@ -101,8 +101,16 @@ namespace osu.Game.Skinning bool userSkinIsLegacy = skins.CurrentSkin.Value is LegacySkin; bool beatmapProvidingResources = skin is LegacySkinTransformer legacySkin && legacySkin.IsProvidingLegacyResources; - // If the beatmap skin looks to have skinnable resources and the user's skin choice is not a legacy skin, - // add the default classic skin as a fallback opportunity. + // Some beatmaps provide a limited selection of skin elements to add some visual flair. + // In stable, these elements will take lookup priority over the selected skin (whether that be a user skin or default). + // + // To replicate this we need to pay special attention to the fallback order. + // If a user has a non-legacy skin (argon, triangles) selected, the game won't normally fall back to a legacy skin. + // In turn this can create an unexpected visual experience. + // + // So here, check what skin the user has selected. If it's already a legacy skin then we don't need to do anything special. + // If it isn't, we insert the classic default. Note that this is only done if the beatmap seems to be providing skin elements, + // as we only want to override the user's (non-legacy) skin choice when required for beatmap skin visuals. if (!userSkinIsLegacy && beatmapProvidingResources && classicFallback != null) SetSources(new[] { skin, classicFallback }); else