Add maximum dimensions limit to skinnable gameplay elements #24706

frenzibyte · 2023-09-02T23:38:49Z

Resolves Ensure all gameplay elements have maximum dimensions #23099

RFC. This applies to osu! hit circles, taiko hits (drum rolls are already limited), and catch hit objects (fruits, bananas, and droplets). Mania is not included as I'm not even sure how to go about it (if I should).

From the get go, I was planning on re-using SkinnableDrawable's "size confinement mode" logic, but it's not quite usable for the cases in here for two reasons:

Most cases don't create SkinnableDrawable because they're already inside a per-skin component and they fetch the texture directly from the skin.
SkinnableDrawable's size confinement mode doesn't handle texture elements smaller than the standard size (there's a bit of an argument over that one).

So I came up with the idea of a simple Drawable class that wraps around a sprite and downscales it if it exceeds the specified maximum dimensions or the draw size of a specified "target" drawable. However, that resulted in few issues with osu! hit circles, as they apply upscaling to the circle & overlay pieces on hit, which cannot work properly with the class in mind (see discord).

Therefore, I switched to applying the maximum dimensions limit at a texture level instead, by a helper method that tinkers with Texture.ScaleAdjust to downscale the texture if it exceeds maximum dimensions.

Opening this pull request as draft because it is mostly for feedback, and because I haven't thoroughly tested it or compared it with #23025 yet.

bdach · 2023-09-04T07:06:12Z

I have several questions...

Most cases don't create SkinnableDrawable because they're already inside a per-skin component and they fetch the texture directly from the skin.

Did you try and see if they can be made SkinnableDrawables? Basically I'm quite loath of providing two ways of doing one thing.

2. SkinnableDrawable's size confinement mode doesn't handle texture elements smaller than the standard size (there's a bit of an argument over that one).

Kinda weird to PR this without reaching a consensus on that first... Basically I can't find a thread on this now, but I recall somebody mentioning somewhere that if you make a really small taiko circle texture, what will happen is that on maps with significant note overlap, the circles due to the shrinkage will overlap less, resulting in easier reading. I think that was the rationale (can we summon like a taiko player or something? maybe i'll try the difficulty channel on discord...)

Other than that:

Do we have any degree of confidence that the coverage of elements you have in this pull is in any way comprehensive? As in, how was it determined that only these specific elements need touching?
Do you have resources that you can provide for testing this? I guess the assets provided in this pull are one, but being able to get a full skin for comparison & crosscheck against stable would be nice.

bdach · 2023-09-04T07:07:44Z

osu.Game/Skinning/LegacySkinExtensions.cs

@@ -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)


More to the code side of things, why is this a separate method, but GetAnimation() gets an optional maxSize param? Should be one or the other all the way.

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.

Just quickly going over source without getting too deep into it, definitely seems much closer to what I'd expect to see.

I've done some code mashing and removed this overload / new method. See if it reads better to you.

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 maxSize parameter & perform the same exact code of trimming the texture, which is better achieved with an extension method rather than providing that as part of the core method itself, especially since the "maximum size" logic can be executed outside of the core method. Not to mention that this massively increases the diff of this PR.

Both of the reasons above are precisely why I haven't considered doing this.

As an alternative, I wouldn't mind something like skin.GetTexture().WithMaximumSize() or just keep everything as is and rename GetTextureWithMaxSize to GetTexture. Whichever feels better (I would prefer the first, because it's very explicit and reads well in code).

it means every skin implementation must respect the maxSize parameter & perform the same exact code of trimming the texture

I don't get why this is a concern... If maxSize is null then the method is a no-op, first of all, so there's no performance consideration there. Secondly, we probably want the max size logic in all implementations for future-proofing. (We were even musing briefly in twitch chat about what happens when user skins are not legacy-texture-based but markup-drawable-based in the future, but dismissed that as unimportant for now.) The diff size concern is like basically irrelevant to me.

Well, the change has already been pushed, and if you're both on board with it then I wouldn't bother to mind it.

bdach · 2023-09-04T07:52:27Z

I recall somebody mentioning somewhere that if you make a really small taiko circle texture, what will happen is that on maps with significant note overlap, the circles due to the shrinkage will overlap less, resulting in easier reading

