Introduce common structure for key counters #22654

ItsShamed · 2023-02-15T23:17:32Z

Supersedes #22640

I decided to split the abstraction and the new design implementation as suggested in #22640 (comment)

This PR abstracts the KeyCounter and KeyCounterDisplay in order to simplfy the implementation of more designs.
This also simplifies the creation of more input triggers for the KeyCounters to react to.

Some commits have extended messages, in case the diff is too messy to understand anything.

Most of these commits were just cherry-picked from #22640

To implement different different sources of input for KeyCounter, it is now possible to create a Trigger class (to inherit) instead of inheriting KeyCounter. This eases the creation of more input sources (like for tests) while allowing to implement different UI variants. That way, if another variant of the key counter needs to implemented (for whathever reason), this can be done by only inheriting KeyCounter and changing how things are arranged visually.

This allows for different layouts of display. Idk, maybe someone would want to mix both variants? (don't do this please). This commit is mostly prep for further changes.

This caused the Keyboard inputs to register twice, which is not what we want.

Makes it better to understand their purpose

peppy · 2023-02-16T06:52:33Z

I'm not too sure I understand why you've added non-action based key triggers. Before I begin to review, could you explain that?

ItsShamed · 2023-02-16T08:01:52Z

miss click was about to answer smh

ItsShamed · 2023-02-16T08:14:10Z

I'm not too sure I understand why you've added non-action based key triggers. Before I begin to review, could you explain that?

If you're talking about KeyCounterKeyboardTrigger and KeyCounterMouseTrigger they basically were KeyCounters themselves before the PR. So in this PR, instead of being KeyCounters, they are now Triggers. The only place they were used is in the tests:

master

osu/osu.Game.Tests/Visual/Gameplay/TestSceneKeyCounter.cs

Lines 28 to 31 in 87901bc

    
           testCounter = new KeyCounterKeyboard(Key.X), 
        
           new KeyCounterKeyboard(Key.X), 
        
           new KeyCounterMouse(MouseButton.Left), 
        
           new KeyCounterMouse(MouseButton.Right),

this PR

osu/osu.Game.Tests/Visual/Gameplay/TestSceneKeyCounter.cs

Lines 34 to 37 in 74e7cc2

    
           testCounter = new DefaultKeyCounter(new KeyCounterKeyboardTrigger(Key.X)), 
        
           new DefaultKeyCounter(new KeyCounterKeyboardTrigger(Key.X)), 
        
           new DefaultKeyCounter(new KeyCounterMouseTrigger(MouseButton.Left)), 
        
           new DefaultKeyCounter(new KeyCounterMouseTrigger(MouseButton.Right)),

Some other tests also had custom KeyCounters for custom actions, so they were also converted to Triggers.

I hope I was clear enough, even though I thought this would be obvious to think about at first

74a58fb message is also relevant to look at.

bdach

First pass. Mostly examination of structure.

osu.Game/Screens/Play/KeyCounter.cs

osu.Game/Screens/Play/KeyCounterDisplay.cs

bdach · 2023-02-16T21:41:31Z

osu.Game/Screens/Play/KeyCounterDisplay.cs

        }

+        protected virtual bool CheckType(KeyCounter key) => true;


This is very weird. What is this for? As best I can tell it's to avoid mixing up key counters of different drawable implementations?

Hesitantly I'd suggest to make the class a generic (as in KeyCounterDisplay<TKeyCounter> where TKeyCounter : KeyCounter) or something. I'm not sure it's ever going to be valid to mix these?

I don't know what to do for this one, as I feel like there could be a lot of directions, but I don't know which one is the best.

From what I can learn from other generics in the codebase, I could separate the class into generic and non-generic base classes.
However, forcing the new implementations to inherit the generic base only is pretty impossible for this class, as making the constructor internal on the non-generic has pretty much no effect since new implementations will essentially be created in the same assembly.

I could also just make the base class generic without splitting anything, but then it would mean that every occurrence of KeyCounterDisplay would have to be replaced with KeyCounterDisplay<KeyCounter>, and I don't know if it's okay to do that (especially for variable declarations).

Why does the check even need to exist?

`IsCounting` is back being an auto-property. `countPresses` is now encapsulated and being exposed as an `IBindable<int>` via `CountPresses`

As for the second suggestion in ppy#22654 (comment), I went with the first one as only one Trigger actually uses this argument for rewinding.

ItsShamed · 2023-02-17T10:39:11Z

All issues have been addressed (hopefully).

bdach · 2023-03-07T20:38:44Z

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

+        public abstract void AddTrigger(InputTrigger trigger);
+
+        public void AddTriggerRange(IEnumerable<InputTrigger> triggers) => triggers.ForEach(AddTrigger);
+
+        public override void Add(KeyCounter counter)
+        {
+            if (!checkType(counter))
+                throw new InvalidOperationException($"{counter.GetType()} is not a supported counter type. (hint: you may want to use {nameof(AddTrigger)} instead.)");
+
+            base.Add(counter);
+            counter.IsCounting.BindTo(IsCounting);
+        }


