Implement osu!mania-specific health bar display #30867

frenzibyte · 2024-11-25T05:52:26Z

Resolves osu!mania legacy skins don't place elements in their correct (mode) default position #22362

Added a migration step to move LegacyHealthDisplay from global target to per-ruleset target, as well as adding the mania-specific LegacyHealthDisplay specification for skins with existing user configuration. This required a minor refactor over the skin migration logic in general, to allow mutating multiple skin layouts in one step (especially when data from the first layout is required to act on the second layout).

frenzibyte · 2024-11-25T05:56:31Z

osu.Game/Skinning/Skin.cs

+                                // should avoid adding legacy health bar to non-legacy skins (unless explicitly added by user).
+                                if (!legacyHealthBars.Any())
+                                    break;


This implies that if the skin being migrated here has configured the Playfield target but never configured the non-ruleset HUD target, then the health bar will not be added.

It's a hard case to handle here because we don't really have a nice way of telling whether a skin is based on LegacySkin, nor do I want to use that as a condition anywhere anyway. I think it's fine to let those kind of skins manually add their mania health bar by resetting the target or otherwise.

I've read this comment five times and I still can't understand what you're trying to say. Can you elaborate? Doesn't the HUD target read fall back to an empty array if it fails? Can't you use that?

Doesn't the HUD target read fall back to an empty array if it fails?

What does "fails" mean here.

Let me attempt to explain how the migration logic works in a case-by-case scenario:

A skin with no configuration

HUD and Playfield are all null, so the lookups will get the default components from LegacySkin and ManiaLegacySkinTransformer if it's a legacy skin, we're gucci.

A skin with HUD configuration but no Playfield configuration

HUD is not null and Playfield is null

If the HUD configuration has a legacy health bar, remove it and store it to legacyHealthBars.

Don't do anything for Playfield, it's already null, the health bar will be looked up from ManiaLegacySkinTransformer if it's a legacy skin.

A skin with HUD configuration and Playfield configuration

HUD and Playfield are both not null

If the HUD configuration has a legacy health bar, remove it and store it to legacyHealthBars.

If legacyHealthBars is not empty, add a legacy mania health bar to Playfield.

The condition used here is good enough to ensure if the configured skin is not legacy then no legacy health bar will be added to mania, by checking whether the skin used to have a legacy health bar or not.

A skin with no HUD configuration but a Playfield configuration

HUD is null, Playfield is not null

Don't do anything for HUD, it's already null

legacyHealthBars is empty because HUD is null

Don't do anything for Playfield since legacyHealthBars is empty

This is the part touched on in this conversation. This is incorrect because if the skin is legacy then there should be a health bar added to Playfield by this migration logic, but I'm intentionally ignoring it for simplicity. It should be a rare case and one that can be fixed by the user.

Might as well write test cases rather than state them word by word.

Test cases written within an introduced testing system TestSceneSkinMigration.

bdach · 2024-11-25T11:32:47Z

osu.Game/Skinning/Skin.cs

+                int version = layoutInfos.Values.Min(l => l.Version);
+
+                for (int i = version + 1; i <= SkinLayoutInfo.LATEST_VERSION; i++)
+                    applyMigration(i);


There should be no taking of .Min() here, and applyMigration() should be called in the inner loop over layoutInfos.Values.

Why? Presume you have two LayoutInfos with different versions each. LayoutInfo X is on version 1, and LayoutInfo Y on version 2. Now when the latest version is bumped to 3, Y will have the migration from version 1 to 2 called twice. Which is only fine if the migration is idempotent, as in it can determine whether it has already run, and bail if it has.

This is a very not obvious property of a migration system; with these you generally assume that a migration is a one-and-done affair. Thus I object to this type of handling, as an unnecessary complication. If targets are allowed to deviate in version, then migrations should be applied on a per-target basis.

and applyMigration() should be called in the inner loop over layoutInfos.Values.

Read OP.

The intention is that we should never have a case where there are two layouts with different migration versions, unless the user decided to mess around with the files. And, hopefully our migration system is good enough to not affect layouts which are already migrated even if the migration was ran again, but that's just a loose
assumption.

