Add legacy key counter support #29027

normalid-awa · 2024-07-23T16:27:01Z

Add the legacy key counter component, just like how the skinnable key counter in stable.

pr.demo.mp4

* Make the font match stable style

For use with ppy/osu#29027. Sources: - https://github.com/peppy/osu-stable-reference/blob/master/osu!ui/Resources/inputoverlay-key%402x.png - https://github.com/peppy/osu-stable-reference/blob/master/osu!ui/Resources/inputoverlay-background%402x.png

bdach · 2024-07-24T10:45:32Z

Gotta say, this is half-baked at best.

Default skin doesn't work correctly out-of-the-box with Add legacy skin input overlay assets osu-resources#332 assets
Not sure what this key counter is even doing on mania, where there is not enough space to physically fit the boxes

As I know people want this, I guess I'll use this as an opportunity to port this properly on top of what's here, but I really expect a better quality of PRs going forward.

normalid-awa · 2024-07-24T10:57:14Z

Gotta say, this is half-baked at best.

Default skin doesn't work correctly out-of-the-box with Add legacy skin input overlay assets osu-resources#332 assets

Not sure what this key counter is even doing on mania, where there is not enough space to physically fit the boxes

As I know people want this, I guess I'll use this as an opportunity to port this properly on top of what's here, but I really expect a better quality of PRs going forward.

https://osu.ppy.sh/wiki/en/Skinning/Interface#input-overlay
Is said that the input overlay is only available in std and catch, so it is good to just not display them in the other mode.

And i think I've missed the colour when key down, and the blending mode is not match the stable.

I dunno what the wiki is claiming with the "24px" figure or why but I'm not playing conversion games either. Dimensions ballparked via screenshots captured at x768 resolution. Also removes a weird homebrew method to keep the text upright. There is one canonical way to do this, namely `UprightAspectMaintainingContainer`. And the other key counters were already using it.

bdach · 2024-07-24T13:19:22Z

Sigh...

I am working on this branch right now. Please do not push further until instructed. I'll probably force-push most of your changes out anyway at this point because this code needs dire improvement anyhow.

osu.Game/Skinning/LegacyKeyCounter.cs

normalid-awa · 2024-07-24T14:16:46Z

miss clicked the button,sorry

osu.Game/Skinning/LegacySkin.cs

After the legacy key counter was moved to ruleset-specific component containers, `TestSceneSkinnableHUDOverlay` no longer had a key counter, because it wasn't creating a ruleset-specific HUD component container due to https://github.com/ppy/osu/blob/4983e5f33ed11ba3777e53face6271066ba01ab9/osu.Game/Screens/Play/HUDOverlay.cs#L131-L133 Therefore, to fix, do just enough persuading to make it create one.

Doxxz · 2024-07-24T17:09:24Z

Since it's possible to have 4 keybinds in osu, could this overlay show these four keys, similar to how it was in stable?

normalid-awa · 2024-07-24T17:32:44Z

Since it's possible to have 4 keybinds in osu, could this overlay show these four keys, similar to how it was in stable?

Imo, Both bind are considered as one input (ie "z","m1" are both registered as left button"). implementing this will require a lot more changes to the base code, and would affect existing key counter.

normalid-awa · 2024-08-06T03:45:22Z

I am done with this branch.

@normalid-awa please read through my changes and take note for the future. If anything is uncertain please ask.

@ppy/team-client requesting a review since I basically rewrote most of this. Stable code for your perusal at https://github.com/peppy/osu-stable-reference/blob/bb57924c1552adbed11ee3d96cdcde47cf96f2b6/osu!/Input/Drawable/InputOverlay.cs.

I only matched the things that were obvious/simple to match (colours, transitions), and left well alone things that were not (I don't even know where to even begin with the stable method of specifying sizings; also stable would always add four switches and just fade out the ones that aren't used to 50% opacity; I decided I'd rather not bother with this as it's not super trivial to support this, but can if there is insistence on it).

Oh, and c3dae81 probably deserves particular scrutiny because it is unprecedented.

@bdach May I ask about the progress of the review?

bdach · 2024-08-06T04:03:05Z

I'm not the one to ask. I asked others for review. I'm not reviewing this myself because I changed too much.

peppy · 2024-08-06T05:40:22Z

osu.Game/Skinning/LegacySkin.cs

@@ -437,6 +388,84 @@ protected override void ParseConfigurationStream(Stream stream)
            return null;
        }

