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.

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

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

	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.

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

frenzibyte Nov 25, 2024 •

edited

Loading

bdach Nov 26, 2024 •

edited

Loading

frenzibyte Nov 25, 2024 •

edited

Loading