That being said, with the concern raised both here and essentially #30867 (comment), I will revert this and compare against the instantiation type instead. I realised at the end of the day if a configuration is null then the default components technically rely on the "instantiation type" (what type of skin class the skin is), so it's more logical to read the instantiation type and go from there instead.

Migration logic has been reverted.

The intention is that we should never have a case where there are two layouts with different migration versions, unless the user decided to mess around with the files.

You can't assume that people don't mess with the files, because they already do, and will be more likely to do it more after external skin mounting is added.

bdach · 2024-11-25T11:35:09Z

osu.Game/Skinning/Skin.cs

+                    if (resources == null)
+                        break;


What is this check... checking for? It's completely inscrutable to me.

Mostly ignore doing anything if we don't have the thing to do it, it only matters in tests where skins are instantiated without resources. This check existed before in previous migrations so I'm not sure why it's being asked in here.

Replaced with a debug assertion, normal tests should never run migrations anyway.

bdach · 2024-11-25T11:38:51Z

osu.Game/Skinning/Skin.cs

+                    if (hudLayout == null || !hudLayout.TryGetDrawableInfo(null, out var globalHUDComponents))
+                        globalHUDComponents = Array.Empty<SerialisedDrawableInfo>();
+
+                    var legacyHealthBars = globalHUDComponents.Where(h => h.Type.Name == nameof(LegacyHealthDisplay)).ToArray();


Why is this checking the name of the type if you already have the type itself in hand?

Suggested change

var legacyHealthBars = globalHUDComponents.Where(h => h.Type.Name == nameof(LegacyHealthDisplay)).ToArray();

var legacyHealthBars = globalHUDComponents.Where(h => h.Type == typeof(LegacyHealthDisplay)).ToArray();

Sure, I don't know how Type equality works, and I hope that if it does other stupid things like assembly version or whatever then it won't break this logic.

I will assume it only checks the name. I tried stepping through the equality implementation and ended up with both sides being equal by reference, I don't know.

If you need clarification here, given that CreateInstance works with however we're serialising types, this case also has to work. We do seem to be serialising versioning out, but I guess it works fine.

bdach · 2024-11-25T11:43:46Z

osu.Game/Skinning/Skin.cs

+                                // should avoid adding legacy health bar to non-legacy skins (unless explicitly added by user).
+                                if (!legacyHealthBars.Any())
+                                    break;


I've read this comment five times and I still can't understand what you're trying to say. Can you elaborate? Doesn't the HUD target read fall back to an empty array if it fails? Can't you use that?

This reverts commit 76f79ce.

… once" This reverts commit f1b5686.

This is different from `SkinDeserialisationTest` in that layouts can be written programmatically with as much ease, allowing to test migration logic with different scenarios without running the game and exporting skins and attaching them to tests.

frenzibyte · 2024-11-26T05:48:40Z

I'll update the deserialisation test once the PR is considered ready to merge.

peppy · 2024-11-27T07:21:47Z

osu.Game.Tests/Skins/TestSceneSkinMigration.cs

+                });
+            });
+
+            // One may argue that if a LegacyHealthDisplay exists in a non-legacy skin,


I'd prefer these comments are in the actual migration rather than the test.

ie. for this one it should be at https://github.com/ppy/osu/pull/30867/files#diff-e662eed704866ec7b4c4ec234173d642e8628330ab7b5c4480e7f69070ca0e2dR345.

I've moved the one highlighted above, but there's also this:

osu/osu.Game.Tests/Skins/TestSceneSkinMigration.cs

Lines 247 to 248 in 5ab9074

// In this case, we must add a health display to the Playfield target,

// otherwise on mania the user will not see a health display anymore.

And there's no direct place to move it to the migration code, and I intended it to be in the test specifically to explain why the test case is written as such.

peppy · 2024-11-28T05:15:47Z

@frenzibyte Probably safe to update the failing migration tests now, I'm okay with how this is looking.

frenzibyte · 2024-11-29T04:31:18Z

I thought the skin deserialisation test failure was happening due to missing types or something, but turns out it was because of addressing #30867 (comment). It appears SkinDeserialisationTest was never able to run migrations, and the test results on some tests were outdated due to not being migrated.

I don't think this is correct behaviour, it feels quite trippy for a test to load an osk file and not have it migrated. I've rewritten the test to import the skin to a SkinManager instead.