I started to apply some final cleanups to this pull but this is where I get stuck. Why is this here still? As in, why is any subclass still allowed to arbitrarily try and add any KeyCounter if the type isn't matching?

Like this is so weird for me, because KeyCounterDisplay is apparently a Container, but then DefaultKeyCounterDisplay is setting InternalChild anyway, and thus is just nullifying the fact that its base class is a container.

I would like to see the process of creating the individual key counters and placing them in the key counter display's layout entirely encapsulated within the key counter display, because it shouldn't ever make sense to mix and match key counter implementations. It should be fundamentally impossible to misuse the component. This would be done by making KeyCounter a plain CompositeDrawable, with the expectation that implementors of AddTrigger() should in response create the KeyCounter they want and place it in the layout of the key counter display according to expectation.

I did consider guarding against this further but wasn't sure it was worth the effort. Don't disagree with direction though.

That said, I did initially try to refactor to a CompositeDrawable but it gets messy since CompositeDrawable doesn't support Content overrides.

As can be seen by 44297a7.

So I'm not 100%, see what you think I guess.

bdach · 2023-03-08T20:50:46Z

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

+        /// <summary>
+        /// The <see cref="KeyCounter"/>s contained in this <see cref="KeyCounterDisplay"/>.
+        /// </summary>
+        public abstract Container<KeyCounter> Counters { get; }


This being a Container<KeyCounter> again means the last 3 commits actually changed nothing in the state of affairs, because if it's exposed as a container, then you can add anything you like again through it.

Can we revert 5d15426? I was honestly quite fine with the IEnumerable<KeyCounter>. I don't find it at all messy. The only consumers of this property are: test scenes, and RulesetInputManager. It's very contained.

The purpose of that commit was to fix this
https://github.com/ppy/osu/actions/runs/4360048022
Does that mean that I need to remove the checks for the counter flows visibility in TestSceneHUDOverlay? This is the only way I see to solve the failures (plus I think that this check will most likely be irrelevant as new variants get implemented)

How does changing that from IEnumerable to Container fix test failures? That's an alarm bell if you ask me. Just changing the returned type fix it? Can you explain why?

Because of this??

osu/osu.Game.Tests/Visual/Gameplay/TestSceneHUDOverlay.cs

Line 48 in 8f6df5e

private FillFlowContainer<KeyCounter> keyCounterFlow => hudOverlay.KeyCounter.ChildrenOfType<FillFlowContainer<KeyCounter>>().First();

Actually the more I look into it, the more the failure feels weird to me [the failures]

Well this is where I would just fix the test to not use .ChildrenOfType<>(). But maybe it's just me.

I'll get into this later, I'm really not focused right now.

I've pushed a potential fix. Tbh I'm a little bit lost rn so I don't know if this fix is heading towards a right direction, sorry.

As suggested by bdach as this would make the last two commits useless Refs: 5d15426

peppy · 2023-03-15T08:54:14Z

osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableHUDOverlay.cs

@@ -43,7 +42,7 @@ public partial class TestSceneSkinnableHUDOverlay : SkinnableTestScene

        // best way to check without exposing.
        private Drawable hideTarget => hudOverlay.KeyCounter;
-        private FillFlowContainer<KeyCounter> keyCounterFlow => hudOverlay.KeyCounter.ChildrenOfType<FillFlowContainer<KeyCounter>>().First();
+        private Drawable keyCounterFlow => (Drawable)hudOverlay.KeyCounter.Counters;


This is the dodgiest code I've seen in a long time. To the point my brain has trouble comprehending how it even compiles..

…h `Container`

peppy · 2023-03-15T09:03:40Z

I've pushed remaining menial fixes. Kinda done with this PR at this point so going to go ahead with the merge. Any further changes can be applied in a new PR.

ItsShamed added 9 commits February 13, 2023 01:24

refactor: separate impl of KeyCounterDisplay

11d0e18

