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 song select rewind logic not handling deleted beatmaps #23312

Merged
merged 5 commits into from
Jun 15, 2023
Merged

Fix song select rewind logic not handling deleted beatmaps #23312

merged 5 commits into from
Jun 15, 2023

Conversation

Renzo904
Copy link
Contributor

When you rewind to a deleted beatmap, the game still selects the deleted beatmap, glitching the carousel, now it should check if the beatmap to rewind exists within beatmapSets
Before:
2023-04-24_23-28

Check if the poped beatmap exists within the beatmapSets
@bdach
Copy link
Collaborator

bdach commented Apr 25, 2023

Please add test coverage for the bugged scenario in this pull request. See TestSceneBeatmapCarousel.TestRandom() for inspiration.

@@ -540,7 +540,7 @@ public void SelectPreviousRandom()
{
var beatmap = randomSelectedBeatmaps.Pop();

if (!beatmap.Filtered.Value)
if (!beatmap.Filtered.Value && beatmapSets.Any(beatset => beatset.Beatmaps.Contains(beatmap)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably not be checking all beatmaps like that and be doing

Suggested change
if (!beatmap.Filtered.Value && beatmapSets.Any(beatset => beatset.Beatmaps.Contains(beatmap)))
if (!beatmap.Filtered.Value && beatmap.BeatmapSet?.PendingDelete != false)

instead.

Copy link
Member

Choose a reason for hiding this comment

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

The only way to access the PendingDelete flag would be by: beatmap.BeatmapInfo.BeatmapSet.PendingDelete. And apparently, that doesn't look to get updated when the beatmap is actually deleted.

I've went with a different solution that's still magnitudes more efficient than iterating through all beatmaps in the carousel.

@frenzibyte frenzibyte self-assigned this Jun 15, 2023
@frenzibyte frenzibyte requested a review from a team June 15, 2023 08:24
@frenzibyte frenzibyte requested a review from a team June 15, 2023 08:26
@frenzibyte frenzibyte changed the title Add extra check in 'SelectPreviousRandom' Fix song select rewind logic not handling deleted beatmaps Jun 15, 2023
@peppy peppy self-requested a review June 15, 2023 09:28
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

probably okay

@peppy peppy merged commit d563de0 into ppy:master Jun 15, 2023
@LittleEndu
Copy link
Contributor

Switch from stack to list wasn't handled correctly. Rewind now selects the last beatmap from list but removes the first instance from the list, so if there are multiple instance of the same beatmap, rewind will keep selecting the same beatmap until all instances are removed. Easy to reproduce if you search for something that returns 5 or so beatmaps, use random 15 times, rewind will select each beatmap 3 times before selecting the next in list.

@bdach
Copy link
Collaborator

bdach commented Dec 18, 2023

@LittleEndu can you open an actual issue please thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants