Move combo counter to ruleset-specific HUD components container #26249

frenzibyte · 2023-12-30T02:57:13Z

Prerequisite for adding ruleset-specific combo counter implementations (e.g. mania). Also removes HiddenByRulesetImplementation in favour of overriding the default layout from within ruleset skin transformers.

For a ruleset to override the default display of the combo counter, it would handle lookups to MainHUDComponents with a non-null ruleset inside the ruleset skin transformer. A call to Skin.GetDrawableComponent is made first, to allow presenting user customisations. If null, then a default layout of the ruleset-specific combo counter is returned.

The pattern above will be taken into practice in follow-up PRs.

…etImplementation` entirely

osu.Game/Rulesets/Ruleset.cs

osu.Game/Skinning/LegacySkinTransformer.cs

osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs

bdach · 2024-06-24T07:54:31Z

@frenzibyte can you respond to reviews on this? would like to see movement here

frenzibyte · 2024-06-24T08:03:56Z

I've kept this away from my sight as I was focusing on song select redesign, but I'll try soon.

frenzibyte · 2024-06-24T22:39:29Z

A casualty from this PR occurred on all lazer-modified skins where the combo counter would be duplicated between non-ruleset and ruleset HUD targets. This is due to the skin containing custom layout on non-ruleset HUD target that includes a combo counter. Not sure how to easily fix...I will probably need to add some logic to migrate custom layout information of the combo counter to the correct target.

…et pairs

frenzibyte · 2024-06-25T01:41:08Z

The above comment should be resolved with 78e0126 (I may have improvised too much on the direction though, it works at least).

osu.Game/Skinning/SkinInfo.cs

osu.Game/Skinning/LegacySkin.cs

frenzibyte · 2024-07-01T05:08:40Z

osu.Game/Skinning/SkinLayoutInfo.cs

+        [JsonProperty(DefaultValueHandling = DefaultValueHandling.Populate)]
+        public int Version = LATEST_VERSION;


I don't know if I'm approaching this correctly but I'm using DefaultValueHandling.Populate so that when deserialising any skin prior to this PR, the value of Version will be assigned to 0 (default int value) instead of LATEST_VERSION.

The documentation on this is hilariously bad:

Members with a default value but no JSON will be set to their default value when deserializing.

Like that sounds like the inverse to the actual desired behaviour here. That said, as far as my real-world testing is concerned, this does appear to function as advertised...

frenzibyte · 2024-07-01T05:28:42Z

osu.Game/Skinning/Skin.cs

+                    resources.RealmAccess.Run(r =>
+                    {
+                        foreach (var ruleset in r.All<RulesetInfo>())
+                        {
+                            layout.Update(ruleset, layout.TryGetDrawableInfo(ruleset, out var rulesetHUDComponents)
+                                ? rulesetHUDComponents.Concat(comboCounters).ToArray()
+                                : comboCounters);
+                        }
+                    });


Not sure how well this will sit with everyone, but it's the easiest way to access the rulesets that the user has locally to populate the layout with.

Alternative approach would be to take the entirety of this migration logic to SkinManager as a post-process of selecting the skin (i.e. changing the CurrentSkinInfo bindable), so that it would at least make more sense to access realm or the ruleset store directly...but I'm not sure if it's really worth the effort over a simple Realm query in here (which could be further improved by adding the ruleset store to the IStorageResourceProvider interface).

No issues from me, not sure why I'd ever have a problem with this

…ation code

bdach

The migration logic is much better.

I'm fine with this in principle but will want a @peppy cross-check to ensure this does not interfere with any ongoing skinning changes in a dealbreaking way.

bdach · 2024-07-03T11:22:30Z

osu.Game/Skinning/Skin.cs

+            // handle namespace changes...
+            jsonContent = jsonContent.Replace(@"osu.Game.Screens.Play.SongProgress", @"osu.Game.Screens.Play.HUD.DefaultSongProgress");
+            jsonContent = jsonContent.Replace(@"osu.Game.Screens.Play.HUD.LegacyComboCounter", @"osu.Game.Skinning.LegacyComboCounter");
+            jsonContent = jsonContent.Replace(@"osu.Game.Screens.Play.HUD.PerformancePointsCounter", @"osu.Game.Skinning.Triangles.TrianglesPerformancePointsCounter");


I digress but I wonder how long this crap will have to live... I'd hope we can remove it in a few months or something.

Well, technically this can live on as part of the migration process for 100% compatibility, I was just unable to move it into the actual migration method because it manipulates the JSON string itself before deserialising it.

bdach · 2024-07-03T11:30:39Z

osu.Game/Skinning/Skin.cs

+                    resources.RealmAccess.Run(r =>
+                    {
+                        foreach (var ruleset in r.All<RulesetInfo>())
+                        {
+                            layout.Update(ruleset, layout.TryGetDrawableInfo(ruleset, out var rulesetHUDComponents)
+                                ? rulesetHUDComponents.Concat(comboCounters).ToArray()
+                                : comboCounters);
+                        }
+                    });


No issues from me, not sure why I'd ever have a problem with this

peppy · 2024-08-07T18:25:53Z

osu.Game/Skinning/ArgonSkinTransformer.cs

+                && containerLookup.Target == SkinComponentsContainerLookup.TargetArea.MainHUDComponents
+                && containerLookup.Ruleset != null)
+            {
+                return base.GetDrawableComponent(lookup) ?? new Container


I've hopefully resolved conflicts, but I still have a lot of questions.

Will start with a simple question: why is this here now? Why does ArgonSkinTransformer have to exist? Transformers were supposed to allow rulesets to do things, but you have changed what transformer means now, as this is being instantiated by Ruleset.

On further reading and understanding of this PR, the probable answer is "because I wanted to be able to provide a per-skin default for cases where a ruleset doesn't do anything special", does that sound right?

@frenzibyte I've applied 60d3834 which removes this stuff. please give it 5 minutes and let me know if it makes sense within that time limit.

That is correct.

@frenzibyte I've applied 60d3834 which removes this stuff. please give it 5 minutes and let me know if it makes sense within that time limit.

I...don't know. It looks correct, but I feel it's introducing another level of complexity to the lookup system in my head / when parsing this mentally. I wouldn't say that the way I had it is better, though.

This is all cheap talk and I cannot easily imagine how it can be put to code, but, it definitely feels like there should be two methods:

Skin.GetDrawableComponent, which tries to look up user configuration, then fall back to...

Skin.GetDefaultDrawableComponent, which returns default skin implementation, and that can be transformed by a skin transformer somehow.

I'm proposing this for Skin specifically because it is the class that introduces the concept of "user configuration".

This is all cheap talk and I cannot easily imagine how it can be put to code, but, it definitely feels like there should be two methods

Yep, in previous discussions with smoogi this was unanimously agreed upon, but I'm not doing that here. It will be a separate step after all your changes are merged.

I don't see any complexity being added here. I'm just explicitly defining which early returns are due to "skin has user layout".

As long as it's all temporary until a split happens then I'm fine looking away, I can't easily explain why this feels complicated in my head.

This didn't make any sense, so let's do it a better way.

frenzibyte added 2 commits December 30, 2023 03:47

Move combo counter to ruleset-specific HUD components target

eedb436

Refactor CatchLegacySkinTransformer logic and remove `HiddenByRules…

