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

Fixes to the song select screen #814

Merged
merged 24 commits into from May 22, 2017

Conversation

3 participants
@MrTheMake
Contributor

MrTheMake commented May 20, 2017

Fixes:

  • Current beatmap track is restarting when entering song select and changing to another difficulty from the same beatmap set
  • You can click through the song select footer
  • Lowest beatmap group not expanding on selection if it is not completely on-screen (Resolves #816)
  • Beatmap tracks do not loop at the song select (Depends on ppy/osu-framework#741)
  • Pressing up always goes to the first difficulty of a beatmap group when switching the group, instead of the last difficulty
  • Unintuitive key behavior. UP now changes to the last difficulty when changing the beatmap set. LEFT and RIGHT now select the selected difficulty of a beatmap set rather than the first one.
  • Difficulty not being randomized when pressing "random"

Not ready to be merged yet.

Show outdated Hide outdated osu.Game/Screens/Select/Footer.cs
protected override bool InternalContains(Vector2 screenSpacePos) => base.InternalContains(screenSpacePos) || StartButton.Contains(screenSpacePos);
protected override bool OnMouseDown(InputState state, MouseDownEventArgs args) => true;

This comment has been minimized.

@huoyaoyuan

huoyaoyuan May 20, 2017

Contributor

Why are these handlers added?

@huoyaoyuan

huoyaoyuan May 20, 2017

Contributor

Why are these handlers added?

This comment has been minimized.

@MrTheMake

MrTheMake May 20, 2017

Contributor

To disallow clicking through the footer container. Without those you can select a beatmap through the footer.

@MrTheMake

MrTheMake May 20, 2017

Contributor

To disallow clicking through the footer container. Without those you can select a beatmap through the footer.

@MrTheMake

This comment has been minimized.

Show comment
Hide comment
@MrTheMake

MrTheMake May 20, 2017

Contributor

This is ready to be merged. I couldn't fix the issue I wanted to fix. I will open an issue on GitHub for that.

Contributor

MrTheMake commented May 20, 2017

This is ready to be merged. I couldn't fix the issue I wanted to fix. I will open an issue on GitHub for that.

@MrTheMake

This comment has been minimized.

Show comment
Hide comment
@MrTheMake

MrTheMake May 21, 2017

Contributor

I managed to resolve the issue. However, I encountered 2 additional things I want to fix. Therefore, please don't merge this yet.

Contributor

MrTheMake commented May 21, 2017

I managed to resolve the issue. However, I encountered 2 additional things I want to fix. Therefore, please don't merge this yet.

Show outdated Hide outdated osu.Game/Screens/Select/SongSelect.cs
{
base.Update();
ensurePlayingSelected();

This comment has been minimized.

@peppy

peppy May 21, 2017

Member

isn't this going to do weird things? it means the user can't pause because the track will just instantly start again. it is also calling a potentially expensive SetExclusiveTrack function too

@peppy

peppy May 21, 2017

Member

isn't this going to do weird things? it means the user can't pause because the track will just instantly start again. it is also calling a potentially expensive SetExclusiveTrack function too

This comment has been minimized.

@MrTheMake

MrTheMake May 21, 2017

Contributor

Yes, I've discovered that too. I am working on resolving it.

@MrTheMake

MrTheMake May 21, 2017

Contributor

Yes, I've discovered that too. I am working on resolving it.

@MrTheMake

This comment has been minimized.

Show comment
Hide comment
@MrTheMake

MrTheMake May 21, 2017

Contributor

Conceptually, this can be reviewed. There are more fixes that need to be implemented, but the fixes which are already there can be checked.

Contributor

MrTheMake commented May 21, 2017

Conceptually, this can be reviewed. There are more fixes that need to be implemented, but the fixes which are already there can be checked.

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy May 22, 2017

Member

For future PRs, I'd recommend splitting each fix into its own PR. This just makes it easier for us to review your code and get it merged.

Member

peppy commented May 22, 2017

For future PRs, I'd recommend splitting each fix into its own PR. This just makes it easier for us to review your code and get it merged.

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy May 22, 2017

Member

Nice job with these fixes.

Member

peppy commented May 22, 2017

Nice job with these fixes.

@peppy

peppy approved these changes May 22, 2017

@peppy peppy merged commit f8c419f into ppy:master May 22, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@MrTheMake MrTheMake deleted the MrTheMake:songselect-fixes branch May 22, 2017

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