Fix skin editor legacy components not displaying correctly when used with non-legacy skins

[peppy]

This is what I'd call a stepping-stone change in the direction of simplifying skinning lookups. I almost aborted the issue with a "requires larger refactor" call, but this change looks to improve from status-quo and I believe it will aid in making future refactors simpler.

The actual fix here is that a LegacyDefaultSkin is now always provided by SkinManager regardless of the user's skin choice. Doing this previously would have caused gameplay elements to leak from the legacy skin into argon skins (consider the case of SliderTailHitCircle).

This is because, previously, we were wrapping all fallback providers inside ruleset transformers, but I argue that this is not correct (or at least, not necessary). The rationale is that ruleset transformers are there to transform known concepts in gameplay. It is assumed that every skin implementation has a solid idea of what it wants, and wouldn't expect any fallback lookups to happen. Any exceptions to this rule can be handled locally (as the sole example see BeatmapSkinProvidingContainer).

So now, only the beatmap skin (if a beatmap provides a skin) and the user skin are wrapped.

As a result, we simplify the lookup process. RulesetSkinProvidingContainer no longer creates its own local lookup and blocks hierarchical lookup.

Closes https://github.com/ppy/osu/issues/29317.

[bdach]

Spot what's wrong with the following picture:

Answer: there's a sneaky 300 legacy judgement on triangles skin. Also there's a legacy combo counter bottom left.

This is what terrifies me every time with one of these skinning PRs, the thing is inscrutable and has inscrutable lookup paths (I read the diff and couldn't tell that this was going to happen - I just stumbled upon this randomly), and we seemingly cannot exercise this with tests because they're very happily green here.

[peppy]

This is what terrifies me every time with one of these skinning PRs, the thing is inscrutable and has inscrutable lookup paths (I read the diff and couldn't tell that this was going to happen - I just stumbled upon this randomly), and we seemingly cannot exercise this with tests because they're very happily green here.

Yep, I was expecting random things like this. And I plan to add test coverage for each and every edge case until we have a test suite that can actually give us some degree of confidence.

[frenzibyte]

I have investigated the issues highlighted in https://github.com/ppy/osu/pull/30068#issuecomment-2385595439:

• The issue of legacy judgements displaying in triangles skin is due to following a different path from how argon judgements are chosen. Specifically, triangles judgements are constructed when absolutely no skin source provides drawables for them. This worked on master because when triangles skin is selected, no other skin source is inserted in the hierarchy. But now that a legacy skin source is provided, the path needs to be changed and match argon judgement lookups.

  Solving this is quite simple (dead giveaway even), I've gone through it in this commit: https://github.com/frenzibyte/osu/commit/d2be5553dd041263f5efdcb9f6e60f77796965a9. Tested to work with all skins in all rulesets.

• The issue of legacy combo counter displaying in triangles skin is actually fixed when merging master to this PR. It was a series of complications that goes in the following order:

  • HUD attempts to look up components specific to the ruleset.
  • TrianglesSkin, back when this PR was first opened, will return null to that particular lookup.
  • Lookup process falls back to the legacy skin source included in this PR, which will return its own ruleset-specific HUD components, which only consists of the legacy combo counter.

  If I had to solve this at the time this PR was opened, I would've made TrianglesSkin return an empty container when ruleset-specific lookups are requested. Because TrianglesSkin does not have anything to provide, and it shouldn't let other skins provide for it.

  Now however, TrianglesSkin returns a container for ruleset-specific HUD, containing the gameplay leaderboard. Therefore this is not an issue anymore.

---

On the other hand, there is another issue in another ruleset. Pippidon displaying in argon/triangles skin. Explaining skinning lookup issues fully in text is a pita so I'm gonna preface it with this video showing why pippidon appears in argon/triangles skin:

https://github.com/user-attachments/assets/38951fb1-ab07-43c2-907c-8a7dd040d99c

Put simply:

• Beatmap skin is transformed with the taiko legacy ruleset transformer.
• The legacy transformer always inserts DrawableTaikoMascot (the pippidon) to the gameplay screen.
• DrawableTaikoMascot (through few layers of code) looks up mascot textures from all skin sources.

Before PR, DrawableTaikoMascot is still always inserted but no textures are retrieved due to lack of a legacy skin source (when argon/triangles skin is selected).

Simplest and sanest solution I would propose is to put hasBarLeft as a condition before returning DrawableTaikoMascot, this is the same condition used to insert the legacy input drum. It should work well enough.

---

Other than all the above, and maybe a few highlights on the code I'll get to once the above is solved, I think this is good to go.

I wrote a lot of text above, so please ask if there's anything not clear or there's nothing clear above.

[peppy]

The issue of legacy combo counter displaying in triangles skin is actually fixed when merging master to this PR

Can you link to the PR which changed/fixed this?

[frenzibyte]

It's not a direct fix to the issue so I didn't link it, but: https://github.com/ppy/osu/pull/32939 / https://github.com/ppy/osu/pull/32939/commits/59b826c321f4f33ced130d40073d53ecc9b82154#diff-faad93ec77e18277128ccf18869d112d64eedcfc23798b2766e6c589fcd4e236. See this removal.

[peppy]

Solving this is quite simple (dead giveaway even), I've gone through it in this commit: https://github.com/frenzibyte/osu/commit/d2be5553dd041263f5efdcb9f6e60f77796965a9. Tested to work with all skins in all rulesets.

I don't find this commit simple. Why was DrawableManiaJudgement removed while the other rulesets remain?

[frenzibyte]

I've just removed the DrawableManiaJudgement and DrawableTaikoJudgement subclasses because they're no longer needed anymore, they existed because of the CreateDefaultJudgement override that returns ruleset-specific triangles skin implementations, but now it's all moved to the corresponding skin transformers.

DrawableOsuJudgement remains because it still adds special logic that applies to all skins (hit lighting).

[peppy]

Unassigning @frenzibyte since he seems to have dropped this.

For the next person to look at this, it may make sense to pull in a commit in the comment above which may improve things.