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 player settings in replay hiding when dragging a slider #31593

Conversation

Rudicito
Copy link
Contributor

Before :

before.mp4

After :

after.mp4

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

I'm always a little bit scared of these unconstrained DraggedDrawable checks, but in this case it looks to work fine.

@smoogipoo smoogipoo merged commit 2c5b438 into ppy:master Jan 21, 2025
7 of 10 checks passed
@peppy
Copy link
Member

peppy commented Jan 21, 2025

I'm always a little bit scared of these unconstrained DraggedDrawable checks, but in this case it looks to work fine.

Yeah, not sure about this, usually we'd check the dragged thing is a child or something?

I think it's maybe going to hold for now, but it will break if anything else is dragged:

osu.2025-01-21.at.05.36.04.mp4

(example for argument's sake, not an actual issue for the time being, until (if) we add more UI elements to the screen)

@smoogipoo
Copy link
Contributor

I am fine with it for now because I looked at another case that came to mind:

&& (inputManager.DraggedDrawable == null || inputManager.DraggedDrawable is OsuLogo)

@Rudicito
Copy link
Contributor Author

Rudicito commented Jan 21, 2025

Something like this would be better right ?

|| inputManager.DraggedDrawable.IsRootedAt(this);

But needs framework with ppy/osu-framework#6500

@peppy
Copy link
Member

peppy commented Jan 22, 2025

Something like this would be better right ?


|| inputManager.DraggedDrawable.IsRootedAt(this);

But needs framework with ppy/osu-framework#6500

Yes this sounds like it would work, amongst other alternatives.

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

Successfully merging this pull request may close these issues.

3 participants