Asked about this on discord and the general response seems to be that yes it's a problem but probably not one that needs dealing with every urgently. So I guess this point can be dismissed as not hugely relevant.

peppy · 2023-09-05T08:33:04Z

Probably needs to be added to more elements (for example taiko-roll-end, taiko-roll-middle).

peppy · 2023-09-05T08:47:52Z

One issue with doing this at the texture level is that in the edge case that a skin element is oversize (ie. due to empty transparent padding), it will be downscaled rather than just-work. That may have been one of the original reasons to do it the way I had it.

peppy · 2023-09-05T09:04:56Z

Do you have resources that you can provide for testing this? I guess the assets provided in this pull are one, but being able to get a full skin for comparison & crosscheck against stable would be nice.

I think a better approach to test this is to roll back all the resource changes, then create a standalone autoplay gameplay test which has a custom skin provider that upscales every element (except whitelisted ones) to 4x normal scale or something stupid.

I'd explicitly avoid using 2x scaling, as that may trigger @2x edge cases. So if anything, something like 2.4 or 3.

That way we can go through and find the elements easily which haven't had their max dimensions set yet, and we also have a whitelist of all elements we want to allow being larger than expected.

I think this kind of testing is required regardless of the solution, and it would allow better experimentation with potential alternatives.

@frenzibyte want to take a look at this if you agree with the direction?

peppy

As commented.

frenzibyte · 2023-09-05T20:08:44Z

I have several questions...

Most cases don't create SkinnableDrawable because they're already inside a per-skin component and they fetch the texture directly from the skin.

Did you try and see if they can be made SkinnableDrawables? Basically I'm quite loath of providing two ways of doing one thing.

It's possible to replace each use case with a SkinnableSprite/SkinnableAnimation, but I was hesitant of doing that as it means defining a SkinnableDrawable inside of a SkinnableDrawable implementation, which doesn't really make sense because 99% of the inner SkinnableDrawable's functionality (updating on skin change) is moot because it's handled by the outer SkinnableDrawable.

If anything, I would remove the SizeConfinementMode thing from SkinnableDrawable and wrap each usage of that enum with the new container. To me, that looks like the logical step towards this.

SkinnableDrawable's size confinement mode doesn't handle texture elements smaller than the standard size (there's a bit of an argument over that one).

Kinda weird to PR this without reaching a consensus on that first... Basically I can't find a thread on this now, but I recall somebody mentioning somewhere that if you make a really small taiko circle texture, what will happen is that on maps with significant note overlap, the circles due to the shrinkage will overlap less, resulting in easier reading. I think that was the rationale (can we summon like a taiko player or something? maybe i'll try the difficulty channel on discord...)

I believed the consensus has already been reached on peppy's message that "smaller is always allowed"? Well, I suppose the OP doesn't clarify that well.

Other than that:

Do we have any degree of confidence that the coverage of elements you have in this pull is in any way comprehensive? As in, how was it determined that only these specific elements need touching?

I've opened this PR to discuss the direction first before making sure this covers all elements. I've already pointed this out in OP at the very end.

Do you have resources that you can provide for testing this? I guess the assets provided in this pull are one, but being able to get a full skin for comparison & crosscheck against stable would be nice.

Sure, I can prepare one once this PR reaches the testing state, but the assets provided were meant to be enough to provide samples of how the logic for the direction works in practice.

frenzibyte · 2023-09-05T20:10:54Z

osu.Game.Rulesets.Taiko/Skinning/Legacy/LegacyDrumRoll.cs

@@ -47,13 +48,13 @@ private void load(ISkinSource skin, OsuColour colours)
                    Anchor = Anchor.CentreRight,
                    Origin = Anchor.CentreLeft,
                    RelativeSizeAxes = Axes.Both,
-                    Texture = skin.GetTexture("taiko-roll-end", WrapMode.ClampToEdge, WrapMode.ClampToEdge),
+                    Texture = skin.GetTexture("taiko-roll-end", new Vector2(128, 256), WrapMode.ClampToEdge, WrapMode.ClampToEdge),


I intentionally haven't included osu!taiko drum rolls because they're already being resized in the Sprite definition (notice RSA = Both & FillMode = Fit).

frenzibyte · 2023-09-05T20:20:48Z

osu.Game/Skinning/LegacySkinExtensions.cs

@@ -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)