frenzibyte · 2024-11-29T04:35:44Z

osu.Game.Tests/Skins/TestSceneSkinDeserialisation.cs

+            AddAssert("hud count = 13",
+                () => skin.LayoutInfos[GlobalSkinnableContainers.MainHUDComponents].AllDrawables.ToArray(),
+                () => Has.Length.EqualTo(13));


Some may notice the count assertions on some of these tests have changed. This is because previously the test did not count the combo counter migration, which moves the combo counter from the global layout to each ruleset's layout, therefore increasing the AllDrawables count by 3.

bdach

Source looks fine (aside from c12cb25 that I pushed in because the method had a bad name). However, for whatever reason the legacy default skin does not have a health bar in osu! and osu!catch specifically (taiko and mania somehow work):

2024-12-02.10-22-53.mp4

Not sure why this passes tests, but I'm not particularly wanting to investigate this, so leaving it to you to diagnose.

frenzibyte · 2024-12-02T20:05:07Z

Default ruleset-based implementations of osu! and osu!catch rulesets override the health display set up by LegacySkin... I'll need to investigate how to add appropriate tests for presence of default components.

… screen

frenzibyte · 2024-12-04T08:40:18Z

Fortunately enough the fix was simple. Added tests to ensure this doesn't slip by, by being critical of every default skinnable component displayed per skin. That area was not being appropriately tested by any of our existing tests so I rolled out a test scene for it, counting a total of 2 new test scenes and a rewrite of an existing one :^).

frenzibyte · 2024-12-04T08:43:26Z

osu.Game/Skinning/LegacySkin.cs

+                                // Notably, there may be existing ruleset skin transformers that override the default layout returned here.
+                                // Any added component in here should be considered in the existing transformers if applicable.


Added comment because I tripped on this one and caused a total disaster (as seen above).

Okay.

bdach · 2024-12-11T08:21:34Z

I'm not sure how much of a big deal to make of this but this diff can seemingly cause health bars to be doubled up on skins that already have a health bar in the playfield target.

Check out what happens when you import https://osu.ppy.sh/community/forums/topics/1923469?n=1 on master and switch to this branch. For me this happens:

before	after

Seems like this'll be the migration doing undesirable things...

frenzibyte · 2024-12-11T08:25:16Z

I think it's fine to exclude from migration skins which already have a health bar on the playfield target.

bdach

seems probably ok although I will complain that this has ballooned to ~~nearly~~ over 1k lines of diff

@peppy not sure if you want to cross-check this or not

peppy · 2024-12-24T12:50:39Z

I was going to get this in but the deserialisation test conflict is a bit of a pain to fix up.

frenzibyte added 3 commits November 25, 2024 00:02

Add osu!mania specifications for health bar display

8dca69e

Refactor skin migration to allow mutating multiple layouts at once

f1b5686

Migrate legacy health bar display to per-ruleset target

76f79ce

frenzibyte added ruleset/osu!mania area:skinning labels Nov 25, 2024

pull-request-size bot added the size/L label Nov 25, 2024

frenzibyte commented Nov 25, 2024

View reviewed changes

bdach requested changes Nov 25, 2024

View reviewed changes

frenzibyte added 4 commits November 25, 2024 23:23

Revert "Migrate legacy health bar display to per-ruleset target"

7df7727

This reverts commit 76f79ce.

Revert "Refactor skin migration to allow mutating multiple layouts at…

83ecfbd

… once" This reverts commit f1b5686.

Add migration logic for legacy health displays

fcbfbd0

pull-request-size bot added size/XL and removed size/L labels Nov 26, 2024

frenzibyte added 2 commits November 25, 2024 23:43

Move test and mark headless

5ab9074

Use type equality

0e920a6

peppy self-requested a review November 27, 2024 07:16

peppy reviewed Nov 27, 2024

View reviewed changes

Move non-legacy conditional reasoning to migration code

01f3a2d

Refactor skin deserialisation tests to better support skin migration

15a4726

frenzibyte commented Nov 29, 2024

View reviewed changes

bdach self-requested a review December 2, 2024 08:57

Document & reduce visibility of GetInstanceType()

c12cb25

bdach requested changes Dec 2, 2024

