Implement legacy combo splash #17616

Feodor0090 · 2022-04-02T17:43:02Z

This is a MVP implementation of combo splashes from osu!stable. Supports multiple (comboburst-N.png) sprites.

Demo (BMS's skin, default skin, custom legacy skin):

2022-04-02.20-30-42.mp4

Closes #4629

osu.Game/Screens/Play/HUD/LegacyComboSplash.cs

bdach · 2022-04-02T18:30:32Z

Aside from the above, there are unresolved code inspections (please run inspectcode locally to check) and this component should come with a test scene to be able to preview it without launching the game.

The component is also missing several features and quirks from stable (among others: ruleset-specific burst sprites and burst combo thresholds, being able to toggle on/off the order randomisation, being able to display the burst from both sides of the screen). But I suppose this was signposted as an MVP, just not sure how viable it is given all of the aforementioned omissions.

Also the Z-order is weird - as is the burst can display on top of the combo counter and other HUD elements which seems undesirable.

Feodor0090 · 2022-04-02T23:54:29Z

ruleset-specific

Where is the correct place to place these 2 settings (threshold check and sprite name)? I think, inside Ruleset?

being able to display the burst from both sides of the screen

To have the burst from both sides player is supposed to add a second component, flip it and anchor to the opposite side, but in this case both bursts will appear every time. Stable shows the splash from the random side each time. I'm not sure how to implement this. Something like "share an bindable between all ComboSplashes that will somehow block ValueChangedEvent for everything except the first ComboSplash; it will choose random ComboSplash from available and trigger animation on it" may work, but feels weird.

Z-order

The latest added component to the skin is always on top. If this to be fixed locally by rearranging skin components somehow, it will become a problem again when sprite placing will be implemented. Maybe, it's better to handle this on skin editor level by adding "bring to back/front" options?

peppy · 2022-04-03T02:14:07Z

Where is the correct place to place these 2 settings (threshold check and sprite name)? I think, inside Ruleset?

You would make a specific return of the component in each ruleset's LegacyTransformer. And potentially falback to a default at LegacySkin for rulesets that don't have support.

To have the burst from both sides player is supposed to add a second component, flip it and anchor to the opposite side, but in this case both bursts will appear every time. Stable shows the splash from the random side each time. I'm not sure how to implement this. Something like "share an bindable between all ComboSplashes that will somehow block ValueChangedEvent for everything except the first ComboSplash; it will choose random ComboSplash from available and trigger animation on it" may work, but feels weird.

You'd make the component handle both sides, rather than having two separate components.

The latest added component to the skin is always on top. If this to be fixed locally by rearranging skin components somehow, it will become a problem again when sprite placing will be implemented. Maybe, it's better to handle this on skin editor level by adding "bring to back/front" options?

This is out of scope. Handle the default case, not the skin editor case.

Feodor0090 · 2022-04-03T19:51:33Z

I'm still not sure how to properly override sprites. Does GetDrawableComponent in legacy transformer fire before component's BDL?

peppy · 2022-04-04T03:50:15Z

I'm still not sure how to properly override sprites. Does GetDrawableComponent in legacy transformer fire before component's BDL?

If you don't want to derive the LegacyComboSplash component, you can nest a skin component inside your component and make an OsuSkinComponents lookup for it. Then you can implement the logic in each ruleset's transformer mapping that lookup to the correct sprite name. Something like this:

diff --git a/osu.Game.Rulesets.Osu/OsuSkinComponents.cs b/osu.Game.Rulesets.Osu/OsuSkinComponents.cs
index 71657ed532..c5f4e5d351 100644
--- a/osu.Game.Rulesets.Osu/OsuSkinComponents.cs
+++ b/osu.Game.Rulesets.Osu/OsuSkinComponents.cs
@@ -20,5 +20,6 @@ public enum OsuSkinComponents
         SliderBody,
         SpinnerBody,
         ApproachCircle,
+        ComboSplash
     }
 }
diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs
index 900ad6f6d3..dbf2480021 100644
--- a/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs
+++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs
@@ -32,8 +32,8 @@ public override Drawable GetDrawableComponent(ISkinComponent component)
             {
                 switch (osuComponent.Component)
                 {
-                    case OsuSkinComponents.FollowPoint:
-                        return this.GetAnimation(component.LookupName, true, true, true, startAtCurrentTime: false);
+                    case OsuSkinComponents.ComboSplash:
+                        return this.GetAnimation("osu-combo-splash", false, false);
 
                     case OsuSkinComponents.SliderFollowCircle:
                         var followCircle = this.GetAnimation("sliderfollowcircle", true, true, true);

Let me know if this doesn't make sense (I haven't tested so I may be missing something).

Feodor0090 · 2022-04-05T23:21:37Z

This didn't allow to abstract from rulesets due to deriving from gameplay skin elements, if i understood correctly.
I created local ISkinComponent subclass, and used it to get a container for sprites with specific names via GetDrawableComponent. Not sure that i done everything properly.
Also, seems that only pippi's sprite is presented in game resources. Sprites for catch and mania are missed.

bdach · 2022-04-06T21:03:01Z

osu.Game/Screens/Play/HUD/LegacyComboSplash.cs

+        [SettingSource("Side, where bursts will appear")]
+        public Bindable<Side> BurstsSide { get; } = new Bindable<Side>(Side.Random);


Not sure about making this the new sort of setting rather than hooking it up to the old setting flow that reads from skin.ini. This is kind of uncharted territory still.

Afaik there was no way to change this in stable (for osu! and osu!taiko) and there is no setting for this in skin.ini. I want to implement this here to make the component more flexible, so decided to use "modern" setting.

A mania-specific option for this exists, but it doesn't suit the behaviour implemented here (anchors are screen's sides, not playfield's ones):

Should i use it instead of "modern" setting anyway?

bdach · 2022-04-06T21:05:20Z

osu.Game/Screens/Play/HUD/LegacyComboSplash.cs

+        [SettingSource("Side, where bursts will appear")]
+        public Bindable<Side> BurstsSide { get; } = new Bindable<Side>(Side.Random);
+
+        public Bindable<int> Current { get; } = new BindableInt { MinValue = 0 };


This bindable shouldn't be public (or if you really want it to be, it should be externally immutable by exposing as IBindable<int>).

bdach · 2022-04-06T21:05:42Z

osu.Game/Screens/Play/HUD/LegacyComboSplash.cs

+
+        private void OnNewCombo(int combo)
+        {
+            if (BurstCondition(combo))


Would invert and early-return to reduce nesting.

bdach · 2022-04-06T21:11:05Z

osu.Game/Screens/Play/HUD/LegacyComboSplash.cs

+
+namespace osu.Game.Screens.Play.HUD
+{
+    public class LegacyComboSplash : Container<Container<Sprite>>, ISkinnableDrawable


This generic type (Container<Container<Sprite>>) is a bit much. Is this just for the test? If so, I'd recommend switching over to .ChildrenOfType<>() there.

bdach · 2022-04-06T21:11:43Z

osu.Game/Screens/Play/HUD/LegacyComboSplash.cs

+            Both,
+            Random


In stable both is random, for what it's worth. Not sure a new option needs to be invented here.

peppy · 2022-04-21T08:20:00Z

osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs

+                case SkinnableTargetComponent targetComponent:
+                    if (targetComponent.Target == SkinnableTarget.MainHUDComponents)
+                    {
+                        var components = base.GetDrawableComponent(component) as SkinnableTargetComponentsContainer;
+                        if (components == null) return null;
+
+                        foreach (var comboSplash in components.OfType<LegacyComboSplash>())
+                        {
+                            comboSplash.BurstCondition = combo => combo > 0 && combo % 100 == 0;
+                        }
+
+                        return components;
+                    }
+
+                    break;


Rather than doing this, it will be cleaner if you make a ManiaLegacyComboSplashSide and return it below, and then include the conditional logic within the subclass (you can likely make it a virutal / override pair if you do this).

peppy · 2022-09-09T09:25:03Z

I've brought this up-to-date, but still needs some work to get it in a good state.

Implement legacy combo splash

f147470

pull-request-size bot added the size/M label Apr 2, 2022

frenzibyte added the area:skinning label Apr 2, 2022

bdach requested changes Apr 2, 2022

View reviewed changes

osu.Game/Screens/Play/HUD/LegacyComboSplash.cs Outdated Show resolved Hide resolved

osu.Game/Screens/Play/HUD/LegacyComboSplash.cs Outdated Show resolved Hide resolved

osu.Game/Screens/Play/HUD/LegacyComboSplash.cs Outdated Show resolved Hide resolved

Feodor0090 added 2 commits April 3, 2022 01:35

Fix CQ issues

d283228

Make animation be closer to stable

0817a2e

Feodor0090 added 5 commits April 3, 2022 14:48

Implement bursting from random/both sides

61cb5c5

Force combo splash always be behind other components

2b9cf5a

Add visual test

fe25868

Add ability to override combo milestones

9e97953

Override combo milestones for osu!mania

dacf314

pull-request-size bot added size/L and removed size/M labels Apr 3, 2022

Feodor0090 added 3 commits April 6, 2022 01:59

Extract LegacyComboSplashSide subcomponent

7f9f224

Implement ruleset-specific burst sprites lookups

2385de6

Attempt to resolve inspections

c1f00f3

bdach reviewed Apr 6, 2022

View reviewed changes

Feodor0090 and others added 6 commits April 7, 2022 15:33

Fix bindable and condition

6143663

Use ChildrenOfType instead of generic container

d3c1ddf

Remove ability to use both sides simultaneously

0c6d3d8

Implement ability to disable sprites order randomization

183a2d1

Merge branch 'master' into combo-splash2

6e7aa54

Convert if casts to switch statements wher possible

df9af9b

pull-request-size bot removed the size/L label Apr 21, 2022

pull-request-size bot added the size/XL label Apr 21, 2022

peppy requested changes Apr 21, 2022

View reviewed changes

peppy self-requested a review September 9, 2022 09:12

peppy added 2 commits September 9, 2022 18:20

Merge branch 'master' into combo-splash2

b627ef8

Fix merge conflict failures

15df509

peppy marked this pull request as draft June 16, 2023 06:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement legacy combo splash #17616

Implement legacy combo splash #17616

Feodor0090 commented Apr 2, 2022 •

edited by peppy

Loading

bdach commented Apr 2, 2022 •

edited

Loading

Feodor0090 commented Apr 2, 2022

peppy commented Apr 3, 2022

Feodor0090 commented Apr 3, 2022

peppy commented Apr 4, 2022 •

edited

Loading

Feodor0090 commented Apr 5, 2022 •

edited

Loading

bdach Apr 6, 2022

Feodor0090 Apr 7, 2022

bdach Apr 6, 2022

bdach Apr 6, 2022

bdach Apr 6, 2022

bdach Apr 6, 2022

peppy Apr 21, 2022 •

edited

Loading

peppy commented Sep 9, 2022

		[SettingSource("Side, where bursts will appear")]
		public Bindable<Side> BurstsSide { get; } = new Bindable<Side>(Side.Random);

Implement legacy combo splash #17616

Are you sure you want to change the base?

Implement legacy combo splash #17616

Conversation

Feodor0090 commented Apr 2, 2022 • edited by peppy Loading

bdach commented Apr 2, 2022 • edited Loading

Feodor0090 commented Apr 2, 2022

peppy commented Apr 3, 2022

Feodor0090 commented Apr 3, 2022

peppy commented Apr 4, 2022 • edited Loading

Feodor0090 commented Apr 5, 2022 • edited Loading

bdach Apr 6, 2022

Choose a reason for hiding this comment

Feodor0090 Apr 7, 2022

Choose a reason for hiding this comment

bdach Apr 6, 2022

Choose a reason for hiding this comment

bdach Apr 6, 2022

Choose a reason for hiding this comment

bdach Apr 6, 2022

Choose a reason for hiding this comment

bdach Apr 6, 2022

Choose a reason for hiding this comment

peppy Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

peppy commented Sep 9, 2022

Feodor0090 commented Apr 2, 2022 •

edited by peppy

Loading

bdach commented Apr 2, 2022 •

edited

Loading

peppy commented Apr 4, 2022 •

edited

Loading

Feodor0090 commented Apr 5, 2022 •

edited

Loading

peppy Apr 21, 2022 •

edited

Loading