ppy
Add view menu toggle for sample points
This toggle reduces visual clutter for mappers who prefer to not see hitsound samples while mapping.
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.
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.
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.
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.
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?
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 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.
^ Information overload
^ 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.
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.
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.
The size reduction seems to work ok, but why is the toggle still there?
https://github.com/user-attachments/assets/32c89d2b-cd48-4497-9eb8-5de96fecb360
I thought the goal was to have the dots shrink instead of the toggle, rather than in addition to? @peppy did I misunderstand?
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.
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.
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.
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:
https://github.com/user-attachments/assets/e0cddb5e-5ff4-4b92-b84a-338c7f2a59ce
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.
I'm not sure why neither https://github.com/ppy/osu/pull/29896#issuecomment-2363054820 nor https://github.com/ppy/osu/pull/29896#issuecomment-2378676851 were ever responded to, but to move this forward I removed the toggle in https://github.com/ppy/osu/pull/29896/commits/8f48682d0a8a8385d2ade6db7ce0f03eac1668b4. 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.
Sorry for not responding to the comments. I'm fine with going the current direction and revisiting the toggle if this is deemed insufficient.