Avoid updates (and update notifications) from appearing in more gameplay cases #30073

peppy · 2024-10-01T08:56:27Z

Second attempt at #29657 taking on feedback from #29657 (comment).

RFC, haven't tested update flow yet.

…/break state

peppy · 2024-10-01T08:58:51Z

osu.Game/OsuGame.cs

+                BeatmapManager.PauseImports = p.NewValue != LocalUserPlayingStates.NotPlaying;
+                SkinManager.PauseImports = p.NewValue != LocalUserPlayingStates.NotPlaying;
+                ScoreManager.PauseImports = p.NewValue != LocalUserPlayingStates.NotPlaying;


Note that this stops imports from running during break time and on the fail screen, which is an extra win.

bdach

just a cursory read of structure, haven't examined the logic in detail yet (or tested)

bdach · 2024-10-01T10:10:09Z

osu.Game/Screens/Play/LocalUserPlayingStates.cs

+
+namespace osu.Game.Screens.Play
+{
+    public enum LocalUserPlayingStates


Bit weird for this enum name to be in plural, I'd normally expect LocalUserPlayingState...?

I originally had it plural to avoid the classic namespace conflict with field (was called PlayingState). Can go with non-plural not, sure.

bdach · 2024-10-01T10:30:58Z

osu.Game/Screens/Play/Player.cs

@@ -94,6 +94,7 @@ public abstract partial class Player : ScreenWithBeatmapBackground, ISamplePlayb
        public IBindable<bool> LocalUserPlaying => localUserPlaying;


I'd argue this should no longer be public (or even exist).

Yeah, it's gonna touch a lot of tests though (take a quick look and see if you thing it's worth it)

As far as I can see the extent of it is mostly asserts, right? If that is accurate then I think it's worth getting rid just to lower the redundancy around here. I'll have to cross-check all of the existing conditional changes anyway to make sure nothing slipped by so that seems like a rather minor inconvenience from both authoring and reviewing angles.

bdach · 2024-10-01T10:33:07Z

osu.Game/OsuGame.cs

+                // For scores, we need to allow imports during "Break" state else local user's scores will never be imported.
+                ScoreManager.PauseImports = p.NewValue == LocalUserPlayingStates.Playing;


This is weird, can the state not transition to NotPlaying on gameplay completion or something?

Can try and make that happen sure.

bdach · 2024-10-01T10:58:01Z

osu.Game/Screens/Play/Player.cs

