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

Collapse sample point indicators into dots if they cannot be displayed in full #29896

Merged
merged 12 commits into from
Nov 14, 2024

Conversation

OliBomby
Copy link
Contributor

This toggle reduces visual clutter for mappers who prefer to not see hitsound samples while mapping.

@bdach
Copy link
Collaborator

bdach commented Sep 18, 2024

I guess I don't have a particularly strong objection to this in isolation, although I do have a few doubts:

  • This is the only place in the entirety of editor where it is possible to set hitsounds without hotkeys, so hiding it is a bit painful. That said, given it defaults to off, I'd think anyone who did disable it for themselves should be able to show it again.
  • Why do sample pieces specifically get this treatment but slider velocity pieces don't?

Probably need to gather more opinions on this, I'm not super sure I'd be fine with merging without a consult.

@bdach bdach requested a review from peppy September 18, 2024 07:39
@peppy
Copy link
Member

peppy commented Sep 18, 2024

Dunno, next are we adding velocity toggles? Then hitobject toggles?

I feel this is misdirected UX. Where's the request coming from? I'd sooner focus on making the sample editor and then considering making the display here less obtrusive to the point people aren't asking(?) for toggles.

@OliBomby
Copy link
Contributor Author

Where's the request coming from?

I got the idea from a small discord discussion. I also heard other people wish to turn off the sample points. Clearly here the sample points add the most visual clutter, having the most objects in the timeline and easily overlapping neighbouring sample points. I think it then makes sense to have a toggle to turn them off because a lot of people map without doing hitsounds. For them it only adds noise.
Discord_KztYdKyZ7z

I'm also PR'ing this assuming there will be a sample editor in the future where you can better view the hitsounds in a separate tab. In that case it still makes sense to have this toggle, because there are two kinds of mappers:

  • Mappers who map first, then hitsound: Map in compose with sample points toggled off, then once the map is done hitsound in the sample editor or toggle on the sample points to do simple hitsounds in compose.
  • Mappers who map and hitsound at the same time: They map with sample points toggled on all the time.

Dunno, next are we adding velocity toggles? Then hitobject toggles?

Please dont extrapolate this to the extreme. I can see velocity toggles maybe being added, but its really not necessary because velocity points are not nearly as common as sample points so they dont add much visual clutter. Hitobject toggle? not at all.

@bdach
Copy link
Collaborator

bdach commented Sep 18, 2024

Discord_KztYdKyZ7z

This looks like a rather extreme example? Looks like the timeline is zoomed out to the maximum and not even the hitobject displays are all that useful at this density? Also I dunno what the "so many colors" comment is supposed to be given that 6 of the 8 unique colours shown are combo colours? Are we going to add a toggle for those too?

Like I have no idea how to interpret that, but to put that screenshot down as "not beginner friendly" with zero elaboration and then move to add a toggle sample pieces just seems like a giant non-sequitur to me. Maybe if the person on that screenshot elaborated on what was "not beginner friendly" about it?

@peppy
Copy link
Member

peppy commented Sep 18, 2024

I think a better initial approach may be to hide them when they begin to overlay? ie. require the user zoom in before it starts to display.

@OliBomby
Copy link
Contributor Author

This looks like a rather extreme example? Looks like the timeline is zoomed out to the maximum and not even the hitobject displays are all that useful at this density? Also I dunno what the "so many colors" comment is supposed to be given that 6 of the 8 unique colours shown are combo colours? Are we going to add a toggle for those too?

Maybe not a great example, but my point is that people want to toggle off the sample points. The visual clutter is there at normal zoom levels too.
osu Game Rulesets Osu Tests_wWMOb7SuIF
^ Information overload
osu Game Rulesets Osu Tests_UlLfVPMAFG
^ Mindful and demure

I think a better initial approach may be to hide them when they begin to overlay? ie. require the user zoom in before it starts to display.

This is an even weirder suggestion to me. The visibility should not be affected by the zoom level, that will get annoying very quick.

The problem is the information density of the sample points being much higher than the other elements in the timeline while some mappers dont even need this information, regardless of zoom level.

@peppy
Copy link
Member

peppy commented Sep 19, 2024

The visibility should not be affected by the zoom level, that will get annoying very quick.

Then have them reduce to a small pink dot, rather than overlapping pink-huge-blobs-with-text-that's-not-legible-anyway. The idea is to keep definition as much as possible.

@kadambishreyas
Copy link
Contributor

