Add maximum dimensions limit to skinnable gameplay elements #24706
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,23 +11,24 @@ | |
| using osu.Framework.Graphics.Animations; | ||
| using osu.Framework.Graphics.Sprites; | ||
| using osu.Framework.Graphics.Textures; | ||
| using osuTK; | ||
| using static osu.Game.Skinning.SkinConfiguration; | ||
|
|
||
| namespace osu.Game.Skinning | ||
| { | ||
| public static partial class LegacySkinExtensions | ||
| { | ||
| public static Drawable? GetAnimation(this ISkin? source, string componentName, bool animatable, bool looping, bool applyConfigFrameRate = false, string animationSeparator = "-", | ||
| bool startAtCurrentTime = true, double? frameLength = null, Vector2? maxSize = null) | ||
| => source.GetAnimation(componentName, default, default, animatable, looping, applyConfigFrameRate, animationSeparator, startAtCurrentTime, frameLength, maxSize); | ||
|
|
||
| public static Drawable? GetAnimation(this ISkin? source, string componentName, WrapMode wrapModeS, WrapMode wrapModeT, bool animatable, bool looping, bool applyConfigFrameRate = false, | ||
| string animationSeparator = "-", bool startAtCurrentTime = true, double? frameLength = null, Vector2? maxSize = null) | ||
| { | ||
| if (source == null) | ||
| return null; | ||
|
|
||
| var textures = GetTextures(source, componentName, wrapModeS, wrapModeT, animatable, animationSeparator, maxSize, out var retrievalSource); | ||
|
|
||
| switch (textures.Length) | ||
| { | ||
|
|
@@ -53,7 +54,7 @@ public static partial class LegacySkinExtensions | |
| } | ||
| } | ||
|
|
||
| public static Texture[] GetTextures(this ISkin? source, string componentName, WrapMode wrapModeS, WrapMode wrapModeT, bool animatable, string animationSeparator, Vector2? maxSize, out ISkin? retrievalSource) | ||
| { | ||
| retrievalSource = null; | ||
|
|
||
|
|
@@ -78,7 +79,9 @@ public static Texture[] GetTextures(this ISkin? source, string componentName, Wr | |
| } | ||
|
|
||
| // if an animation was not allowed or not found, fall back to a sprite retrieval. | ||
| var singleTexture = maxSize != null | ||
| ? retrievalSource.GetTextureWithMaxSize(componentName, maxSize.Value, wrapModeS, wrapModeT) | ||
| : retrievalSource.GetTexture(componentName, wrapModeS, wrapModeT); | ||
|
|
||
| return singleTexture != null | ||
| ? new[] { singleTexture } | ||
|
|
@@ -88,9 +91,11 @@ IEnumerable<Texture> getTextures(ISkin skin) | |
| { | ||
| for (int i = 0; true; i++) | ||
| { | ||
| var texture = maxSize != null | ||
| ? skin.GetTextureWithMaxSize(getFrameName(i), maxSize.Value, wrapModeS, wrapModeT) | ||
| : skin.GetTexture(getFrameName(i), wrapModeS, wrapModeT); | ||
|
|
||
| if (texture == null) | ||
| break; | ||
|
|
||
| yield return texture; | ||
|
|
@@ -100,6 +105,20 @@ IEnumerable<Texture> getTextures(ISkin skin) | |
| string getFrameName(int frameIndex) => $"{componentName}{animationSeparator}{frameIndex}"; | ||
| } | ||
|
|
||
| public static Texture? GetTextureWithMaxSize(this ISkin source, string componentName, Vector2 maxSize, WrapMode wrapModeS = WrapMode.None, WrapMode wrapModeT = WrapMode.None) | ||
|
There was a problem hiding this comment. More to the code side of things, why is this a separate method, but There was a problem hiding this comment. I've done some code mashing and removed this overload / new method. See if it reads better to you. Note that I did this as a prelim requirement. I haven't yet assessed whether the overall direction is correct yet. There was a problem hiding this comment. Just quickly going over source without getting too deep into it, definitely seems much closer to what I'd expect to see. There was a problem hiding this comment.
It would've been better presenting the change as a proposal first before pushing. I'm not in favour with the new code as it means every skin implementation must respect the Both of the reasons above are precisely why I haven't considered doing this. As an alternative, I wouldn't mind something like There was a problem hiding this comment.
I don't get why this is a concern... If There was a problem hiding this comment. Well, the change has already been pushed, and if you're both on board with it then I wouldn't bother to mind it. |
||
| { | ||
| var texture = source.GetTexture(componentName, wrapModeS, wrapModeT); | ||
| if (texture == null) | ||
| return texture; | ||
|
|
||
| if (texture.DisplayWidth <= maxSize.X && texture.DisplayHeight <= maxSize.Y) | ||
| return texture; | ||
|
|
||
| // use scale adjust property for downscaling the texture in order to meet the specified maximum dimensions. | ||
| texture.ScaleAdjust *= Math.Max(texture.DisplayWidth / maxSize.X, texture.DisplayHeight / maxSize.Y); | ||
|
Comment on lines
+115
to
+116
There was a problem hiding this comment. We're going to have to be very conscious about this, to ensure we don't have any local usages overriding I've checked all usages in the project right now and it seems okay. For now. There was a problem hiding this comment. On a second thought, an alternative way to achieve this could be to have something like a "maximum texture size" property in That might be better as to avoid tinkering around with low properties like |
||
| return texture; | ||
| } | ||
|
|
||
| public static bool HasFont(this ISkin source, LegacyFont font) | ||
| { | ||
| return source.GetTexture($"{source.GetFontPrefix(font)}-0") != null; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does 100 come from? The template skin is 82 x 103 (
@1x).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to say it's correct, but I think I picked 100 as it's closest to 103...I'll change to 82x103