@@ -508,7 +508,7 @@ private void onBreakTimeChanged(ValueChangedEvent<bool> isBreakTime)

        private void updateGameplayState()
        {
-            bool inGameplay = !DrawableRuleset.HasReplayLoaded.Value;
+            bool inGameplay = !DrawableRuleset.HasReplayLoaded.Value && !GameplayState.HasPassed;
            bool inBreak = breakTracker.IsBreakTime.Value || DrawableRuleset.IsPaused.Value || GameplayState.HasFailed;


Hmm with failing being considered as "break" I'm not sure the ScoreManager.PauseImports change is entirely correct. I'd say any sort of gameplay completion should transition the state to NotPlaying...?

The reason I didn't originally add failing is that I thought we'd want things like imports to stay blocked while at the fail screen (else they may continue into the start of the next play). I think it's also quite crippling when imports run during HighPerformanceSession. But maybe this is an edge case we just allow to happen for now?

we'd want things like imports to stay blocked while at the fail screen

Except for the part where you can request the failed play to be imported at the fail screen which will presumably deadlock now.

Yeah, probably need a test and just doing as you say for now.

We had a test covering this but it wasn't within `OsuGame` so didn't have full import blocking coverage (see https://github.com/ppy/osu/blob/cbbe2f9dc04abb052a0d762aa798eba094fdd80c/osu.Game.Tests/Visual/Gameplay/TestScenePlayerLocalScoreImport.cs#L89-L88).

bdach · 2024-10-04T06:47:06Z

Not sure of the status or urgency of this, are you looking to still check off the todo item in the OP or should I consider doing so part of review?

peppy · 2024-10-04T08:09:57Z

I'll test this now, forgot.

peppy · 2024-10-04T08:47:19Z

In testing it looks to work as expected.

Tested using inserted Task.Delay() to make sure I had enough time to get to gameplay. If you test, it's as simple as running dotnet run on osu-deploy and "installing" the game (however that is required by your testing platform).

We have downgrades enabled so it will detect the last published version and start to download / update to it.

smoogipoo · 2024-10-07T07:31:49Z

osu.Android/GameplayScreenRotationLocker.cs

-        private Bindable<bool> localUserPlaying = null!;
+        private IBindable<LocalUserPlayingStates> localUserPlaying = null!;


What happened here? The class doesn't exist? See android build failure.

Probably something something missed in the rename because osu.Desktop.slnf.

Suggested change

private Bindable<bool> localUserPlaying = null!;

private IBindable<LocalUserPlayingStates> localUserPlaying = null!;

private Bindable<bool> localUserPlaying = null!;

private IBindable<LocalUserPlayingState> localUserPlaying = null!;

probably fixes.

At this point the update is already started in the background but I guess we can still block the notification from interrupting the user.

smoogipoo · 2024-10-07T08:18:32Z

From a structural perspective, I really really don't like this one.

We have way too many states flying around right now:

OsuScreen.OverlayActivationMode
OsuScreen.Activity
High performance session.
... and now yet another bindable that is messaging very similar things in a slightly different manner.

Resulting in a 19 file diff that is pretty rough to review.

Not mentioned here: what is a "local user playing state"? why is it not a "gameplay state"? we already have a GameplayState why can't that be used? do we really need to wrap one bindable in an interface like this?

While I'm going to grit my teeth and continue powering through this one given that it's the second attempt, in the future I suggest we use SessionStatics for this.

A `false` value marks the user as being on the latest release, and notifies them as such when clicking the button in settings. In reality, we don't know whether this is the case yet - we're just deferring the check. Somewhat minor change because the chance of a user manually going into settings and clicking the button is very small.

smoogipoo · 2024-10-07T10:54:14Z

Have made a few changes here, please check the last few commits @peppy

peppy · 2024-10-07T17:20:36Z

osu.Game/Input/OsuUserInputManager.cs

@@ -10,7 +10,7 @@ namespace osu.Game.Input
 {
    public partial class OsuUserInputManager : UserInputManager
    {
-        protected override bool AllowRightClickFromLongTouch => PlayingState.Value == LocalUserPlayingState.NotPlaying;
+        protected override bool AllowRightClickFromLongTouch => PlayingState.Value != LocalUserPlayingState.Playing;


Arguable. I set this to avoid the silly hold ring from showing during breaks. And there's nothing at pause / fail that needs right click.

But if you feel strong about this then I'm fine with this change I guess.

Prefer review comments for stuff like this instead

peppy added 3 commits October 1, 2024 17:53

Adjust ILocalUserPlayInfo to expose whether gameplay is in a paused…

ad3007e

…/break state

Make states better defined

3d54f4a

Avoid updates and update notifications appearing in more gameplay cases

4b1c2c0

peppy added the area:gameplay label Oct 1, 2024

pull-request-size bot added the size/L label Oct 1, 2024

peppy commented Oct 1, 2024

View reviewed changes

peppy added 2 commits October 1, 2024 18:21

Fix local scores never importing due to new conditionals

8f0fedb

Attempt to fix android class

7977ce8

bdach reviewed Oct 1, 2024

View reviewed changes

peppy added 2 commits October 1, 2024 19:55

Passing means NotPlaying

8dba4cf

Rename enum to non-plural now that it won't conflict

8773c34

bdach reviewed Oct 1, 2024

View reviewed changes

peppy added 2 commits October 3, 2024 17:11

Change failed states to be considered as NotPlaying

24d6ea5

peppy added next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! and removed next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! labels Oct 4, 2024

smoogipoo requested changes Oct 7, 2024

View reviewed changes

Fix android build failure due to enum rename

a41c6dc

smoogipoo self-requested a review October 7, 2024 07:45

peppy added 2 commits October 7, 2024 17:04

Remove unnecessary runOutsideOfGameplay call

1f45b21

Add second call of runOutsideGameplay on update progress notification

38ee824

At this point the update is already started in the background but I guess we can still block the notification from interrupting the user.

smoogipoo added 4 commits October 7, 2024 17:41

Merge branch 'master' into updates-outside-of-gameplay-only-2

8dece70

Remove unrelated test

19b586e

Refactoring for correctness

6e659e1

Fix incorrect condition

aee5f0e

smoogipoo previously approved these changes Oct 7, 2024

View reviewed changes

smoogipoo added 2 commits October 7, 2024 19:35

Remove unused parameter

c3f2c82

smoogipoo dismissed their stale review via c3f2c82 October 7, 2024 10:50

smoogipoo approved these changes Oct 7, 2024

View reviewed changes

peppy commented Oct 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid updates (and update notifications) from appearing in more gameplay cases #30073

Avoid updates (and update notifications) from appearing in more gameplay cases #30073

peppy commented Oct 1, 2024 •

edited

Loading

peppy Oct 1, 2024

bdach left a comment •

edited

Loading

bdach Oct 1, 2024

peppy Oct 1, 2024

bdach Oct 1, 2024

peppy Oct 1, 2024

bdach Oct 1, 2024

bdach Oct 1, 2024

peppy Oct 1, 2024

bdach Oct 1, 2024

peppy Oct 2, 2024

bdach Oct 3, 2024

peppy Oct 3, 2024

bdach commented Oct 4, 2024

peppy commented Oct 4, 2024

peppy commented Oct 4, 2024

smoogipoo Oct 7, 2024 •

edited

Loading

bdach Oct 7, 2024

smoogipoo commented Oct 7, 2024

smoogipoo commented Oct 7, 2024

peppy Oct 7, 2024

		@@ -94,6 +94,7 @@ public abstract partial class Player : ScreenWithBeatmapBackground, ISamplePlayb
		public IBindable<bool> LocalUserPlaying => localUserPlaying;

		// For scores, we need to allow imports during "Break" state else local user's scores will never be imported.
		ScoreManager.PauseImports = p.NewValue == LocalUserPlayingStates.Playing;

		private Bindable<bool> localUserPlaying = null!;
		private IBindable<LocalUserPlayingStates> localUserPlaying = null!;

Avoid updates (and update notifications) from appearing in more gameplay cases #30073

Are you sure you want to change the base?

Avoid updates (and update notifications) from appearing in more gameplay cases #30073

Conversation

peppy commented Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

bdach left a comment • edited Loading

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

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

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 commented Oct 4, 2024

peppy commented Oct 4, 2024

peppy commented Oct 4, 2024

smoogipoo Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo commented Oct 7, 2024

smoogipoo commented Oct 7, 2024

Choose a reason for hiding this comment

peppy commented Oct 1, 2024 •

edited

Loading

bdach left a comment •

edited

Loading

smoogipoo Oct 7, 2024 •

edited

Loading