+        private static DefaultSkinComponentsContainer? createDefaultHUDComponents(SkinComponentsContainerLookup containerLookup)
+        {
+            switch (containerLookup.Ruleset?.ShortName)


So.. this hinges on the fact that there's guaranteed to be a SkinComponentsContainerLookup request for null (global) then per ruleset?

I'm sure you did already consider this, but would it be better or worse to put the ruleset version in OsuLegacySkinTransformer etc.?

Seems to be discussed before. ref
But I'm still have the preferred as put the legacy key per-ruleset.

So.. this hinges on the fact that there's guaranteed to be a SkinComponentsContainerLookup request for null (global) then per ruleset?

I'm not sure what you mean by "then" here. Both happen. Always. The legacy counter has been moved out of the legacy skin's all-rulesets HUD layer, into its per-ruleset layers where applicable.

As stated in the review conversation linked above, mania and taiko will have to receive their own special stuff anyway in the future, e.g. for combo counter. And I would intend that to be done in this exact same manner.

I'm not sure how a OsuLegacySkinTransformer-based solution would be - would you be checking if the transformed skin is legacy or something? I think that is worse than this no?

I'll make a diff proposal.

~~diff is gone~~

This seems to work well, and follow more closely to the expectations I'd have from the skinning structure (we already have these transformer classes for rulesets modification like this).

I had to update the diff, accidentally shared a partial diff, sorry.

I just committed it, the diff is huge and whatever.

725dc4d Highly recommend hiding whitespace changes.

I just committed it, the diff is huge and whatever.

725dc4d Highly recommend hiding whitespace changes.

Looks good, Yours approach looks similar to the original thought when I was trying to bring the legacy key counter to the default. But my approach is to instantiate the key counter globally, and disable them in taiko/mania, or whatever the ruleset doesn't compact with the legacy key counter. But seems like yours approach looks more clean and readable!

I don't buy that commit.

Why are legacy skin defaults being handled by skin-agnostic ruleset transformers?

I'm not even gonna comment on the code itself because it's basically illegible to me and breaks my entire mental model of how any of this is supposed to work. Ruleset transformers doing something for null ruleset, then some fallback to base, and then some manual operating on a skin that may or may not contain a legacy counter. How is that better than what I had?

I might just give up touching skinning altogether because everyone seems to be going in a common direction that makes no sense to me. I have no idea what anything is supposed to be. Transformers on transformers, stuff getting handled out-of-band in places three levels away. I don't know what a "ruleset-specific" transformer is supposed to do, nor what a "skin-specific" transformer is supposed to do. They basically seem to arbitrarily perform each other's purposes for what feels like no reason. (Previously.)

osu.Game/Skinning/LegacyKeyCounterDisplay.cs

frenzibyte · 2024-08-06T05:56:07Z

I'm also noticing the key overlay to display incorrectly when "beatmap skins" is enabled:

CleanShot.2024-08-06.at.08.54.30.mp4

Skin: - CK WhiteCat 2.0 _ new (cyperdark).osk.zip

Happens on any beatmap as far as I'm aware.

peppy · 2024-08-06T05:59:51Z

I'm also noticing the key overlay to display incorrectly when "beatmap skins" is enabled

Looks like this config lookup needs to use getCustomColour instead of legacySettingLookup. I'll take a quick look at fixing this too.

Fixed in 90395ae.

Adds a note about `GetConfig` being stupid.

… hidden

peppy · 2024-08-06T08:27:49Z

@normalid-awa please refrain from committing.

peppy

Seems probably okay now

osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs

normalid-awa added 10 commits July 23, 2024 19:05

Implement the overlay

ad1a86e

Update the offset formula

777a0de

Make the text are always horizontal

5dcc8b7

Clean up the code

f7dc0b6

Remove unused blending mode

dce8941

Change the default height to match the stable

a015fde

Replace SkinnableSprite with Sprite

9fe369b

Fix the return button being squshed

989ac56

* Fix the default position

c7b110a

* Make the font match stable style

Support custom input overlay color

c52a993

pull-request-size bot added the size/L label Jul 23, 2024

normalid-awa added 8 commits July 24, 2024 12:18

Add test coverage

661f58a

Add visual test seane

95f2871

Match the stable animation

395f842

Remove logging

e2beacb

Update offset factor

56143de

Fix code quality

b24be96

Fix indent problems

fede6b3

Update test assets

0306ef4

bdach added the area:skinning label Jul 24, 2024

bdach self-requested a review July 24, 2024 10:30

bdach mentioned this pull request Jul 24, 2024

Add legacy skin input overlay assets ppy/osu-resources#332

Merged

bdach added 3 commits July 24, 2024 14:25

Add better test scene

ace5071

Simplify input overlay text colour decode (and fix incorrect default)

3c28c11

bdach reviewed Jul 24, 2024

View reviewed changes

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

normalid-awa closed this Jul 24, 2024

normalid-awa reopened this Jul 24, 2024

normalid-awa commented Jul 24, 2024

View reviewed changes

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

peppy self-requested a review August 6, 2024 05:05

Merge branch 'master' into feature/skin/legacy-input-overlay

60fc02b

peppy reviewed Aug 6, 2024

View reviewed changes

peppy added 2 commits August 6, 2024 15:17

Sanitise font sizes / weights

c4572ec

Fix incorrect colour fallback handling

90395ae

Adds a note about `GetConfig` being stupid.

peppy force-pushed the feature/skin/legacy-input-overlay branch from 5f69b74 to 1f6704b Compare August 6, 2024 07:09

Fix legacy key counter's background being visible when intended to be…

8619bbb

… hidden

peppy force-pushed the feature/skin/legacy-input-overlay branch from 1f6704b to 8619bbb Compare August 6, 2024 07:12

peppy force-pushed the feature/skin/legacy-input-overlay branch from 06aee1b to 8619bbb Compare August 6, 2024 08:29

Use transformers for per-skin key counter implementation

725dc4d

pull-request-size bot added size/XL and removed size/L labels Aug 6, 2024

peppy approved these changes Aug 6, 2024

View reviewed changes

peppy requested a review from bdach August 6, 2024 13:10

smoogipoo reviewed Aug 7, 2024

View reviewed changes

osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs Show resolved Hide resolved

peppy merged commit b1488fd into ppy:master Aug 7, 2024
9 of 13 checks passed

smoogipoo mentioned this pull request Aug 14, 2024

Legacy key counter is positioned incorrectly #29420

Closed

Joehuu mentioned this pull request Aug 17, 2024

Add support for legacy skinning of the key overlay #10944

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add legacy key counter support #29027

Add legacy key counter support #29027

normalid-awa commented Jul 23, 2024 •

edited

Loading

bdach commented Jul 24, 2024

normalid-awa commented Jul 24, 2024

bdach commented Jul 24, 2024 •

edited

Loading

normalid-awa commented Jul 24, 2024

Doxxz commented Jul 24, 2024

normalid-awa commented Jul 24, 2024

normalid-awa commented Aug 6, 2024

bdach commented Aug 6, 2024

peppy Aug 6, 2024

normalid-awa Aug 6, 2024

bdach Aug 6, 2024 •

edited

Loading

peppy Aug 6, 2024

peppy Aug 6, 2024 •

edited

Loading

peppy Aug 6, 2024

peppy Aug 6, 2024 •

edited

Loading

normalid-awa Aug 6, 2024

bdach Aug 6, 2024 •

edited

Loading

frenzibyte commented Aug 6, 2024

peppy commented Aug 6, 2024 •

edited

Loading

peppy commented Aug 6, 2024

peppy left a comment

Add legacy key counter support #29027

Add legacy key counter support #29027

Conversation

normalid-awa commented Jul 23, 2024 • edited Loading

bdach commented Jul 24, 2024

normalid-awa commented Jul 24, 2024

bdach commented Jul 24, 2024 • edited Loading

normalid-awa commented Jul 24, 2024

Doxxz commented Jul 24, 2024

normalid-awa commented Jul 24, 2024

normalid-awa commented Aug 6, 2024

bdach commented Aug 6, 2024

peppy Aug 6, 2024

Choose a reason for hiding this comment

normalid-awa Aug 6, 2024

Choose a reason for hiding this comment

bdach Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

peppy Aug 6, 2024

Choose a reason for hiding this comment

peppy Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

peppy Aug 6, 2024

Choose a reason for hiding this comment

peppy Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

normalid-awa Aug 6, 2024

Choose a reason for hiding this comment

bdach Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

frenzibyte commented Aug 6, 2024

peppy commented Aug 6, 2024 • edited Loading

peppy commented Aug 6, 2024

peppy left a comment

Choose a reason for hiding this comment

normalid-awa commented Jul 23, 2024 •

edited

Loading

bdach commented Jul 24, 2024 •

edited

Loading

bdach Aug 6, 2024 •

edited

Loading

peppy Aug 6, 2024 •

edited

Loading

peppy Aug 6, 2024 •

edited

Loading

bdach Aug 6, 2024 •

edited

Loading

peppy commented Aug 6, 2024 •

edited

Loading