I've done some code mashing and removed this overload / new method. See if it reads better to you.

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 maxSize parameter & perform the same exact code of trimming the texture, which is better achieved with an extension method rather than providing that as part of the core method itself, especially since the "maximum size" logic can be executed outside of the core method. Not to mention that this massively increases the diff of this PR.

Both of the reasons above are precisely why I haven't considered doing this.

As an alternative, I wouldn't mind something like skin.GetTexture().WithMaximumSize() or just keep everything as is and rename GetTextureWithMaxSize to GetTexture. Whichever feels better (I would prefer the first, because it's very explicit and reads well in code).

frenzibyte · 2023-09-05T20:23:39Z

Do you have resources that you can provide for testing this? I guess the assets provided in this pull are one, but being able to get a full skin for comparison & crosscheck against stable would be nice.

I think a better approach to test this is to roll back all the resource changes, then create a standalone autoplay gameplay test which has a custom skin provider that upscales every element (except whitelisted ones) to 4x normal scale or something stupid.

I'd explicitly avoid using 2x scaling, as that may trigger @2x edge cases. So if anything, something like 2.4 or 3.

That way we can go through and find the elements easily which haven't had their max dimensions set yet, and we also have a whitelist of all elements we want to allow being larger than expected.

I think this kind of testing is required regardless of the solution, and it would allow better experimentation with potential alternatives.

@frenzibyte want to take a look at this if you agree with the direction?

Definitely, I can look into that. It will get a bit funny with elements that are fine to be scaled (e.g. HUD elements), but I'll play with the test to exclude those and see how it goes.

peppy · 2023-09-06T05:17:43Z

As an alternative, I wouldn't mind something like skin.GetTexture().WithMaximumSize() or just keep everything as is and rename GetTextureWithMaxSize to GetTexture. Whichever feels better (I would prefer the first, because it's very explicit and reads well in code).