e469e06

…etImplementation` entirely

frenzibyte added the area:skinning label Dec 30, 2023

pull-request-size bot added the size/L label Dec 30, 2023

This was referenced Dec 30, 2023

Refactor classic osu!catch combo counter and place it in HUD overlay #26253

Closed

Add argon/classic osu!mania combo counter #26254

Merged

Merge branch 'master' into ruleset-specific-combo-counter

5fcea01

bdach reviewed Jun 4, 2024

View reviewed changes

frenzibyte added 2 commits June 25, 2024 01:10

Simplify default layout initialisation

fbc9989

Remove pointless assert

e8de293

Migrate combo counter layouts in custom skins to correct target-rules…

78e0126

…et pairs

frenzibyte added 2 commits June 25, 2024 05:54

Fix migration logic overwriting existing components in ruleset targets

fc2202e

Add test coverage

dc1fb4f

bdach reviewed Jun 27, 2024

View reviewed changes

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

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

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

peppy self-requested a review June 29, 2024 07:25

frenzibyte commented Jul 1, 2024

View reviewed changes

frenzibyte added 2 commits July 1, 2024 08:30

Store layout version in SkinLayoutVersion instead and refactor migr…

0c34e7b

…ation code

Merge branch 'master' into ruleset-specific-combo-counter

dce1b4e

frenzibyte force-pushed the ruleset-specific-combo-counter branch from 73f363c to dce1b4e Compare July 1, 2024 05:32

bdach reviewed Jul 3, 2024

View reviewed changes

bdach mentioned this pull request Aug 6, 2024

Add legacy key counter support #29027

Merged

Merge branch 'master' into ruleset-specific-combo-counter

3c572ab

peppy reviewed Aug 7, 2024

View reviewed changes

peppy added priority:1 Very important. Feels bad without fix. Affects the majority of users. and removed priority:1 Very important. Feels bad without fix. Affects the majority of users. labels Aug 7, 2024

peppy self-requested a review August 8, 2024 06:43

Avoid making non-ruleset transformers in Ruleset.CreateSkinTransformer

60d3834

This didn't make any sense, so let's do it a better way.

peppy self-assigned this Aug 8, 2024

Add back removed xmldoc

88c5997

peppy force-pushed the ruleset-specific-combo-counter branch from 02a370f to 88c5997 Compare August 8, 2024 07:37

peppy approved these changes Aug 8, 2024

View reviewed changes

peppy merged commit dcafee7 into ppy:master Aug 8, 2024
10 of 13 checks passed

frenzibyte deleted the ruleset-specific-combo-counter branch August 8, 2024 08:55

cdwcgt mentioned this pull request Aug 16, 2024

SkinnableSprite not working #29444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move combo counter to ruleset-specific HUD components container #26249

Move combo counter to ruleset-specific HUD components container #26249

frenzibyte commented Dec 30, 2023

bdach commented Jun 24, 2024

frenzibyte commented Jun 24, 2024

frenzibyte commented Jun 24, 2024

frenzibyte commented Jun 25, 2024 •

edited

Loading

frenzibyte Jul 1, 2024

bdach Jul 3, 2024

frenzibyte Jul 1, 2024

bdach Jul 3, 2024

bdach left a comment

bdach Jul 3, 2024

frenzibyte Jul 3, 2024

bdach Jul 3, 2024

peppy Aug 7, 2024

peppy Aug 8, 2024

peppy Aug 8, 2024

frenzibyte Aug 8, 2024

frenzibyte Aug 8, 2024

peppy Aug 8, 2024 •

edited

Loading

frenzibyte Aug 8, 2024

		[JsonProperty(DefaultValueHandling = DefaultValueHandling.Populate)]
		public int Version = LATEST_VERSION;

Move combo counter to ruleset-specific HUD components container #26249

Move combo counter to ruleset-specific HUD components container #26249

Conversation

frenzibyte commented Dec 30, 2023

bdach commented Jun 24, 2024

frenzibyte commented Jun 24, 2024

frenzibyte commented Jun 24, 2024

frenzibyte commented Jun 25, 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

bdach left a comment

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

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

Choose a reason for hiding this comment

peppy Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte commented Jun 25, 2024 •

edited

Loading

peppy Aug 8, 2024 •

edited

Loading