Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add legacy osu!mania combo counter #20579

Closed
wants to merge 11 commits into from

Conversation

frenzibyte
Copy link
Member

Transitions were based off stable code, I've also made an abstract version of LegacyComboCounter given the similarities that both the osu! and osu!mania variants share.

LegacyCatchComboCounter should also be updated to inherit from LegacyComboCounter but I've left that for a future effort seeing that I've spent too much time on this PR alone.

All that's left now is taiko, which should be easy to achieve with the new base LegacyComboCounter class.


CleanShot.2022-10-05.at.04.26.25.mp4

osu.Game/Skinning/LegacyOsuComboCounter.cs Outdated Show resolved Hide resolved
@@ -16,6 +16,8 @@ namespace osu.Game.Rulesets.Mania.Edit
{
public class DrawableManiaEditorRuleset : DrawableManiaRuleset
{
protected override bool DisplayComboCounter => false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just for the editor? If so can't it be done in ManiaEditorPlayfield?

Copy link
Member Author

@frenzibyte frenzibyte Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it to DrawableManiaRuleset since ManiaPlayfield on its own shouldn't display a combo counter by default (see TestSceneBarLine for example). I agree it's awkward though, I intend it to be a temporary property that would be removed once something like LegacyRulesetSpecificCombo exists.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't quite understand. The only time this is set to false is in DrawableManiaEditorRuleset, which also overrides CreatePlayfield and creates the playfield without the combo counter. Am I missing something?

@peppy
Copy link
Member

peppy commented Oct 5, 2022

I'm curious on the forward direction you see here.

I can imagine potentially:

  • One HUD component which is "LegacyRulesetSpecificCombo" which internally chooses the correct combo counter for the ruleset at hand, keeping all logic local rather than split across many places. This would be automatically added for legacy skins
    • Individual ruleset versions could be exposed as HUD components if the user prefers one specific one (ie. showing the osu! legacy combo counter in mania mode, which is currently impossible).

Not sure if that would come with any detractors, but worth exploring maybe? Could tidy things up.

I'm not completely against how it's done in this PR because it's following by example... but the example in this case is also quite hacky/temporary.

@frenzibyte
Copy link
Member Author

frenzibyte commented Oct 5, 2022

That's similar to what I had in mind, but I kept things simple since this would require moving the combo counter for catch as well, which is already deep on some completely different type of logic. I'll attempt it and see if it helps.

@peppy
Copy link
Member

peppy commented Oct 7, 2022

@frenzibyte if you haven't got far in the refactor work, I'm happy to review this with the intent of getting it merged as-is, and leaving the rest as follow-up work.

@frenzibyte
Copy link
Member Author

After spending few hours exploring through this, I think it's best leaving it as-is until the skinning flow supports ruleset-specific configuration (which is not really simple to do either).

To start with, the idea of having a LegacyRulesetSpecificCombo component that chooses the correct counter based on the selected ruleset doesn't really work well mainly because the main component would have to somehow delegate its skinning properties to the underlying ruleset-specific counter in order to have skinning support, since losing that technically means a regression for osu! ruleset skinning (no longer be able to adjust legacy osu! combo counter).

I've came up with another idea that involves a main component which would add all ruleset-specific combo counters in order to be serialised, but each counter would kill itself if it's loaded in the wrong ruleset. While this works, I don't think the code is within standards at all (see diff).

Therefore let's continue with this temporary direction until we have ruleset-specific skinning support, as mentioned above.

@peppy
Copy link
Member

peppy commented Oct 12, 2022

Tests still need attention here.

@peppy peppy self-requested a review October 13, 2022 03:16
@peppy peppy marked this pull request as draft June 16, 2023 06:09
@frenzibyte
Copy link
Member Author

I'm going to close this and split it to multiple PRs since there are other changes required to have this implemented correctly.

@frenzibyte frenzibyte closed this Dec 30, 2023
@frenzibyte frenzibyte deleted the legacy-mania-combo-counter branch December 30, 2023 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mania LegacyComboCounter get a shadow backdrop when counter increases
2 participants