This allows for different layouts of display. Idk, maybe someone would want to mix both variants? (don't do this please). This commit is mostly prep for further changes.

refactor: hide trigger presence from content

aa2e002

refactor: rename Trigger class to InputTrigger

157bba7

fix(KeyCounter): don't override Handle

df06338

This caused the Keyboard inputs to register twice, which is not what we want.

style(KeyCounter): rename (Un)lit methods to (Un)light

a644fae

refactor: rename trigger classes

076eb81

Makes it better to understand their purpose

style: nullable pass on KeyCounterDisplay

b0a2e69

reafactor: simplify type checking

e9dcc25

pull-request-size bot added the size/XL label Feb 15, 2023

This was referenced Feb 15, 2023

Implement new design of Key Counter #22655

Merged

Implement new design of the key counter #22640

Closed

peppy self-requested a review February 16, 2023 06:51

ItsShamed closed this Feb 16, 2023

ItsShamed reopened this Feb 16, 2023

peppy added the type:code-quality label Feb 16, 2023

bdach changed the title ~~Abstracting Key Counter~~ Introduce common structure for key counters Feb 16, 2023

bdach reviewed Feb 16, 2023

View reviewed changes

ItsShamed added 8 commits February 16, 2023 21:59

refactor(KeyCounter): remove circularity

6340730

refactor(KeyCounter): address bindables issues

ddd6c1a

`IsCounting` is back being an auto-property. `countPresses` is now encapsulated and being exposed as an `IBindable<int>` via `CountPresses`

style(KeyCounter): rename methods and arguments

c61fac5

As for the second suggestion in ppy#22654 (comment), I went with the first one as only one Trigger actually uses this argument for rewinding.

refactor: make FillFlowContainer read-only

e3ca751

fix(TestSceneOsuTouchInput): missing Value call

6193aee

style(KeyCounter): rename IsLit to IsActive

d0e8d65

style(KeyCounterDisplay): remove type check

c94e647

fix: compilation

8830e06

peppy previously approved these changes Mar 7, 2023

View reviewed changes

bdach added 3 commits March 7, 2023 21:16

Remove unused using directive

f711915

Improve documentation of InputTrigger

98f40b2

Reorder and add xmldoc to KeyCounter members

12af002

bdach dismissed peppy’s stale review via 12af002 March 7, 2023 20:34

bdach reviewed Mar 7, 2023

View reviewed changes

ItsShamed added 3 commits March 8, 2023 00:47

refactor: make KCD a CompositeDrawable

44297a7

docs: add XMLDoc for methods in KCD

5b0db94

refactor: make Counters return a Container

5d15426

bdach reviewed Mar 8, 2023

View reviewed changes

revert: make counters an IEnumerable again

d806b85

As suggested by bdach as this would make the last two commits useless Refs: 5d15426

peppy self-requested a review March 15, 2023 08:38

peppy reviewed Mar 15, 2023

View reviewed changes

peppy added 3 commits March 15, 2023 18:01

Fix super-dodgy cast of IEnumerable to Drawable

8908648

Use object initialisers and fix order of initialisation vs add

9e444af

Rename Add/AddRange methods as they are no longer conflicting wit…

5f9b13a

…h `Container`

peppy approved these changes Mar 15, 2023

View reviewed changes

peppy self-requested a review April 3, 2023 06:33

peppy added 2 commits April 3, 2023 15:33

Merge branch 'master' into gameplay/key-counter-abstraction

b0c09df

Fix missing using statements in multiple test scenes

6239789

peppy previously approved these changes Apr 3, 2023

View reviewed changes

peppy dismissed their stale review via 6239789 April 3, 2023 06:37

peppy previously approved these changes Apr 3, 2023

View reviewed changes

bdach added 2 commits April 5, 2023 19:45

Merge branch 'master' into gameplay/key-counter-abstraction

432f698

Fix incorrect indent

e5d57a6

bdach dismissed peppy’s stale review via e5d57a6 April 5, 2023 18:25

bdach approved these changes Apr 5, 2023

View reviewed changes

bdach enabled auto-merge April 5, 2023 18:25

bdach merged commit a4e68e1 into ppy:master Apr 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce common structure for key counters #22654

Introduce common structure for key counters #22654

ItsShamed commented Feb 15, 2023 •

edited

Loading

peppy commented Feb 16, 2023

ItsShamed commented Feb 16, 2023

ItsShamed commented Feb 16, 2023 •

edited

Loading

bdach left a comment

bdach Feb 16, 2023

ItsShamed Feb 17, 2023

peppy Feb 17, 2023

ItsShamed commented Feb 17, 2023

bdach Mar 7, 2023 •

edited

Loading

peppy Mar 8, 2023

peppy Mar 8, 2023

bdach Mar 8, 2023 •

edited

Loading

ItsShamed Mar 8, 2023

bdach Mar 8, 2023

ItsShamed Mar 8, 2023

ItsShamed Mar 8, 2023 •

edited

Loading

bdach Mar 8, 2023

ItsShamed Mar 8, 2023

ItsShamed Mar 9, 2023 •

edited

Loading

peppy Mar 15, 2023

peppy commented Mar 15, 2023

Introduce common structure for key counters #22654

Introduce common structure for key counters #22654

Conversation

ItsShamed commented Feb 15, 2023 • edited Loading

peppy commented Feb 16, 2023

ItsShamed commented Feb 16, 2023

ItsShamed commented Feb 16, 2023 • edited Loading

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

ItsShamed commented Feb 17, 2023

bdach Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Mar 8, 2023 • 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

ItsShamed Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItsShamed Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppy commented Mar 15, 2023

ItsShamed commented Feb 15, 2023 •

edited

Loading

ItsShamed commented Feb 16, 2023 •

edited

Loading

bdach Mar 7, 2023 •

edited

Loading

bdach Mar 8, 2023 •

edited

Loading

ItsShamed Mar 8, 2023 •

edited

Loading

ItsShamed Mar 9, 2023 •

edited

Loading