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
4 changes: 0 additions & 4 deletions 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 @@ -35,10 +35,6 @@ public CatchLegacySkinTransformer(ISkin skin)
if (containerLookup.Ruleset == null)
return base.GetDrawableComponent(lookup);

// Skin has configuration.
if (base.GetDrawableComponent(lookup) is UserConfiguredLayoutContainer d)
return d;

// we don't have enough assets to display these components (this is especially the case on a "beatmap" skin).
if (!IsProvidingLegacyResources)
return null;
Expand Down
4 changes: 0 additions & 4 deletions 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 @@ -85,10 +85,6 @@ public override Drawable GetDrawableComponent(ISkinComponentLookup lookup)
if (containerLookup.Ruleset == null)
return base.GetDrawableComponent(lookup);

// Skin has configuration.
if (base.GetDrawableComponent(lookup) is UserConfiguredLayoutContainer d)
return d;

// we don't have enough assets to display these components (this is especially the case on a "beatmap" skin).
if (!IsProvidingLegacyResources)
return null;
Expand Down
4 changes: 0 additions & 4 deletions 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 @@ -49,10 +49,6 @@ public OsuLegacySkinTransformer(ISkin skin)
if (containerLookup.Ruleset == null)
return base.GetDrawableComponent(lookup);

// Skin has configuration.
if (base.GetDrawableComponent(lookup) is UserConfiguredLayoutContainer d)
return d;

// we don't have enough assets to display these components (this is especially the case on a "beatmap" skin).
if (!IsProvidingLegacyResources)
return null;
Expand Down
27 changes: 16 additions & 11 deletions osu.Game/Skinning/Skin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,18 +187,23 @@ public void UpdateDrawableTarget(SkinnableContainer targetContainer)
case SkinnableSprite.SpriteComponentLookup sprite:
return this.GetAnimation(sprite.LookupName, false, false, maxSize: sprite.MaxSize);

case GlobalSkinnableContainerLookup containerLookup:

// 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.Component, out var layoutInfo)) return null;
if (!layoutInfo.TryGetDrawableInfo(containerLookup.Ruleset, out var drawableInfos)) return null;

return new UserConfiguredLayoutContainer
case UserSkinComponentLookup userLookup:
switch (userLookup.Component)
{
RelativeSizeAxes = Axes.Both,
ChildrenEnumerable = drawableInfos.Select(i => i.CreateInstance())
};
case GlobalSkinnableContainerLookup containerLookup:
// 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.Component, out var layoutInfo)) return null;
if (!layoutInfo.TryGetDrawableInfo(containerLookup.Ruleset, out var drawableInfos)) return null;

return new UserConfiguredLayoutContainer
{
RelativeSizeAxes = Axes.Both,
ChildrenEnumerable = drawableInfos.Select(i => i.CreateInstance())
};
}

break;
}

return null;
Expand Down
5 changes: 4 additions & 1 deletion osu.Game/Skinning/SkinnableContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ public SkinnableContainer(GlobalSkinnableContainerLookup lookup)
Lookup = lookup;
}

public void Reload() => Reload(CurrentSkin.GetDrawableComponent(Lookup) as Container);
public void Reload() => Reload((
CurrentSkin.GetDrawableComponent(new UserSkinComponentLookup(Lookup))
?? CurrentSkin.GetDrawableComponent(Lookup))
as Container);

public void Reload(Container? componentsContainer)
{
Expand Down
18 changes: 18 additions & 0 deletions osu.Game/Skinning/UserSkinComponentLookup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

namespace osu.Game.Skinning
{
/// <summary>
/// A lookup class which is only for internal use, and explicitly to get a user-level configuration.
/// </summary>
internal class UserSkinComponentLookup : ISkinComponentLookup
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here I guess I have my answer to the previous thing I guess, or maybe not?

The problem I have here is that UserSkinComponentLookup is going to wrap an ISkinComponentLookup, which can be a GlobalSkinnableContainerLookup, which again confuses me because I delineate the notion of components and containers of components as two notions that will not cross. If that is the case, then I'd argue the following renames should also happen:

  • ISkinComponentLookup -> ISkinLookup
  • UserSkinComponentLookup -> UserSkinLookup

because they encapsulate both lookups of individual components as well as containers of components.

If I am not correct in assuming that containers of components and components are separate notions, then I guess disregard all I said and you can probably merge this.

Copy link
Contributor

Choose a reason for hiding this comment

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

ISkinComponentLookup -> ISkinLookup

I think I agree with this in principle, just to remove N+1 keywords. However....

UserSkinComponentLookup -> UserSkinLookup

I don't agree with this one because it sounds like "look up this thing from the user's skin specifically", like "ignore any argon/whatever base skin". Original naming I think is best for this one.

Also, UserSkinComponentLookup may not be just containers in the future. It depends on the path this takes but it may be something passed through to a ISkin.GetConfiguration(UserSkinComponentLookup) to configure SkinnableDrawables (but not override them).
THAT SAID, I will stress that this is up in the air, so it's just my own ramblings at this point.

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.

Suggestions thrown around for that second one: UserConfiguredSkinLookup JsonSkinLookup. I'd be alright with that, they don't have to be 1-to-1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If UserSkinComponentLookup is going to wrap an ISkinComponentLookup then I'm not sure it's an option to drop Component from only one. It's essentially a decorator pattern, so it should be transformative in nature, and not add any new qualifiers or whatever - otherwise something is just straight up not shaped right.

Probably fine to leave as is, I'm not hugely fussed at this stage.

Copy link
Member Author

@peppy peppy 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.

FWIW I also proposed dropping "Component" from everywhere, including GetDrawableComponent, but smoogi didn't agree with this so I dropped it for now.

We can probably leave it everywhere for now and reconsider later.

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.

Rather than disagreeing, I want things to be consistent above all. It's a big part of the reason why we got into this mess in the first place (SkinComponentsContainer <> SkinnableDrawable) and the core of my arguments this year.

If we can't agree that keeping naming consistent based on what things are rather than what their end product is, then I don't think we can comfortably move forward, which is why I pushed back against GlobalSkinnableContainerLookup.
Especially during a refactoring stage when everything's new, I think it's best to keep things consistent so we don't step over our feet, and hopefully the next time we look at this code we won't have to piece together what, if any, special functionality is awarded to certain types that justifies their distinct naming.

Over time, when people get used to the structure of things, it will matter not whether a *ComponentLookup type returns a container or a drawable; but only that it represents a lookup type that's defined somewhere in a transformer. There's many paths this could go down in the future, but I'll leave that for a separate time to not derail this.

That is all to say, I'm not tied to "Component", but it is the keyword that has remained consistent and this PR is just moving the rest back in line with it. I'm not against dropping it unless it causes confusion somwhere - saying certain words like "ISkinLookup" or "OsuSkinLookup" or "OsuSkinLookups" (I guess? the enum name) out loud makes it quite appealing.

Copy link
Member

Choose a reason for hiding this comment

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

Adding my two cents here, dropping the term "Component" in these lookup classes makes me kinda anxious because it gives a more generalised name for the type than what it's actually supposed to be used for. As in, we can have ISkinLookup, but that only will look up drawables/containers, it does not look up textures/samples/config/etc.

We even have ManiaSkinConfigurationLookup existing right now, and having that next to what would be ManiaSkinLookup does not sound good to me at all.

{
public readonly ISkinComponentLookup Component;

public UserSkinComponentLookup(ISkinComponentLookup component)
{
Component = component;
}
}
}
Loading