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

Fix score processor no longer applying results when failing in multiplayer match #26751

Merged
merged 5 commits into from
Jan 29, 2024

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Jan 27, 2024

This happens because ScoreProcessor completely stops accepts any judgement when health goes to zero (determined by HealthProcessor.HasFailed). As far as I understand, the intention of this behaviour is to match stable and also because it's simply logical (score shouldn't increase when player fails).

Testing wise, the logic of ensuring that normal player instances stop applying results after reaching a fail state is already covered by TestSceneFailJudgement, but I've added another test case specific to MultiplayerPlayer ensuring that results still affect score after reaching a fail state.

/// <remarks>
/// In multiplayer, this is never set to <c>true</c> even if the player reached zero health, due to <see cref="PlayerConfiguration.AllowFailAnimation"/> being turned off.
/// </remarks>
public bool ShownFailAnimation { get; set; }
Copy link
Collaborator

@bdach bdach Jan 27, 2024

Choose a reason for hiding this comment

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

I disagree so much with this change and everything it implies.

Any gameplay logic being conditional on "being shown the fail animation" is completely and utterly wrong.

This feels like going in a completely off-base direction. And I think the fix to this entire issue should be local to the multiplayer flow, because it's the one stretching the definition of "fail" to begin with.

Copy link
Member Author

Choose a reason for hiding this comment

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

I approached this initially with the thought that Player has two separate states of failing, each being:

  1. marking score as failed / F rank (determined by HealthProcessor.HasFailed)
  2. presenting fail animation and stopping gameplay (determined by GameplayState.HasFailed, never set to true in multiplayer since Fix multiplayer scores being submitted as pass even if failed #26384).

Therefore I've went with renaming GameplayState.HasFailed to the name in the diff of this conversation, as well as using that state for whether the judgement processors should no longer apply new results.

I will be honest that I didn't pay enough attention at the diff and missed the fact that GameplayState.HasFailed is used in more places than usual, and having the name specifically referring to "presence of fail animation" is indeed sketchy.

I could use better names such as HasStoppedDueToFail, but I've went ahead with your alternative proposal in 64b6110 (making the solution completely local) if discussion for this is not necessary right now (I could imagine another player screen that would require this sort of fix, namely ReplayPlayer when replaying failing multiplayer scores).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I could imagine another player screen that would require this sort of fix, namely ReplayPlayer when replaying failing multiplayer scores

I think this is out of scope for now because we currently don't even have a way to tell whether a replay is from multiplayer or from solo. So this has been an issue for a long time now, and it is one caused by the fact that the health processor is allowed to fail a replay, while not considering the fact that in some circumstances the conditions for failure may not be the same.

@pull-request-size pull-request-size bot added size/M and removed size/L labels Jan 27, 2024
@frenzibyte frenzibyte requested a review from bdach January 28, 2024 19:21
@peppy peppy self-requested a review January 29, 2024 07:30
@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Jan 29, 2024
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

fine by me

@peppy peppy merged commit a1fe5ee into ppy:master Jan 29, 2024
13 of 17 checks passed
@frenzibyte frenzibyte deleted the fix-multiplayer-fail-freezing-score branch January 29, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:gameplay area:multiplayer next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Score/accuracy/combo freezes on multiplayer match after reaching a fail state
3 participants