agree with peppy for both sample and velocity honestly.
the clutter is caused by the size of each button + number of buttons, so minimizing to a circle (perhaps with a generous clickable area) to provide a clickable to toggle settings when there is no change in the visible statistics (i.e. sv for green labels, sampleset OR hitsound volume for pink labels) from the previous object seems like a massive improvement imo.

@bdach
Copy link
Collaborator

bdach commented Sep 20, 2024

The size reduction seems to work ok, but why is the toggle still there?

2024-09-20.09-45-49.mp4

I thought the goal was to have the dots shrink instead of the toggle, rather than in addition to? @peppy did I misunderstand?

@peppy
Copy link
Member

peppy commented Sep 27, 2024

I'd prefer not having the toggle at all.

@peppy
Copy link
Member

peppy commented Sep 27, 2024

Seems to contract way too early depending on BPM/SV (tested using this map)?

osu! 2024-09-27 at 08 14 04

@OliBomby
Copy link
Contributor Author

Seems to contract way too early depending on BPM/SV

The best point to contract depends on the density of the beatmap which various wildly between different BPMs and star ratings. I think we need a more sophisticated criterion to determine when to contract the sample points which looks at the hitobjects in the timeline.

@peppy
Copy link
Member

peppy commented Sep 27, 2024

I think something could be done in TimelineBlueprintContainer to that efffect. Basically constantly watching for the lowest/highest SV of visible objects and adjusting a BindableBool governing the contracted state.

It finds the smallest distance between two sample point pieces on the alive timeline blueprints
@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 5, 2024
@OliBomby
Copy link
Contributor Author

OliBomby commented Oct 5, 2024

I made it contract the sample point pieces based on the sample point pieces which are closest together on the visible timeline, so basically it tried to prevent any overlap from happening and it will not contract if there is no overlap.

It's a lot better but ideally I'd want to mix contracted and non-contracted sample point pieces on the same timeline, so you don't have all the sample points contracting together when only one is overlapping. However I don't know how to get a list of just the sample point drawables on the timeline because of the way they exist as children of TimelineHitObjectBlueprint. It might require a refactor to put all the sample pieces on a seperate timeline part.

@bdach bdach self-requested a review October 14, 2024 07:46
@bdach
Copy link
Collaborator

bdach commented Oct 14, 2024

I made it contract the sample point pieces based on the sample point pieces which are closest together on the visible timeline, so basically it tried to prevent any overlap from happening and it will not contract if there is no overlap.

Doesn't work properly when seeking:

2024-10-14.09-50-47.mp4

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.

see above

@OliBomby
Copy link
Contributor Author

To fix the contraction not updating I followed the example of updateStacking() and made it update every frame. There doesn't seem to be an easy way to detect for changes in the SelectionBlueprints.

double lastTime = double.PositiveInfinity;

// The blueprints are ordered in reverse chronological order
foreach (var selectionBlueprint in SelectionBlueprints)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can't be done. While SelectionBlueprints will only contain the visible blueprints in the general case... it will also contain more blueprints if they are selected. In fact if you select every object you may see these contract for no reason:

2024-10-21.12-00-51.mp4

I imagine this will also make selecting all objects even slower than it usually is, although crude profiling shows maybe not very much:

1729505224

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're very right. I forgot the Ctrl+A loads every single selection blueprint. I'll find some way to get only the visible blueprints.

The performance impact is probably small because it only does a simple operation per each blueprint.

@bdach
Copy link
Collaborator

bdach commented Nov 1, 2024

I'm not sure why neither #29896 (comment) nor #29896 (comment) were ever responded to, but to move this forward I removed the toggle in 8f48682. We can revisit if the collapse change is deemed insufficient. (A pesky merge conflict gave it away.)

Otherwise seems fine, but I will wait for the @peppy UX sign-off.

@bdach bdach changed the title Add view menu toggle for sample points Collapse sample point indicators into dots if they cannot be displayed in full Nov 1, 2024
@OliBomby
Copy link
Contributor Author

OliBomby commented Nov 1, 2024

Sorry for not responding to the comments. I'm fine with going the current direction and revisiting the toggle if this is deemed insufficient.

@peppy peppy enabled auto-merge November 13, 2024 10:18
@bdach bdach disabled auto-merge November 14, 2024 07:28
@bdach bdach merged commit 02e4907 into ppy:master Nov 14, 2024
7 of 10 checks passed
@OliBomby OliBomby deleted the hs-toggle2 branch November 14, 2024 07:42
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.

4 participants