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

Reset SongSelect dim before applying PlayerLoader dim #27746

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

64ArthurAraujo
Copy link
Contributor

@64ArthurAraujo 64ArthurAraujo commented Mar 28, 2024

Fixes #25515, and as i explained there the issue is that when you disable background blur the game sets a 60% dim for the SongSelect background, when the user enters the PlayerLoader screen it adds +20% dim on top of the song select background causing it to appear darker. The fix was to simply reset the dim Song Select applied to 0 before applying the PlayerLoader dim, the dim is applied again when the user goes back to Song Select.

Before the PR

PlayerLoader Editor PlayerLoader
player-pre-pr editor-pre-pr

After the PR

PlayerLoader Editor PlayerLoader
player-post-pr editor-post-pr

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.

Unit tests would be nice

osu.Game/Screens/Play/PlayerLoader.cs Outdated Show resolved Hide resolved
@64ArthurAraujo
Copy link
Contributor Author

64ArthurAraujo commented Mar 29, 2024

Unit tests would be nice

since it was such a small thing i tought it wouldnt need one. Also im not sure where to add them

@bdach
Copy link
Collaborator

bdach commented Apr 3, 2024

Looking at this again, I'm not sure whether this is the correct change to apply and the screenshots in the OP even illustrate that. Yes it makes player loader display consistently, but the issue thread also raises text readability concerns:

I don't think this is valid behavior, some backgrounds are too bright to view the text.

which I tend to agree with. So maybe rather than consistently less dim, we want consistently more dim here. Not sure.

Probably requires further ux/design feedback. (@peppy? @arflyte?)

@peppy peppy self-requested a review April 3, 2024 06:41
@peppy
Copy link
Sponsor Member

peppy commented Apr 3, 2024

I don't really get this either. The goal was to dim 20% on top of the song select dim, but this seems to be undoing that? It doesn't feel like the correct solution, which would be that the editor should also get 20% darker when entering the loading screen no?

@64ArthurAraujo
Copy link
Contributor Author

64ArthurAraujo commented Apr 3, 2024

I don't really get this either. The goal was to dim 20% on top of the song select dim, but this seems to be undoing that?

It is undoing that because when disabling background blur in song select, song select makes the background darker so the UI doesn't look bad or something, when going from SongSelect -> PlayerLoader that dim is carried over to PlayerLoader making it darker as in the first example.

which would be that the editor should also get 20% darker when entering the loading screen no?

I think player loader's background should be "independent" from anything else so changes are consistent across every instance of PlayerLoader in the game, avoiding issues like this in the future.

So maybe rather than consistently less dim, we want consistently more dim here. Not sure.

I agree as well, more dim across every PlayerLoader may be better.

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.

PlayerLoader(Loading screen) is not dimmed in editor test mode
4 participants