Sure, that works better and requires less code changes. (I was almost going to suggest a TextureRetrievalArgs class since there's already so many arguments on the methods).

Feel free to undo my change and apply that, or just revert it.

Definitely, I can look into that. It will get a bit funny with elements that are fine to be scaled (e.g. HUD elements), but I'll play with the test to exclude those and see how it goes.

That's what I meant by a whitelist. It means we will be sure we haven't missed any elements we care about.

This reverts commit 96f12cf.

peppy · 2023-09-19T05:17:13Z

osu.Game.Tests/Visual/Gameplay/TestSceneGameplayElementDimensions.cs

+
+namespace osu.Game.Tests.Visual.Gameplay
+{
+    public partial class TestSceneGameplayElementDimensions : TestSceneAllRulesetPlayers


What on earth is this naming..

I...don't know?

peppy

As commented.

peppy · 2023-09-20T01:25:47Z

I managed to fix the crash, it was something weird with my local caches.

peppy · 2023-09-20T03:48:27Z

osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs

@@ -41,14 +42,14 @@ public OsuLegacySkinTransformer(ISkin skin)
                        return this.GetAnimation("sliderscorepoint", false, false);

                    case OsuSkinComponents.SliderFollowCircle:
-                        var followCircleContent = this.GetAnimation("sliderfollowcircle", true, true, true);
+                        var followCircleContent = this.GetAnimation("sliderfollowcircle", true, true, true, maxSize: new Vector2(308f));


Where does this 308 come from?

Wiki, maximum effective follow circle size (https://osu.ppy.sh/wiki/en/Skinning/osu%21#slider, see sliderfollowcircle.png).

…ze is per glyph

peppy · 2023-09-20T04:01:27Z

osu.Game/Skinning/LegacySkinExtensions.cs

+            // 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);


We're going to have to be very conscious about this, to ensure we don't have any local usages overriding ScaleAdjust themselves.

I've checked all usages in the project right now and it seems okay. For now.

On a second thought, an alternative way to achieve this could be to have something like a "maximum texture size" property in SkinnableSprite, and if the sprite exceeds that property then the scale of it is adjusted accordingly.

That might be better as to avoid tinkering around with low properties like Texture.ScaleAdjust. Also saves from having to add extra properties to GetTexture/GetAnimation. @peppy thoughts? would it be worth the effort?

peppy · 2023-09-20T04:05:16Z

I know for a fact that some of these limitations are going to cause skins to break, and will need to be relaxed. For instance, there are skins which add glows to hitobjects, which we previously allowed. In fact, I'm pretty sure there was a previous iteration in lazer where we were limiting things and I fixed such a case.

The only way to figure out sane limits will be to push out these changes and wait for users to report breakage, then decide how much wiggle-room we give skins for exceeding the "correct" dimensions.

peppy · 2023-09-20T04:14:56Z

osu.Game.Rulesets.Taiko/Skinning/Legacy/LegacyDrumRoll.cs

@@ -47,13 +48,13 @@ private void load(ISkinSource skin, OsuColour colours)
                    Anchor = Anchor.CentreRight,
                    Origin = Anchor.CentreLeft,
                    RelativeSizeAxes = Axes.Both,
-                    Texture = skin.GetTexture("taiko-roll-end", WrapMode.ClampToEdge, WrapMode.ClampToEdge),
+                    Texture = skin.GetTexture("taiko-roll-end", WrapMode.ClampToEdge, WrapMode.ClampToEdge)?.WithMaximumSize(new Vector2(128, 256)),


Why are these 256 based, not 128?

Also adjusting this doesn't affect the test scene at all?

Uhh...9d17539

And yes, adjusting doesn't affect because the sprite is resized regardless of the texture, see #24706 (comment).

peppy · 2023-09-20T04:16:02Z

osu.Game.Rulesets.Catch/Skinning/Legacy/LegacyDropletPiece.cs

 using osuTK;

 namespace osu.Game.Rulesets.Catch.Skinning.Legacy
 {
    public partial class LegacyDropletPiece : LegacyCatchHitObjectPiece
    {
+        private static readonly Vector2 droplet_max_size = new Vector2(100);


Where does 100 come from? The template skin is 82 x 103 (@1x).

Not to say it's correct, but I think I picked 100 as it's closest to 103...I'll change to 82x103

Already handled by the sprites themselves being resized.

…absurdly high

peppy · 2023-09-26T07:41:11Z

Will reiterate that this will 100% cause some kind of user skin regressions, which we'll deal with based on feedback.

I know for a fact that some of these limitations are going to cause skins to break, and will need to be relaxed. For instance, there are skins which add glows to hitobjects, which we previously allowed. In fact, I'm pretty sure there was a previous iteration in lazer where we were limiting things and I fixed such a case.

The only way to figure out sane limits will be to push out these changes and wait for users to report breakage, then decide how much wiggle-room we give skins for exceeding the "correct" dimensions.

peppy · 2023-09-26T08:30:54Z

Turns out hot reload was not working, and my scale change in the test scene was not applying correctly in all cases.

As a result, the test scene is quite broken. But in addition, I missed quite a few elements which weren't correctly limited by this PR. Things like follow points (which can be used to reveal object positions in flashlight) need special consideration.

@frenzibyte did you want to look into these cases again?

frenzibyte added 5 commits September 3, 2023 02:21

Make gameplay skin elements in special-skin awfully large

2c81fe5

Add support for limiting skin texture/animation dimensions

6674c70

Add limit to osu! hit circle elements

351081e

Add limit to taiko hit elements

d286816

Add limit to catch palpable object elements

f182f57

frenzibyte added area:gameplay type:behavioural area:skinning labels Sep 2, 2023

pull-request-size bot added the size/M label Sep 2, 2023

bdach reviewed Sep 4, 2023

View reviewed changes

bdach assigned frenzibyte Sep 4, 2023

peppy self-requested a review September 5, 2023 08:13

peppy added 2 commits September 5, 2023 18:01

Update GetTexture signature rather than creating new overload

96f12cf

Add size limitations for taiko drum rolls

9d17539

pull-request-size bot added size/L and removed size/M labels Sep 5, 2023

peppy self-assigned this Sep 5, 2023

peppy reviewed Sep 5, 2023

View reviewed changes

frenzibyte commented Sep 5, 2023

View reviewed changes

frenzibyte added 3 commits September 19, 2023 03:21

Revert "Update GetTexture signature rather than creating new overload"

57dc76b

This reverts commit 96f12cf.

Change extension from retrieval to post-processing instead

291a91b

Add basic test scene upscaling all skin elements

a373d71

frenzibyte requested a review from bdach September 19, 2023 02:22

peppy reviewed Sep 19, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

peppy reviewed Sep 19, 2023

View reviewed changes

frenzibyte marked this pull request as ready for review September 19, 2023 18:07

This comment was marked as resolved.

Sign in to view

peppy self-requested a review September 20, 2023 03:37

Simplify some maximum size specs

8e16b1d

peppy reviewed Sep 20, 2023

View reviewed changes

Rename parameter on LegacySpriteText to better imply the maximum si…

bd66285

…ze is per glyph

peppy reviewed Sep 20, 2023

View reviewed changes

Fix inspection in new test scene

1316403

peppy reviewed Sep 20, 2023

View reviewed changes

frenzibyte added 2 commits September 21, 2023 19:02

Use correct maximum size for droplets

c4fc419

Revert redundant size limitations

ad86bf2

Already handled by the sprites themselves being resized.

peppy self-requested a review September 26, 2023 07:32

Rename test scene, add xmldoc and increase scale factor to something …

b1561b6

…absurdly high

peppy approved these changes Sep 26, 2023

View reviewed changes

Merge branch 'master' into limit-gameplay-sprite-dimensions

990c545

peppy enabled auto-merge September 26, 2023 07:40

peppy merged commit 3b85a63 into ppy:master Sep 26, 2023
11 checks passed

This was referenced Sep 26, 2023

Adjust catch playfield and catcher layout to further match osu!(stable) #24895

Closed

Second pass on element sizing (specifically followpoint) #24940

Closed

frenzibyte mentioned this pull request Sep 29, 2023

Skin inconsistency between release and master #24973

Closed

bdach mentioned this pull request Oct 5, 2023

Adjust gameplay element maximum size limits for backwards compatibility #25023

Merged

frenzibyte deleted the limit-gameplay-sprite-dimensions branch October 10, 2023 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add maximum dimensions limit to skinnable gameplay elements #24706

Add maximum dimensions limit to skinnable gameplay elements #24706

frenzibyte commented Sep 2, 2023

bdach commented Sep 4, 2023

bdach Sep 4, 2023

peppy Sep 5, 2023

bdach Sep 5, 2023

frenzibyte Sep 5, 2023

bdach Sep 5, 2023 •

edited

frenzibyte Sep 5, 2023

bdach commented Sep 4, 2023

peppy commented Sep 5, 2023

peppy commented Sep 5, 2023

peppy commented Sep 5, 2023 •

edited

peppy left a comment

frenzibyte commented Sep 5, 2023 •

edited

frenzibyte Sep 5, 2023

frenzibyte Sep 5, 2023

frenzibyte commented Sep 5, 2023

peppy commented Sep 6, 2023

peppy Sep 19, 2023

frenzibyte Sep 19, 2023

This comment was marked as resolved.

peppy left a comment

This comment was marked as resolved.

peppy commented Sep 20, 2023

peppy Sep 20, 2023

frenzibyte Sep 20, 2023

peppy Sep 20, 2023

frenzibyte Sep 21, 2023 •

edited

peppy commented Sep 20, 2023

peppy Sep 20, 2023

frenzibyte Sep 20, 2023

peppy Sep 20, 2023

frenzibyte Sep 20, 2023

peppy commented Sep 26, 2023

peppy commented Sep 26, 2023

		// 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);

Add maximum dimensions limit to skinnable gameplay elements #24706

Add maximum dimensions limit to skinnable gameplay elements #24706

Conversation

frenzibyte commented Sep 2, 2023

bdach commented Sep 4, 2023

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 Sep 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach commented Sep 4, 2023

peppy commented Sep 5, 2023

peppy commented Sep 5, 2023

peppy commented Sep 5, 2023 • edited

peppy left a comment

Choose a reason for hiding this comment

frenzibyte commented Sep 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte commented Sep 5, 2023

peppy commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

peppy left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

peppy commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte Sep 21, 2023 • edited

Choose a reason for hiding this comment

peppy commented Sep 20, 2023

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

peppy commented Sep 26, 2023

peppy commented Sep 26, 2023

bdach Sep 5, 2023 •

edited

peppy commented Sep 5, 2023 •

edited

frenzibyte commented Sep 5, 2023 •

edited

frenzibyte Sep 21, 2023 •

edited