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

Implement skinnable mod display #30993

Merged
merged 13 commits into from
Dec 21, 2024
Merged

Implement skinnable mod display #30993

merged 13 commits into from
Dec 21, 2024

Conversation

Tom94
Copy link
Contributor

@Tom94 Tom94 commented Dec 7, 2024

Addresses #30596

Also makes the mod display initialization sequence (start expanded, then unexpand) controlled by HUDOverlay rather than mod display itself. This enabled different treatment depending on whether the mod display is viewed in the skin editor or in the player.

Also makes the mod display initialization sequence (start expanded, then
unexpand) controlled by HUDOverlay rather than mod display itself. This
enabled different treatment depending on whether the mod display is
viewed in the skin editor or in the player.

Co-authored-by: Bartłomiej Dach <[email protected]>
@Tom94 Tom94 force-pushed the skinnable-mod-display branch from 5da21d8 to 0a00f7a Compare December 7, 2024 03:06
@peppy peppy self-requested a review December 7, 2024 04:13
bdach and others added 2 commits December 7, 2024 13:47
It was mostly a demonstrative thing to use in the heat in the moment for
the skinnable mod display and it breaks all other tests. So let's just
not.
@peppy
Copy link
Member

peppy commented Dec 7, 2024

Hmm, something looks to have changed here behaviour wise. During local gameplay, the mods now stay visible through the whole play whereas they used to fade away after a second or two.

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

As commented.

This was very weird on master - `ModDisplay` applied a fade-in on the
`iconsContainer` that lasted 1000ms, and `HUDOverlay` would stack
another 200ms fade-in on top if a replay was loaded. Moving that first
fadeout to a higher level broke fade-out because transforms got
overwritten.
@bdach
Copy link
Collaborator

bdach commented Dec 19, 2024

During local gameplay, the mods now stay visible through the whole play whereas they used to fade away after a second or two.

Should be fixed with 772ac2d.

@peppy peppy self-requested a review December 20, 2024 05:02
@peppy
Copy link
Member

peppy commented Dec 20, 2024

My remaining concerns are that there is now going to be a doubled-up mod display when in replay mode, and during the start of gameplay. I guess the fully "correct" way to implement this would be to migrate that element to the "break" or "intro" layer of the skinning system, which we don't have prepared just yet.

@bdach I guess you're okay with this being the case for now? I'm pretty indifferent as by default a user is not inconvenienced by this. It's just an oddity if you have added this new element.


A more important concern for me is that this looks weird in the skin editor when viewing with no mods:

osu! 2024-12-20 at 05 11 01

This would be an amicable fix for me:

diff --git a/osu.Game/Screens/Play/HUD/ModDisplay.cs b/osu.Game/Screens/Play/HUD/ModDisplay.cs
index 417ce355a5..e0b149191a 100644
--- a/osu.Game/Screens/Play/HUD/ModDisplay.cs
+++ b/osu.Game/Screens/Play/HUD/ModDisplay.cs
@@ -22,6 +22,8 @@ namespace osu.Game.Screens.Play.HUD
     /// </summary>
     public partial class ModDisplay : CompositeDrawable, IHasCurrentValue<IReadOnlyList<Mod>>
     {
+        public const float INITIAL_SCALE = 0.6f;
+
         private ExpansionMode expansionMode = ExpansionMode.ExpandOnHover;
 
         public ExpansionMode ExpansionMode
@@ -72,6 +74,7 @@ public ModDisplay(bool showExtendedInformation = true)
             this.showExtendedInformation = showExtendedInformation;
 
             AutoSizeAxes = Axes.Both;
+            Scale = new Vector2(INITIAL_SCALE);
 
             InternalChild = iconsContainer = new ReverseChildIDFillFlowContainer<ModIcon>
             {
@@ -93,7 +96,7 @@ private void updateDisplay(ValueChangedEvent<IReadOnlyList<Mod>> mods)
             iconsContainer.Clear();
 
             foreach (Mod mod in mods.NewValue.AsOrdered())
-                iconsContainer.Add(new ModIcon(mod, showExtendedInformation: showExtendedInformation) { Scale = new Vector2(0.6f) });
+                iconsContainer.Add(new ModIcon(mod, showExtendedInformation: showExtendedInformation));
         }
 
         private void updateExpansionMode(double duration = 500)
diff --git a/osu.Game/Screens/Play/HUD/SkinnableModDisplay.cs b/osu.Game/Screens/Play/HUD/SkinnableModDisplay.cs
index 819484e8ba..cfcc5c8163 100644
--- a/osu.Game/Screens/Play/HUD/SkinnableModDisplay.cs
+++ b/osu.Game/Screens/Play/HUD/SkinnableModDisplay.cs
@@ -10,6 +10,8 @@
 using osu.Game.Rulesets.Mods;
 using osu.Game.Skinning;
 using osu.Game.Localisation.SkinComponents;
+using osu.Game.Rulesets.UI;
+using osuTK;
 
 namespace osu.Game.Screens.Play.HUD
 {
@@ -32,7 +34,13 @@ public partial class SkinnableModDisplay : CompositeDrawable, ISerialisableDrawa
         [BackgroundDependencyLoader]
         private void load()
         {
-            InternalChild = modDisplay = new ModDisplay();
+            InternalChildren = new Drawable[]
+            {
+                // Provide a minimum autosize.
+                new Container { Size = ModIcon.MOD_ICON_SIZE * ModDisplay.INITIAL_SCALE },
+                modDisplay = new ModDisplay(),
+            };
+
             modDisplay.Current = mods;
             AutoSizeAxes = Axes.Both;
         }

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

As commented.

@bdach
Copy link
Collaborator

bdach commented Dec 20, 2024

My remaining concerns are that there is now going to be a doubled-up mod display when in replay mode, and during the start of gameplay. I guess the fully "correct" way to implement this would be to migrate that element to the "break" or "intro" layer of the skinning system, which we don't have prepared just yet.

@bdach I guess you're okay with this being the case for now? I'm pretty indifferent as by default a user is not inconvenienced by this. It's just an oddity if you have added this new element.

As you said, I'm not sure how I would resolve this right now. There are no tools to do this, and I don't want to make any changes to skinning systems since I see that as "your turf" so to speak.

A more important concern for me is that this looks weird in the skin editor when viewing with no mods:
(...)
This would be an amicable fix for me:

Applied, although I had to back out the part that pulled up the scale application to a higher level, since it broke the mod display on the footer which has scale applied to it directly:

// must be created in ctor for correct operation of `Current`.
modDisplay = new ModDisplay
{
Anchor = Anchor.Centre,
Origin = Anchor.Centre,
Scale = new Vector2(0.8f),
ExpansionMode = ExpansionMode.AlwaysContracted,
};

@peppy peppy self-requested a review December 20, 2024 12:26
@peppy peppy merged commit cf987bc into ppy:master Dec 21, 2024
10 checks passed
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.

4 participants