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.

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

Adjust taiko (again) to limit minimum aspect ratio to 5:4 #22581

Closed
wants to merge 2 commits into from

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Feb 9, 2023

Anything lower than this looks stupid and broken. This should not affect any normal player. If it does, they should be using a "lane cover" mod instead. Or classic mod.

Why change this for a third or fourth time? Because to a normal user the current behaviour would look like a bug. This should be a middle ground which I hope everyone can accept.

@bdach
Copy link
Collaborator

bdach commented Feb 9, 2023

The proportions look better, but the hitobjects appear to be going hyperspeed on very narrow windows:

2023-02-09.22-09-25.mp4

I'm not sure if this is expected or not? Logically I'd say it shouldn't be happening but I don't even know anymore with all this jankage.

@peppy
Copy link
Sponsor Member Author

peppy commented Feb 10, 2023

That is indeed weird and I have no immediate idea why it would happen.

@bdach
Copy link
Collaborator

bdach commented Feb 10, 2023

I was suspecting that it's due to something to do with #22325. That said I was a bit too knackered yesterday to confirm that it's that and figure out what would be the proper thing to do there to not have this happen.

@peppy
Copy link
Sponsor Member Author

peppy commented Feb 10, 2023

You're probably right. I thought what I had felt too simple for the amount of changes which were done over the last few releases. I'll take a look today regardless.

@peppy
Copy link
Sponsor Member Author

peppy commented Feb 10, 2023

I've applied a fix which seems to work as intended.. as far as my brain can tell.

// This is still a bit weird, because readability changes with window size, but it is what it is.
if (LockPlayfieldMaxAspect.Value && Parent.ChildSize.X / Parent.ChildSize.Y > default_aspect)
height *= Math.Clamp(Parent.ChildSize.X / Parent.ChildSize.Y, 0.4f, 4) / default_aspect;
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this clamp breaks things on wide aspect ratios, the playfield seemingly gets unloaded?

clamp.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

that looks cropped out by masking moreso than unloaded

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct, this is a masking/presence concern. Doesn't change the fact that it probably needs to be addressed somehow.

@peppy
Copy link
Sponsor Member Author

peppy commented Feb 13, 2023

I'm not sure anymore. I tried applying the clamping back but the numbers no longer match. Also making the window short seems to make notes travel slower again expectations. This is cursed.

@sw1tchbl4d3r
Copy link
Contributor

sw1tchbl4d3r commented Feb 13, 2023

I've tried to solve this as well, and ended up with sw1tchbl4d3r@c82b74a this crude code right here, which does work, but I will be honest, I am not quite sure why (mostly because I am not too well versed in the graphics process yet, and have no idea how the aspect ratios and sizes relate to each other).
This whole thing makes me feel like clamping the ratio to avoid the masking error is the wrong approach, as the code above has the same issue lazer has had for a pretty long time, that being the notes getting smaller and going hyperspeed on super wide aspect ratios due to the aspect ratio being clamped to 0.4 - 4 in this weird way...

@bdach
Copy link
Collaborator

bdach commented Feb 13, 2023

I stared at this a bit today and yep, that clamp is pretty cursed. I may have something to offer as a fix, though - namely changing the layouting in TaikoPlayfieldAdjustmentContainer to... not move contents off screen, like so:

diff --git a/osu.Game.Rulesets.Taiko/UI/TaikoPlayfieldAdjustmentContainer.cs b/osu.Game.Rulesets.Taiko/UI/TaikoPlayfieldAdjustmentContainer.cs
index e97bb195d2..c7f8fbf5c8 100644
--- a/osu.Game.Rulesets.Taiko/UI/TaikoPlayfieldAdjustmentContainer.cs
+++ b/osu.Game.Rulesets.Taiko/UI/TaikoPlayfieldAdjustmentContainer.cs
@@ -1,6 +1,7 @@
 // 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.
 
+using System;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Game.Rulesets.UI;
@@ -37,9 +38,12 @@ protected override void Update()
                     height *= currentAspect / MINIMUM_ASPECT;
             }
 
+            // Limit the maximum relative height of the playfield to one-third of available area to avoid it masking out on extreme resolutions.
+            height = Math.Min(height, 1f / 3f);
             Height = height;
 
-            // Position the taiko playfield exactly one playfield from the top of the screen.
+            // Position the taiko playfield exactly one playfield from the top of the screen, if there is enough space for it.
+            // Note that the relative height cannot exceed one-third - if that limit is hit, the playfield will be exactly centered.
             RelativePositionAxes = Axes.Y;
             Y = height;
         }

In action:

2023-02-13.21-36-53.mp4

Thoughts? (Disclaimer: I did not yet compare against master, just fiddled around until I arrived at something that looked not-broken.)

@sw1tchbl4d3r
Copy link
Contributor

The only direct problem I can see in that video is more notes becoming visible on wider aspect ratios than 16:9 without the classic mod, though that's something to be fixed in the timerange calculation.

@peppy
Copy link
Sponsor Member Author

peppy commented Feb 15, 2023

That sounds about right. Outside of the min/max aspect ratios the time range needs to be fixed rather than flexible. ie. how it was before all of these recent changes were applied.

@sw1tchbl4d3r
Copy link
Contributor

sw1tchbl4d3r commented Apr 26, 2023

I think I've found a pretty good solution to this which seems pretty logical, mind if I open a separate PR for it? (@peppy)
It incorporates the comments here, and fixes a flaw in the scroll speed calculation which lead to the problems on wider ratios. (https://github.com/sw1tchbl4d3r/osu/tree/taiko_aspect_limit)

@peppy
Copy link
Sponsor Member Author

peppy commented Apr 27, 2023

@sw1tchbl4d3r please do, yes. i haven't had a chance to revisit this and even when i do, i know it's going to take a lot of trial and error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants