Add argon/classic osu!mania combo counter #26254
Conversation
|
I think this can be used for taiko as well? |
|
Yes but it's not in scope of this PR. |
| /// Whether this component supports the "closest" anchor. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This is disabled by some components that shift position automatically. |
There was a problem hiding this comment.
What's an example of this? I don't quite understand.
There was a problem hiding this comment.
Disabled by LegacyManiaComboCounter because its position is shifted when the user flips the scrolling direction. I don't recall why though.
There was a problem hiding this comment.
This seems like an odd choice. So the user can still choose an anchor and it will just flip? I'd say that closest should still be allowed and the flip is only applied if the counter is in a certain region of the screen (or have a certain anchor applied).
There was a problem hiding this comment.
I've removed this for now as I can't really understand why it is required. It would make more sense to completely disable anchors (or a certain subset) but honestly I'd just leave this up to the user, and adjust the automatic flipping logic to only work in the cases where it makes sense.
| /// <summary> | ||
| /// Uses the 'x' symbol and has a pop-out effect while rolling over. | ||
| /// </summary> | ||
| public partial class LegacyDefaultComboCounter : LegacyComboCounter |
There was a problem hiding this comment.
I'm honestly not sure this abstraction was required. Could have just copy-pasted and spent less effort? And potentially had more maintainable / legible code.
There was a problem hiding this comment.
No need to change it at this point, I'll do so if the review is painful enough.
peppy
force-pushed
the
mania-combo-counter
branch
2 times, most recently
from
08df05c
to
b95e05f
Compare
This may have had a good reason to be added, but I can't find that reason, so let's keep things simple for the time being.
peppy
force-pushed
the
mania-combo-counter
branch
from
b95e05f
to
7666e8b
Compare
| // two schedules are required so that updateAnchor is executed in the next frame, | ||
| // which is when the combo counter receives its Y position by the default layout in ArgonManiaSkinTransformer. |
There was a problem hiding this comment.
really dunno but willing to overlook for now because it works.
|
@frenzibyte I've made sizable changes to this PR. Want to give it a once-over? |
osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs
Outdated
Show resolved
Hide resolved
can you look into this? it shows fine here. |
|
|
| Anchor &= ~(Anchor.y0 | Anchor.y2); | ||
| Anchor |= direction.Value == ScrollingDirection.Up ? Anchor.y2 : Anchor.y0; |
There was a problem hiding this comment.
The legacy part handles the case where the anchor is set to center, but this one doesn't?
There was a problem hiding this comment.
I've updated this to match but there still seems to be something weird going on here.
- Set scroll direction to "Down"
- Put the combo counter near the bottom of the screen
- Change scroll direction
The combo won't move the first time changing direction.
There was a problem hiding this comment.
Alleviated the issue in 66adddb. This is the reason why "Closest" anchor was disabled in this PR (in response to #26254 (comment)).
To explain briefly, I am using the sign of the Y coordinate as an indicator for whether the combo counter is in the correct position or not. When the scroll direction is "Down", the anchor is set to top and the Y position is made sure to be positive. When the scroll direction is "Up", the anchor is set to bottom and the Y position is made sure to be negative.
When the "Closest" anchor is selected, if the user moves their combo counter from top to bottom or vice versa, the anchor will change, and defies everything that the logic above does, therefore breaking apart.
osu.Game.Rulesets.Mania/Skinning/Argon/ManiaArgonSkinTransformer.cs
Outdated
Show resolved
Hide resolved
|
As you touched on in discord, it turns out that the cause of this issue is that I had "beatmap skins" enabled, which makes the beatmap skin layer return a The reason why the combo position is zero is because of the lookup only happening on the beatmap skin layer, and the beatmap skin layer disables mania lookups entirely (see I'm not sure what is the best way to solve this, one way could be to use the |
We need a general method to do this amicably, such as an HUD target that flips the position of its children when the direction is flipped. Something to consider later.
frenzibyte
dismissed
peppy’s stale review
The merge-base changed after approval.
Adds support for classic osu!mania combo counter, as well as a very basic implementation for "Argon" skin to increase its usability (cc @peppy for adding animations to the argon one if wanted).
This does not include turning the combo colour to yellow during a hold note, especially since hold notes don't have ticks anymore, which makes the yellow state look odd.
Preview:
CleanShot.2023-12-30.at.07.25.25-converted.mp4
Argon preview (again, not final):
CleanShot.2023-12-30.at.07.34.03-converted.mp4