View reviewed changes

frenzibyte added 2 commits December 4, 2024 03:23

Merge branch 'master' into mania-hp-bar-position

45101b5

Mark skin deserialisation test as headless

03f06ae

frenzibyte added 2 commits December 4, 2024 03:36

Add test scene for checking all displayed default skin components per…

2318535

… screen

Fix osu! and osu!catch no longer displaying a health display

a2c57ee

pull-request-size bot added size/XXL and removed size/XL labels Dec 4, 2024

frenzibyte commented Dec 4, 2024

View reviewed changes

Remove redundant array creation expressions

c4a411e

Okay.

frenzibyte added 3 commits December 11, 2024 05:07

Add failing test case

c6d74aa

Exclude skins that contain health display on playfield target

b08a417

Merge branch 'master' into mania-hp-bar-position

8c238dd

bdach previously approved these changes Dec 16, 2024

View reviewed changes

peppy self-requested a review December 24, 2024 12:48

Merge branch 'master' into mania-hp-bar-position

855fa4e

frenzibyte dismissed bdach’s stale review via 855fa4e December 24, 2024 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement osu!mania-specific health bar display #30867

Implement osu!mania-specific health bar display #30867

frenzibyte commented Nov 25, 2024

frenzibyte Nov 25, 2024 •

edited

Loading

bdach Nov 25, 2024

frenzibyte Nov 25, 2024

frenzibyte Nov 26, 2024

frenzibyte Nov 26, 2024

bdach Nov 25, 2024

frenzibyte Nov 25, 2024

frenzibyte Nov 26, 2024

bdach Nov 26, 2024 •

edited

Loading

bdach Nov 25, 2024

frenzibyte Nov 25, 2024 •

edited

Loading

frenzibyte Nov 26, 2024

bdach Nov 25, 2024

frenzibyte Nov 25, 2024

frenzibyte Nov 26, 2024

peppy Nov 27, 2024

bdach Nov 25, 2024

frenzibyte commented Nov 26, 2024

peppy Nov 27, 2024

frenzibyte Nov 27, 2024

peppy commented Nov 28, 2024

frenzibyte commented Nov 29, 2024

frenzibyte Nov 29, 2024

bdach left a comment

frenzibyte commented Dec 2, 2024

frenzibyte commented Dec 4, 2024

frenzibyte Dec 4, 2024

bdach commented Dec 11, 2024 •

edited

Loading

frenzibyte commented Dec 11, 2024 •

edited

Loading

bdach left a comment •

edited

Loading

peppy commented Dec 24, 2024

	var legacyHealthBars = globalHUDComponents.Where(h => h.Type.Name == nameof(LegacyHealthDisplay)).ToArray();
	var legacyHealthBars = globalHUDComponents.Where(h => h.Type == typeof(LegacyHealthDisplay)).ToArray();

	// In this case, we must add a health display to the Playfield target,
	// otherwise on mania the user will not see a health display anymore.

		// Notably, there may be existing ruleset skin transformers that override the default layout returned here.
		// Any added component in here should be considered in the existing transformers if applicable.

Implement osu!mania-specific health bar display #30867

Are you sure you want to change the base?

Implement osu!mania-specific health bar display #30867

Conversation

frenzibyte commented Nov 25, 2024

frenzibyte Nov 25, 2024 • 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

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

bdach Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte Nov 25, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppy commented Nov 28, 2024

frenzibyte commented Nov 29, 2024

Choose a reason for hiding this comment

bdach left a comment

Choose a reason for hiding this comment

frenzibyte commented Dec 2, 2024

frenzibyte commented Dec 4, 2024

Choose a reason for hiding this comment

bdach commented Dec 11, 2024 • edited Loading

frenzibyte commented Dec 11, 2024 • edited Loading

bdach left a comment • edited Loading

Choose a reason for hiding this comment

peppy commented Dec 24, 2024

frenzibyte Nov 25, 2024 •

edited

Loading

bdach Nov 26, 2024 •

edited

Loading

frenzibyte Nov 25, 2024 •

edited

Loading

bdach commented Dec 11, 2024 •

edited

Loading

frenzibyte commented Dec 11, 2024 •

edited

Loading

bdach left a comment •

edited

Loading