Skip to content

Conversation

@bdach
Copy link
Collaborator

@bdach bdach commented Jul 7, 2025

For future use.

Namely, this is storing the count to all possible relevant places: to realm database, to replays, and sending it to web (wherein it won't be used or stored without web-side changes). If that's overkill any of the last 3 commits here can be dropped, I've added it everywhere because @peppy's pitch for this was a bit vague.

@bdach bdach requested a review from peppy July 7, 2025 09:08
@bdach bdach self-assigned this Jul 7, 2025
@bdach bdach added area:online functionality Deals with online fetching / sending but don't change much on a surface UI level. area:replay area:database Deals with local realm database blocked/has realm migration Change involves a database migration and should be deployed across all release streams. labels Jul 7, 2025
Comment on lines 239 to 244
bool wasPaused = GameplayClockContainer.IsPaused.Value;

bool paused = base.Pause();

if (!wasPaused && paused)
Score.ScoreInfo.PauseCount++;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is required because in real-world testing I discovered that the count could be wrong if e.g. the game was paused and then the user tabbed out, thus activating the pause-on-focus-loss logic in

private void updatePauseOnFocusLostState()
{
if (!PauseOnFocusLost || !pausingSupportedByCurrentState || breakTracker.IsBreakTime.Value)
return;
if (gameActive.Value == false)
{
bool paused = Pause();
// if the initial pause could not be satisfied, the pause cooldown may be active.
// reschedule the pause attempt until it can be achieved.
if (!paused)
Scheduler.AddOnce(updatePauseOnFocusLostState);
}
}

In other words, Pause() can be called multiple times in succession and return true every time even if gameplay was already paused.

It's arguable that this should be changed and only the first call should return true but I didn't feel it wise to be changing that in this diff.

@smoogipoo
Copy link
Contributor

What exactly is the "future use" here?

@peppy
Copy link
Member

peppy commented Jul 7, 2025

The first step would just be to display the pause count on results screen (and potentially, leaderboards).

If we look back, there's been people asking for disallowing pausing (especially in the mania community, one example) for yonks. While I don't agree it should be disallowed, a starting point is transparency about whether a user is (ab)using pause during their plays.

Whether we use it further than that (ie disqualifying plays that pause more then n times from ranked) is up in the air. This is just a step forward in storing more metadata regarding how a user is playing the game. I hope we can store more metadata like this to scores going forward to make replays and spectating more immersive.

@smoogipoo
Copy link
Contributor

smoogipoo commented Jul 7, 2025

What I want to get at by asking that question is, is it enough to simply store a count, or do we need more data like a time range?

For example, if a future consideration is PP, is a pause during a break time to be considered the same as a pause during gameplay itself? This PR currently makes no such distinction.

If this is purely for the purpose of displaying pause count, then it's fine. But it can't meaningfully be used for anything else.

@stanriders
Copy link
Member

stanriders commented Jul 7, 2025

Accounting for pausing in pp would indeed require being able to distinguish mid-gameplay pauses and break time pauses, but I don't think we'd want to account for pauses in pp any time soon anyway so I don't think you should care too much about it

I would however say that having a break/gameplay flag would be useful for display purposes too because they don't really have the same weight

@tsunyoku
Copy link
Member

tsunyoku commented Jul 7, 2025

I don't see any world where accounting for pauses in PP would be viable to do fairly, no matter the format of the pause data. I'm also not really interested in it.

@peppy
Copy link
Member

peppy commented Jul 8, 2025

Storing timestamps would be interesting for display purposes, for sure. But I'm on the fence as to whether we should do it or not. Knowing whether a pause was in a break or not is also beneficial for understanding the nature of the pause. If we don't separate the counts out, it might be that we only want to count pauses outside of break time?

@bdach
Copy link
Collaborator Author

bdach commented Jul 8, 2025

For whatever it's worth I don't have a particularly strong opinion on how much data should be getting stored here.

From a pragmatic standpoint if there are concerns as to whether a plain count will be enough for unknown future use then I'd probably go for storing timestamps because that doesn't lose information.

@bdach
Copy link
Collaborator Author

bdach commented Jul 22, 2025

I've pushed a merge conflict resolution but am still awaiting feedback as to how much data should be getting stored here.

@peppy
Copy link
Member

peppy commented Jul 22, 2025

If it's not too much work, I'd say including timestamps would be hugely beneficial (stored as integer values is fine IMO). Being able to visualise this on a play graph would add a lot of insight into gameplay.

@bdach
Copy link
Collaborator Author

bdach commented Jul 22, 2025

Adjusted to store timestamps in d8900de.

bool paused = base.Pause();

if (!wasPaused && paused)
Score.ScoreInfo.Pauses.Add((int)Math.Round(GameplayClockContainer.CurrentTime));
Copy link
Member

Choose a reason for hiding this comment

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

There's argument that we may want some degree of debounce here for sanity (100 ms or something arbitrary). Any thoughts on that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure it's useful given the process of pausing itself has a debounce / cooldown mechanism built in already.

Copy link
Member

Choose a reason for hiding this comment

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

Fair, forgot about that.

Co-authored-by: Dean Herbert <[email protected]>
peppy
peppy previously approved these changes Jul 22, 2025
@peppy peppy enabled auto-merge July 22, 2025 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:database Deals with local realm database area:online functionality Deals with online fetching / sending but don't change much on a surface UI level. area:replay blocked/has realm migration Change involves a database migration and should be deployed across all release streams. size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants