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

Further timeline usability tweaks #29545

Merged
merged 7 commits into from
Aug 22, 2024
Merged

Conversation

peppy
Copy link
Member

@peppy peppy commented Aug 21, 2024

Based on general feedback over the last while.

Before After
osu Game Tests 2024-08-21 at 07 47 39 osu Game Tests 2024-08-21 at 07 47 11

I couldn't deal with the silly control point group handling so I had to rewrite that component. Huge simplification as a result. Already split into individual commits, can split out further if required (but I think it's pretty readable).

@peppy peppy requested a review from bdach August 21, 2024 07:50
@peppy peppy force-pushed the timeline-redesign-in-circles branch from c4b875e to fef56cc Compare August 21, 2024 07:58
@bdach
Copy link
Collaborator

bdach commented Aug 21, 2024

I dunno how deep I want to get into the conversation here since I'm rather tired of looking at this thing over and over and probably so are you, but I'll bite, starting from general feel without looking at the code yet...

Random first map I opened, I can't see any of the bpm text because it's covered by objects:

osu_2024-08-21_10-56-57

Also not sure how to feel about it even on the timing screen, if I have to tilt my head 90deg to even read the value.

If I were to suggest anything productive then I'd say that the value should be upright and not rotated, and maybe show to the left side of the timing point since that area is more likely to be unused.

And I guess the "show timing changes" toggle in the View menu just... doesn't do anything anymore?

Tick visibility seems okay I guess even after reducing the heights again, thanks to the increased spacing between pieces from the previous batch of changes (you can sorta look at the gaps to find the snap colour).

@peppy
Copy link
Member Author

peppy commented Aug 21, 2024

Random first map I opened, I can't see any of the bpm text because it's covered by objects

As I touched on in discord, I don't think it needs to be completely legible. The red shows through and people will know that that means there's a timing change. Hitting F3 will make it visible, or we can add tooltips or whatever. I'm open to changes to the design proposal, what's here is what I came up with in 10 minutes.

And I guess the "show timing changes" toggle in the View menu just... doesn't do anything anymore?

Not sure about this, can remove it if you think that's best.

@bdach
Copy link
Collaborator

bdach commented Aug 21, 2024

As I touched on in discord, I don't think it needs to be completely legible. The red shows through and people will know that that means there's a timing change. Hitting F3 will make it visible, or we can add tooltips or whatever.

Sure, let's just go with it and see what happens then.

Not sure about this, can remove it if you think that's best.

If it does nothing then remove it. Could just start with removal from the menu and keep the config key around (marked as deprecated or something) just in case we decide to bring it back at some point, to not drop user settings.

@peppy
Copy link
Member Author

peppy commented Aug 21, 2024

If it does nothing then remove it.

Oh I didn't actually realise I broke the functionality, intended to just remove the expanding part 😅. I'm okay to add it back and have it toggle, what do you think? I'm very neutral at this point (but less toggles better I guess?)

@bdach
Copy link
Collaborator

bdach commented Aug 21, 2024

If you're not fussed let's just make it work again I guess. We know already how people react to having toggles removed.

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.

Not gonna overthink this, let's put this to the test

@bdach bdach merged commit df94c22 into ppy:master Aug 22, 2024
11 of 13 checks passed
@peppy peppy deleted the timeline-redesign-in-circles branch August 23, 2024 09:56
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.

2 participants