Implement new design of the key counter #22640
Conversation
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
This reverts commit 7bce3ed. Things regressed on reference leaks when trying to integrate the new counter. I think it's better to just leave the implementation as is fwiw and integrate it in a follow up PR.
I added comments instead to clarify that the values come from the Figma design.
|
I really dont think this should've been done before the conversion of the keycounter into a skinnable drawable. I had a prototype working version of this month's back ( nowhere near as clean but that's beside the point ) which I remember asking about in the discord and being told that it would likely be better to do that first. My concern is that that already difficult refactor that multiple people have failed at doing will only be made harder by the addition of this component being done before it. https://discord.com/channels/188630481301012481/188630652340404224/1019248279512432650 relevant discussion |
|
Oh, my very bad. I wasn't aware of there was already a discussion on this. I don't think it will be hard to convert this to a Moreover, since this is just the implementation without any intention of integrating it yet, I don't think this is much of a concern right now. |
i hope it ends up being easier, just voicing my concerns, and its understandable that you werent aware, it was only mentioned in passing on a long buried discord convo, but yeah thats all i wanted to add. |
|
I think for the time being, it's okay to keep that hidden until we find a way to make it customizable. |
|
Converting to a skinnable component should definitely be one of the next steps, as above. |
| // Make things look bigger without using Scale | ||
| private const float scale_factor = 1.5f; |
There was a problem hiding this comment.
Can you explain this a bit? Is it to match figma metrics to game?
There was a problem hiding this comment.
Basically the Figma values were too small visually, and since I want to keep the Figma values for clarity, I thought that it's best to just scale out the values to make them bigger.
|
I feel like you could probably split this PR up into two pieces: the part which is abstracting out a portion of the original implementation, then a second PR which implements the new design. It's going to be a bit hard to review both pieces at once as a lot of movement is going on here. |
This is just the component in itself for now. I tried to integrate it in the player, but I ended up regressing other things instead. For now, I think it's better to keep things somewhat simple and leave the integration for another PR.
osu.Game.Tests_Tpe6qj95jz.mp4