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.

Sign up for GitHub

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

Jump to bottom

Remove requirement of base calls to ensure user skin container layouts are retrieved #29564

Merged
merged 8 commits into from
Aug 23, 2024
Merged

Remove requirement of base calls to ensure user skin container layouts are retrieved #29564

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4175652
Rename `SkinComponentsContainer` to `SkinnableContainer`
peppy Aug 22, 2024
f37cab0
Rename `SkinComponentsContainerLookup` to `GlobalSkinnableContainerLo…
peppy Aug 22, 2024
36b4013
Rename `GameplaySkinComponentLookup` -> `SkinComponentLookup`
peppy Aug 22, 2024
9a21174
Move `GlobalSkinnableContainers` to global scope
peppy Aug 22, 2024
b57b816
Rename `Target` lookup to `Component`
peppy Aug 22, 2024
1435fe2
Remove requirement of `base` calls to ensure user skin container layo…
peppy Aug 22, 2024
46d55d5
Remove remaining early `base` lookup calls which were missed
peppy Aug 22, 2024
48cfd77
`Component` -> `Lookup`
peppy Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public CatchLegacySkinTransformer(ISkin skin)
return null;

// Our own ruleset components default.
switch (containerLookup.Target)
switch (containerLookup.Component)
{
case GlobalSkinnableContainers.MainHUDComponents:
// todo: remove CatchSkinComponents.CatchComboCounter and refactor LegacyCatchComboCounter to be added here instead.
Expand Down
2 changes: 1 addition & 1 deletion osu.Game.Rulesets.Mania/Skinning/Argon/ManiaArgonSkinTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public ManiaArgonSkinTransformer(ISkin skin, IBeatmap beatmap)
if (base.GetDrawableComponent(lookup) is UserConfiguredLayoutContainer d)
return d;

switch (containerLookup.Target)
switch (containerLookup.Component)
{
case GlobalSkinnableContainers.MainHUDComponents:
return new DefaultSkinComponentsContainer(container =>
Expand Down
2 changes: 1 addition & 1 deletion osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public override Drawable GetDrawableComponent(ISkinComponentLookup lookup)
if (!IsProvidingLegacyResources)
return null;

switch (containerLookup.Target)
switch (containerLookup.Component)
{
case GlobalSkinnableContainers.MainHUDComponents:
return new DefaultSkinComponentsContainer(container =>
Expand Down
2 changes: 1 addition & 1 deletion osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public OsuLegacySkinTransformer(ISkin skin)
return null;

// Our own ruleset components default.
switch (containerLookup.Target)
switch (containerLookup.Component)
{
case GlobalSkinnableContainers.MainHUDComponents:
return new DefaultSkinComponentsContainer(container =>
Expand Down
2 changes: 1 addition & 1 deletion osu.Game.Tests/Visual/Gameplay/TestSceneBeatmapSkinFallbacks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ protected void CreateSkinTest(SkinInfo gameCurrentSkin, Func<ISkin> getBeatmapSk

protected bool AssertComponentsFromExpectedSource(GlobalSkinnableContainers target, ISkin expectedSource)
{
var targetContainer = Player.ChildrenOfType<SkinnableContainer>().First(s => s.Lookup.Target == target);
var targetContainer = Player.ChildrenOfType<SkinnableContainer>().First(s => s.Lookup.Component == target);
var actualComponentsContainer = targetContainer.ChildrenOfType<Container>().SingleOrDefault(c => c.Parent == targetContainer);

if (actualComponentsContainer == null)
Expand Down
4 changes: 2 additions & 2 deletions osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,10 +378,10 @@ public void TestCopyPaste()
}

private SkinnableContainer globalHUDTarget => Player.ChildrenOfType<SkinnableContainer>()
.Single(c => c.Lookup.Target == GlobalSkinnableContainers.MainHUDComponents && c.Lookup.Ruleset == null);
.Single(c => c.Lookup.Component == GlobalSkinnableContainers.MainHUDComponents && c.Lookup.Ruleset == null);

private SkinnableContainer rulesetHUDTarget => Player.ChildrenOfType<SkinnableContainer>()
.Single(c => c.Lookup.Target == GlobalSkinnableContainers.MainHUDComponents && c.Lookup.Ruleset != null);
.Single(c => c.Lookup.Component == GlobalSkinnableContainers.MainHUDComponents && c.Lookup.Ruleset != null);

[Test]
public void TestMigrationArgon()
Expand Down
2 changes: 1 addition & 1 deletion osu.Game/Skinning/ArgonSkin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public ArgonSkin(SkinInfo skin, IStorageResourceProvider resources)
if (base.GetDrawableComponent(lookup) is UserConfiguredLayoutContainer c)
return c;

switch (containerLookup.Target)
switch (containerLookup.Component)
{
case GlobalSkinnableContainers.SongSelect:
var songSelectComponents = new DefaultSkinComponentsContainer(_ =>
Expand Down
14 changes: 7 additions & 7 deletions osu.Game/Skinning/GlobalSkinnableContainerLookup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,33 @@ public class GlobalSkinnableContainerLookup : ISkinComponentLookup, IEquatable<G
/// <summary>
/// The target area / layer of the game for which skin components will be returned.
/// </summary>
public readonly GlobalSkinnableContainers Target;
public readonly GlobalSkinnableContainers Component;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure of this rename, I'd probably propose to call this something like GlobalSkinnableContainer or SkinnableContainer or something.

Unless I'm wildly misunderstanding and about to open another debate into naming (in which case please do feel free to immediately tell me to shut the ---- up), GlobalSkinnableContainerLookup is supposed to lookup skinnable containers, which are not individual skinnable components themselves (think score counter, health bar, etc.), but rather containers of skinnable components.

If that understanding is correct then I find this being called Component misleading because it's not going to look up a component as I think of it.

Copy link
Contributor

@smoogipoo smoogipoo Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As for my perspective, I originally wanted this to be called GlobalSkinComponentLookup, because I think of everything skinning as a "component". In that case, this being Component makes sense to me.

In my original PR, I named this Target so I'm definitely thinking in the direction of renaming, regardless of the above.

I disagree with GlobalSkinnableContainer or the other suggestion because there is a world where this is moved to ISkinComponentLookup as I originally had, for the purpose of determining the file name, at which point being called that doesn't really make sense. I'm not really sure what path this will go down in the end, but it feels too inflexible and hard-lined as that.

How about Lookup? We are throwing this around in the office and it seems agreeable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As for my perspective, I originally wanted this to be called GlobalSkinComponentLookup, because I think of everything skinning as a "component". In that case, this being Component makes sense to me.

If this is the consensus from everyone I'll just rewire my brain. Lookup is also fine I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm alone in that regard 😛


/// <summary>
/// The ruleset for which skin components should be returned.
/// A <see langword="null"/> value means that returned components are global and should be applied for all rulesets.
/// </summary>
public readonly RulesetInfo? Ruleset;

public GlobalSkinnableContainerLookup(GlobalSkinnableContainers target, RulesetInfo? ruleset = null)
public GlobalSkinnableContainerLookup(GlobalSkinnableContainers component, RulesetInfo? ruleset = null)
{
Target = target;
Component = component;
Ruleset = ruleset;
}

public override string ToString()
{
if (Ruleset == null) return Target.GetDescription();
if (Ruleset == null) return Component.GetDescription();

return $"{Target.GetDescription()} (\"{Ruleset.Name}\" only)";
return $"{Component.GetDescription()} (\"{Ruleset.Name}\" only)";
}

public bool Equals(GlobalSkinnableContainerLookup? other)
{
if (ReferenceEquals(null, other)) return false;
if (ReferenceEquals(this, other)) return true;

return Target == other.Target && (ReferenceEquals(Ruleset, other.Ruleset) || Ruleset?.Equals(other.Ruleset) == true);
return Component == other.Component && (ReferenceEquals(Ruleset, other.Ruleset) || Ruleset?.Equals(other.Ruleset) == true);
}

public override bool Equals(object? obj)
Expand All @@ -55,7 +55,7 @@ public override bool Equals(object? obj)

public override int GetHashCode()
{
return HashCode.Combine((int)Target, Ruleset);
return HashCode.Combine((int)Component, Ruleset);
}
}
}
2 changes: 1 addition & 1 deletion osu.Game/Skinning/LegacyBeatmapSkin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ private static IResourceStore<byte[]> createRealmBackedStore(BeatmapInfo beatmap
{
if (lookup is GlobalSkinnableContainerLookup containerLookup)
{
switch (containerLookup.Target)
switch (containerLookup.Component)
{
case GlobalSkinnableContainers.MainHUDComponents:
// this should exist in LegacySkin instead, but there isn't a fallback skin for LegacySkins yet.
Expand Down
2 changes: 1 addition & 1 deletion osu.Game/Skinning/LegacySkin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ protected override void ParseConfigurationStream(Stream stream)
if (base.GetDrawableComponent(lookup) is UserConfiguredLayoutContainer c)
return c;

switch (containerLookup.Target)
switch (containerLookup.Component)
{
case GlobalSkinnableContainers.MainHUDComponents:
if (containerLookup.Ruleset != null)
Expand Down
8 changes: 4 additions & 4 deletions osu.Game/Skinning/Skin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ protected virtual void ParseConfigurationStream(Stream stream)
/// <param name="targetContainer">The target container to reset.</param>
public void ResetDrawableTarget(SkinnableContainer targetContainer)
{
LayoutInfos.Remove(targetContainer.Lookup.Target);
LayoutInfos.Remove(targetContainer.Lookup.Component);
}

/// <summary>
Expand All @@ -173,8 +173,8 @@ public void ResetDrawableTarget(SkinnableContainer targetContainer)
/// <param name="targetContainer">The target container to serialise to this skin.</param>
public void UpdateDrawableTarget(SkinnableContainer targetContainer)
{
if (!LayoutInfos.TryGetValue(targetContainer.Lookup.Target, out var layoutInfo))
layoutInfos[targetContainer.Lookup.Target] = layoutInfo = new SkinLayoutInfo();
if (!LayoutInfos.TryGetValue(targetContainer.Lookup.Component, out var layoutInfo))
layoutInfos[targetContainer.Lookup.Component] = layoutInfo = new SkinLayoutInfo();

layoutInfo.Update(targetContainer.Lookup.Ruleset, ((ISerialisableDrawableContainer)targetContainer).CreateSerialisedInfo().ToArray());
}
Expand All @@ -191,7 +191,7 @@ public void UpdateDrawableTarget(SkinnableContainer targetContainer)

// It is important to return null if the user has not configured this yet.
// This allows skin transformers the opportunity to provide default components.
if (!LayoutInfos.TryGetValue(containerLookup.Target, out var layoutInfo)) return null;
if (!LayoutInfos.TryGetValue(containerLookup.Component, out var layoutInfo)) return null;
if (!layoutInfo.TryGetDrawableInfo(containerLookup.Ruleset, out var drawableInfos)) return null;

return new UserConfiguredLayoutContainer
Expand Down
2 changes: 1 addition & 1 deletion osu.Game/Skinning/TrianglesSkin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public TrianglesSkin(SkinInfo skin, IStorageResourceProvider resources)
if (containerLookup.Ruleset != null)
return null;

switch (containerLookup.Target)
switch (containerLookup.Component)
{
case GlobalSkinnableContainers.SongSelect:
var songSelectComponents = new DefaultSkinComponentsContainer(_ =>
Expand Down