-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Improve osu!mania playability on mobile devices #31368
base: master
Are you sure you want to change the base?
Conversation
This improves display in portrait screen, where the stage is scaled up.
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.
Haven't tested android yet. Do you have a set of tests that should be done? E.g. open screen X in landscape then rotate to portrait, is should/shouldn't rotate.
CI failures need attention.
base.Update(); | ||
|
||
float aspectRatio = DrawWidth / DrawHeight; | ||
bool isPortrait = aspectRatio < 4 / 3f; |
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.
Should use osu.Framework.Utils.Precision
, or just compare with 1f
. Otherwise a 4:3 tablet in landscape might be considered portrait by float imprecision.
Another reason to use 1f
is that older 1280x1024 displays (5:4) are properly considered as landscape.
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.
This didn't matter at first because I didn't want to apply a general upscale of the playfield, but after further considerations I've changed it as such. I'll change the boolean to check against 1f
instead.
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.
Tests I've done:
|
I get this behavior on my Xiaomi Pad out.mp4 |
@PercyDan54 Cannot reproduce on my iPad. Try debugging through the |
It's not only when pausing the game once. If I put my tablet in Landscape it immediately enters that broken state when gameplay starts. The Orientation in gameplay is |
I had a quick look in Android docs and seems the game is put into Letterboxing mode https://developer.android.com/guide/topics/manifest/activity-element#screen
|
Sounds like it's working correctly then. |
I guess my case might fall into the "device manufacturers configured". Imo it's better to test on more tablets to know whether most manufacturers do this, since it's a bit counter-intuitve |
Similar to mania's `ArgonJudgementPiece`.
I'm not sure this approach is going to work well. My original thoughts were that we don't change the OS-level orientation locking and just adjust the ruleset's playfield to render according to a user's preference. As per the reported issues above, I have no idea how solid mobile APIs are to have an application decide whether the orientation should be switched on the fly at an OS level, disregarding how the user is holding the device. |
I have considered make the playfield render in a portrait-like mode without changing the OS orientation, but there are multiple things to take into consideration when doing this.
For the reasons above, I thought I can change the orientation at an OS level and tick all the three points above without fuss. UIKit define explicit API to let the device know that the orientation needs to be updated, see I'm not sure what are the conditions for making Android switch to letterbox mode as in the reported issue above. Need someone that can investigate that and see how bad it is and/or if letterboxing can be avoided. |
I (and the mentioned android xiaomi tablet) disagree with this. There is no good reason to force portrait orientation on tablets. People may want to play osu!mania with a keyboard, and they will usually want to have their tablet in landscape mode. |
Landscape orientation for mania on tablets can be brought back. It felt unplayable at first with the idea of "touchable columns" and no touch input overlay, but upon testing again it seems roughly fine. However I find it slightly weird to allow landscape orientation when portrait orientation works and works much better than landscape. I would rather recommend tablet users to always be on portrait if they intend to play mania. @peppy thoughts? |
|
- Caches `DrawableRuleset` in editor compose screen for mania playfield adjustment container (because it's used to wrap the blueprint container as well) - Fixes `ManiaModWithPlayfieldCover` performing a no-longer-correct direct cast with a naive-but-working approach.
On a read of the code and a test of how this looks in portrait (on PC, but that counts right?) I'm pretty on board with this direction. e5713e5 caught me off-guard – is this something you could PR separately along with a visual explanation? I think bundling it with this PR makes things a bit hard to swallow. |
@Susko3 are you able to give this a go/no-go for android (remaining blocking bullet point)? |
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.
Initial pass, no major issues.
It's good from my side. I've tested as set out in #31368 (comment). I've only tested on a phone, but I've also simulated a tablet by forcing Tagging @PercyDan54 to give it a test on a real tablet, as things are updated and should work better. |
Co-authored-by: Susko3 <[email protected]>
I'll say this is the expectation of attempting to play on landscape, it won't always feel good. Switch to portrait for better experience. Allowing landscape was a choice made few comments above yours #31368 (comment).
This is to do with the playfield not being updated while the game is paused, it is out of scope in this PR and most relevant to #7285 |
Alright. |
Given the above responses, I'm going to do a code-level review pass this week with the intention of getting it in 👍 . |
This reverts commit e5713e5.
I would have to specifically revert changes to the This is happening because in 1e08b3d the anchoring of the judgement container has changed so that judgements get moved upwards proportional to the upscaling factor of the playfield. And due to that change, the positioning of triangles skin mania judgements is completely broken as they're used to display close to the top of the playfield. Therefore both the Before: CleanShot.2025-01-21.at.05.10.08.mp4After: CleanShot.2025-01-21.at.05.10.51.mp4 |
This reverts commit 6ec7183.
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.
Just a code level check
using osu.Framework.Graphics; | ||
using osu.Game.Screens.Play; | ||
|
||
namespace osu.Game.Mobile |
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.
I'm not digging this namespace after we've come this far without avoiding such pollution.
Could you try an implementation where you just read the portrait requirement from either
((IOsuScreen)ScreenStack.CurrentScreen).RequiresPortraitOrientation
inUpdate
- (probably better) expose
screenChanged
and override in mobile projects:
diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs
index 93918e4541..419944f4f1 100644
--- a/osu.Game/OsuGame.cs
+++ b/osu.Game/OsuGame.cs
@@ -1551,7 +1551,7 @@ protected override void UpdateAfterChildren()
GlobalCursorDisplay.ShowCursor = (ScreenStack.CurrentScreen as IOsuScreen)?.CursorVisible ?? false;
}
- private void screenChanged(IScreen current, IScreen newScreen)
+ protected virtual void ScreenChanged(IOsuScreen current, IOsuScreen newScreen)
{
SentrySdk.ConfigureScope(scope =>
{
@@ -1595,32 +1595,35 @@ private void screenChanged(IScreen current, IScreen newScreen)
break;
}
- if (current is IOsuScreen currentOsuScreen)
+ if (current != null)
{
- backButtonVisibility.UnbindFrom(currentOsuScreen.BackButtonVisibility);
- OverlayActivationMode.UnbindFrom(currentOsuScreen.OverlayActivationMode);
- API.Activity.UnbindFrom(currentOsuScreen.Activity);
+ // Unbind from previous screen.
+ backButtonVisibility.UnbindFrom(current.BackButtonVisibility);
+ OverlayActivationMode.UnbindFrom(current.OverlayActivationMode);
+ API.Activity.UnbindFrom(current.Activity);
}
- if (newScreen is IOsuScreen newOsuScreen)
+ // Bind to new screen.
+ if (newScreen != null)
{
- backButtonVisibility.BindTo(newOsuScreen.BackButtonVisibility);
- OverlayActivationMode.BindTo(newOsuScreen.OverlayActivationMode);
- API.Activity.BindTo(newOsuScreen.Activity);
+ backButtonVisibility.BindTo(newScreen.BackButtonVisibility);
+ OverlayActivationMode.BindTo(newScreen.OverlayActivationMode);
+ API.Activity.BindTo(newScreen.Activity);
- GlobalCursorDisplay.MenuCursor.HideCursorOnNonMouseInput = newOsuScreen.HideMenuCursorOnNonMouseInput;
+ // Handle various configuration updates based on new screen settings.
+ GlobalCursorDisplay.MenuCursor.HideCursorOnNonMouseInput = newScreen.HideMenuCursorOnNonMouseInput;
- requiresPortraitOrientation.Value = newOsuScreen.RequiresPortraitOrientation;
+ requiresPortraitOrientation.Value = newScreen.RequiresPortraitOrientation;
- if (newOsuScreen.HideOverlaysOnEnter)
+ if (newScreen.HideOverlaysOnEnter)
CloseAllOverlays();
else
Toolbar.Show();
- if (newOsuScreen.ShowFooter)
+ if (newScreen.ShowFooter)
{
BackButton.Hide();
- ScreenFooter.SetButtons(newOsuScreen.CreateFooterButtons());
+ ScreenFooter.SetButtons(newScreen.CreateFooterButtons());
ScreenFooter.Show();
}
else
@@ -1628,16 +1631,16 @@ private void screenChanged(IScreen current, IScreen newScreen)
ScreenFooter.SetButtons(Array.Empty<ScreenFooterButton>());
ScreenFooter.Hide();
}
- }
- skinEditor.SetTarget((OsuScreen)newScreen);
+ skinEditor.SetTarget((OsuScreen)newScreen);
+ }
}
- private void screenPushed(IScreen lastScreen, IScreen newScreen) => screenChanged(lastScreen, newScreen);
+ private void screenPushed(IScreen lastScreen, IScreen newScreen) => ScreenChanged((OsuScreen)lastScreen, (OsuScreen)newScreen);
private void screenExited(IScreen lastScreen, IScreen newScreen)
{
- screenChanged(lastScreen, newScreen);
+ ScreenChanged((OsuScreen)lastScreen, (OsuScreen)newScreen);
if (newScreen == null)
Exit();
in the individual mobile projects?
for any required shared logic, you could make an osu.Game.Utils.OrientationInfo
(similar to BatteryInfo
) or if that's not required, a static helper class in the same namespace?
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.
Good call, sounds reasonable. I've removed OrientationManager
entirely and moved any shared logic to a MobileUtils
static class (name is arbitrary, can be changed, can be renamed to OrientationUtils
/shrug). c18128e
InternalChild = child; | ||
} | ||
|
||
internal void Add(Drawable drawable) |
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.
Adding things to a SkinReloadableDrawable
from external sounds like it will fail on skin reload. Does it not?
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.
Technically not, since all that SkinReloadableDrawable
does is trigger an event when the selected skin is changed. Now that I look at it, I can totally avoid this inheritance by just...resolving the skin source and subscribing to skin change...
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.
} | ||
|
||
[Resolved] | ||
private DrawableRuleset drawableRuleset { get; set; } = 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.
Let's not:
diff --git a/osu.Game.Rulesets.Mania/UI/DrawableManiaRuleset.cs b/osu.Game.Rulesets.Mania/UI/DrawableManiaRuleset.cs
index a186d9aa7d..e33cf092c3 100644
--- a/osu.Game.Rulesets.Mania/UI/DrawableManiaRuleset.cs
+++ b/osu.Game.Rulesets.Mania/UI/DrawableManiaRuleset.cs
@@ -161,7 +161,7 @@ private void updateTimeRange()
/// <returns>The scroll time.</returns>
public static double ComputeScrollTime(double scrollSpeed) => MAX_TIME_RANGE / scrollSpeed;
- public override PlayfieldAdjustmentContainer CreatePlayfieldAdjustmentContainer() => new ManiaPlayfieldAdjustmentContainer();
+ public override PlayfieldAdjustmentContainer CreatePlayfieldAdjustmentContainer() => new ManiaPlayfieldAdjustmentContainer(this);
protected override Playfield CreatePlayfield() => new ManiaPlayfield(Beatmap.Stages);
diff --git a/osu.Game.Rulesets.Mania/UI/ManiaPlayfieldAdjustmentContainer.cs b/osu.Game.Rulesets.Mania/UI/ManiaPlayfieldAdjustmentContainer.cs
index b0203643b0..feb75b9f1e 100644
--- a/osu.Game.Rulesets.Mania/UI/ManiaPlayfieldAdjustmentContainer.cs
+++ b/osu.Game.Rulesets.Mania/UI/ManiaPlayfieldAdjustmentContainer.cs
@@ -2,7 +2,6 @@
// See the LICENCE file in the repository root for full licence text.
using System;
-using osu.Framework.Allocation;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
using osu.Game.Rulesets.UI;
@@ -16,8 +15,11 @@ public partial class ManiaPlayfieldAdjustmentContainer : PlayfieldAdjustmentCont
private readonly DrawSizePreservingFillContainer scalingContainer;
- public ManiaPlayfieldAdjustmentContainer()
+ private readonly DrawableManiaRuleset drawableManiaRuleset;
+
+ public ManiaPlayfieldAdjustmentContainer(DrawableManiaRuleset drawableManiaRuleset)
{
+ this.drawableManiaRuleset = drawableManiaRuleset;
InternalChild = scalingContainer = new DrawSizePreservingFillContainer
{
Anchor = Anchor.Centre,
@@ -30,9 +32,6 @@ public ManiaPlayfieldAdjustmentContainer()
};
}
- [Resolved]
- private DrawableRuleset drawableRuleset { get; set; } = null!;
-
protected override void Update()
{
base.Update();
@@ -40,8 +39,6 @@ protected override void Update()
float aspectRatio = DrawWidth / DrawHeight;
bool isPortrait = aspectRatio < 1f;
- var drawableManiaRuleset = (DrawableManiaRuleset)drawableRuleset;
-
if (isPortrait && drawableManiaRuleset.Beatmap.Stages.Count == 1)
{
// Scale playfield up by 25% to become playable on mobile devices,
diff --git a/osu.Game/Rulesets/Edit/DrawableEditorRulesetWrapper.cs b/osu.Game/Rulesets/Edit/DrawableEditorRulesetWrapper.cs
index 573eb8c42f..174b278d89 100644
--- a/osu.Game/Rulesets/Edit/DrawableEditorRulesetWrapper.cs
+++ b/osu.Game/Rulesets/Edit/DrawableEditorRulesetWrapper.cs
@@ -19,16 +19,16 @@ namespace osu.Game.Rulesets.Edit
internal partial class DrawableEditorRulesetWrapper<TObject> : CompositeDrawable
where TObject : HitObject
{
- public Playfield Playfield => DrawableRuleset.Playfield;
+ public Playfield Playfield => drawableRuleset.Playfield;
- public readonly DrawableRuleset<TObject> DrawableRuleset;
+ private readonly DrawableRuleset<TObject> drawableRuleset;
[Resolved]
private EditorBeatmap beatmap { get; set; } = null!;
public DrawableEditorRulesetWrapper(DrawableRuleset<TObject> drawableRuleset)
{
- DrawableRuleset = drawableRuleset;
+ this.drawableRuleset = drawableRuleset;
RelativeSizeAxes = Axes.Both;
@@ -38,7 +38,7 @@ public DrawableEditorRulesetWrapper(DrawableRuleset<TObject> drawableRuleset)
[BackgroundDependencyLoader]
private void load()
{
- DrawableRuleset.FrameStablePlayback = false;
+ drawableRuleset.FrameStablePlayback = false;
Playfield.DisplayJudgements.Value = false;
}
@@ -67,27 +67,27 @@ protected override void LoadComplete()
private void regenerateAutoplay()
{
- var autoplayMod = DrawableRuleset.Mods.OfType<ModAutoplay>().Single();
- DrawableRuleset.SetReplayScore(autoplayMod.CreateScoreFromReplayData(DrawableRuleset.Beatmap, DrawableRuleset.Mods));
+ var autoplayMod = drawableRuleset.Mods.OfType<ModAutoplay>().Single();
+ drawableRuleset.SetReplayScore(autoplayMod.CreateScoreFromReplayData(drawableRuleset.Beatmap, drawableRuleset.Mods));
}
private void addHitObject(HitObject hitObject)
{
- DrawableRuleset.AddHitObject((TObject)hitObject);
- DrawableRuleset.Playfield.PostProcess();
+ drawableRuleset.AddHitObject((TObject)hitObject);
+ drawableRuleset.Playfield.PostProcess();
}
private void removeHitObject(HitObject hitObject)
{
- DrawableRuleset.RemoveHitObject((TObject)hitObject);
- DrawableRuleset.Playfield.PostProcess();
+ drawableRuleset.RemoveHitObject((TObject)hitObject);
+ drawableRuleset.Playfield.PostProcess();
}
public override bool PropagatePositionalInputSubTree => false;
public override bool PropagateNonPositionalInputSubTree => false;
- public PlayfieldAdjustmentContainer CreatePlayfieldAdjustmentContainer() => DrawableRuleset.CreatePlayfieldAdjustmentContainer();
+ public PlayfieldAdjustmentContainer CreatePlayfieldAdjustmentContainer() => drawableRuleset.CreatePlayfieldAdjustmentContainer();
protected override void Dispose(bool isDisposing)
{
diff --git a/osu.Game/Rulesets/Edit/HitObjectComposer.cs b/osu.Game/Rulesets/Edit/HitObjectComposer.cs
index 8882d55b42..15b60114af 100644
--- a/osu.Game/Rulesets/Edit/HitObjectComposer.cs
+++ b/osu.Game/Rulesets/Edit/HitObjectComposer.cs
@@ -133,7 +133,6 @@ private void load(OsuConfigManager config, [CanBeNull] Editor editor)
if (DrawableRuleset is IDrawableScrollingRuleset scrollingRuleset)
dependencies.CacheAs(scrollingRuleset.ScrollingInfo);
- dependencies.CacheAs<DrawableRuleset>(drawableRulesetWrapper.DrawableRuleset);
dependencies.CacheAs(Playfield);
InternalChildren = new[]
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.
Co-authored-by: Dean Herbert <[email protected]>
Everyone is right, too much inheritance and polymorphism backfires very badly.
Co-authored-by: Dean Herbert <[email protected]>
AndroidOrientationManager
works as it should (see Improve osu!mania playability on mobile devices #31368 (comment))RFC. This is quite a packed PR, but the changes made here are closely tied to each other and cannot be separated as they cannot be merged / reviewed on their own (except for maybe the mania judgement positioning changes).
The aim of this PR is to lay down a path forward for supporting osu!mania gameplay on mobile devices and discuss the approach taken here and whether it's good / ethical / does not violate any existing gameplay rules or mechanics.
Preview of final product:
IMG_1845-converted.MOV
IMG_0460.mov
Allow locking orientation on iOS in certain circumstances
Adds support for making
OsuScreen
s able to force the device to a "portrait" orientation, to be used for mania-basedPlayer
screen sessions.Improve osu!mania gameplay scaling on portrait orientation
The main change of this PR. Naturally the entire game scene is surrounded by a scaling container that enforces the game to fit to the window size in a 4:3 aspect ratio. Since osu!mania is generally vertically based, this scaling behaviour is completely unfriendly on screens in portrait orientations.
To improve on this, I've added scaling logic in
ManiaPlayfieldAdjustmentContainer
that scales up the osu!mania stage just enough to be playable with a touchscreen on phones and tablet devices, and try to accommodate as much horizontal space as possible before scaling down on high key numbers / wide columnsBack-to-back comparison between master and this PR:
CleanShot.2024-12-31.at.12.30.37.mp4
Make mania judgements relative to the hit target position
Following the commit above, since the playfield can be roughly scaled up to 125% its normal size, the judgements shift position, and they do so incorrectly, shifting down to below the hit target and look broken. This commit changes the anchoring of the judgements so they're tied to the hit target, so that when the playfield is scaled up, the judgements shift to the top rather than to the bottom.
Before:
CleanShot.2024-12-31.at.12.36.14.mp4
After:
CleanShot.2024-12-31.at.12.37.22.mp4
Replace
ManiaTouchInputArea
with touchable columnsAlso following the scaling changes, which ensure that the playfield is scaled enough for the player to hit the notes with their fingers, I cannot find any use for
ManiaTouchInputArea
anymore, so I chose to remove it completely. This may be debatable, but I'm not sure refactoring it would make any sense and it's adding very bad visual clutter when displayed on top of the columns (it looks mispositioned from each column and basically broken to the user).Due to this, I've looked into applying the portrait orientation changes to Android as well so osu!mania doesn't become broken there.