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.

Sign up for GitHub

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

Jump to bottom

Skip score uploads when nothing was hit in score #224

Merged
merged 1 commit into from Mar 14, 2024
Merged

Skip score uploads when nothing was hit in score #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 47 additions & 2 deletions osu.Server.Spectator.Tests/SpectatorHubTest.cs
View file
Open in desktop
Expand Up @@ -133,7 +133,13 @@ public async Task ReplayDataIsSaved(bool savingEnabled)

await hub.BeginPlaySession(1234, state);
await hub.SendFrameData(new FrameDataBundle(
new FrameHeader(new ScoreInfo(), new ScoreProcessorStatistics()),
new FrameHeader(new ScoreInfo
{
Statistics =
{
[HitResult.Great] = 1
}
}, new ScoreProcessorStatistics()),
new[] { new LegacyReplayFrame(1234, 0, 0, ReplayButtonState.None) }));
await hub.EndPlaySession(state);

Expand All @@ -145,6 +151,39 @@ public async Task ReplayDataIsSaved(bool savingEnabled)
mockScoreStorage.Verify(s => s.WriteAsync(It.IsAny<Score>()), Times.Never);
}

[Fact]
public async Task ReplaysWithoutAnyHitsAreDiscarded()
{
AppSettings.SaveReplays = true;

Mock<IHubCallerClients<ISpectatorClient>> mockClients = new Mock<IHubCallerClients<ISpectatorClient>>();
Mock<ISpectatorClient> mockReceiver = new Mock<ISpectatorClient>();
mockClients.Setup(clients => clients.All).Returns(mockReceiver.Object);
mockClients.Setup(clients => clients.Group(SpectatorHub.GetGroupId(streamer_id))).Returns(mockReceiver.Object);

Mock<HubCallerContext> mockContext = new Mock<HubCallerContext>();

mockContext.Setup(context => context.UserIdentifier).Returns(streamer_id.ToString());
hub.Context = mockContext.Object;
hub.Clients = mockClients.Object;

mockDatabase.Setup(db => db.GetScoreFromToken(1234)).Returns(Task.FromResult<SoloScore?>(new SoloScore
{
id = 456,
passed = true
}));

await hub.BeginPlaySession(1234, state);
await hub.SendFrameData(new FrameDataBundle(
new FrameHeader(new ScoreInfo(), new ScoreProcessorStatistics()),
new[] { new LegacyReplayFrame(1234, 0, 0, ReplayButtonState.None) }));
await hub.EndPlaySession(state);

await scoreUploader.Flush();

mockScoreStorage.Verify(s => s.WriteAsync(It.IsAny<Score>()), Times.Never);
}

[Theory]
[InlineData(false)]
[InlineData(true)]
Expand Down Expand Up @@ -287,7 +326,13 @@ public async Task ScoresAreOnlySavedOnRankedBeatmaps(BeatmapOnlineStatus status,

await hub.BeginPlaySession(1234, state);
await hub.SendFrameData(new FrameDataBundle(
new FrameHeader(new ScoreInfo(), new ScoreProcessorStatistics()),
new FrameHeader(new ScoreInfo
{
Statistics =
{
[HitResult.Great] = 10
}
}, new ScoreProcessorStatistics()),
new[] { new LegacyReplayFrame(1234, 0, 0, ReplayButtonState.None) }));
await hub.EndPlaySession(state);

Expand Down
13 changes: 10 additions & 3 deletions osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@
using Microsoft.Extensions.Caching.Distributed;
using osu.Game.Beatmaps;
using osu.Game.Online.Spectator;
using osu.Game.Rulesets.Scoring;
using osu.Game.Scoring;
using osu.Server.Spectator.Database;
using osu.Server.Spectator.Database.Models;
Expand Down Expand Up @@ -145,10 +146,16 @@ public async Task EndPlaySession(SpectatorState state)
if (status < min_beatmap_status_for_replays || status > max_beatmap_status_for_replays)
return;

score.ScoreInfo.Date = DateTimeOffset.UtcNow;
// if the user never hit anything, further processing that depends on the score existing can be waived because the client won't have submitted the score anyway.
// note that this isn't an early return as we still want to end the play session.
// see: https://github.com/ppy/osu/blob/a47ccb8edd2392258b6b7e176b222a9ecd511fc0/osu.Game/Screens/Play/SubmittingPlayer.cs#L281
Comment on lines +150 to +151
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand what this comment means by "end the play session" - there's nothing running after the block other than the finally, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is actually:

osu-server-spectator/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs

Line 159 in cc1d36d

await endPlaySession(Context.GetUserId(), state);

Skipping that causes a test to fail. I considered moving that call inside finally but wasn't convinced that was correct.

Copy link
Contributor

@smoogipoo smoogipoo Mar 14, 2024 •
edited

Choose a reason for hiding this comment

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

Oh, it was hidden. How come that isn't an issue for the above return? Something somewhere (osu-web?) blocks it from starting?

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 can understand this one skipping it:

osu-server-spectator/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs

Lines 138 to 141 in cc1d36d

// Score may be null if the BeginPlaySession call failed but the client is still sending frame data.
// For now it's safe to drop these frames.
if (score == null || scoreToken == null)
return;

because it assumes that BeginPlaySession() failed. But I don't understand this one:

osu-server-spectator/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs

Lines 143 to 146 in cc1d36d

// Do nothing with scores on unranked beatmaps.
var status = score.ScoreInfo.BeatmapInfo!.Status;
if (status < min_beatmap_status_for_replays || status > max_beatmap_status_for_replays)
return;

I think that may be an actual bug?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm okay. That second one is the one I was referring to specifically. I think this is fine to get in as-is but that should probably be investigated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I'm looking at it.

if (score.ScoreInfo.Statistics.Any(s => s.Key.IsHit() && s.Value > 0))
{
score.ScoreInfo.Date = DateTimeOffset.UtcNow;

scoreUploader.Enqueue(scoreToken.Value, score);
await scoreProcessedSubscriber.RegisterForNotificationAsync(Context.ConnectionId, Context.GetUserId(), scoreToken.Value);
scoreUploader.Enqueue(scoreToken.Value, score);
await scoreProcessedSubscriber.RegisterForNotificationAsync(Context.ConnectionId, Context.GetUserId(), scoreToken.Value);
}
}
finally
{
Expand Down