ppy
Distance snap not snapping to radius
Since #20850, the distance spacing grid is supposed to ignore slider velocity. Currently it is visually correct, but the placement doesn't match these visuals. I'm sure it was working as expected when that PR was merged, so something may have regressed in the surrounding paths.
Discussed in https://github.com/ppy/osu/discussions/21325
Originally posted by Axilotl17 November 20, 2022
it appears that while the UI that shows the distance snap is accurate and not tied to SV, the actual distance snap still is, so when the distance snap is after a slider with SV =/= 1 then it is inaccurate to the UI.
I've spent some hours looking into this and it's... a pain.
What I can say is that we probably don't want the bool that was added in https://github.com/ppy/osu/pull/21049. It's not applied in enough places as can be seen here, and it adds more complexity to already hard-to-understand methods.
Also while investigating, I noticed that the brightness of the distance snap grid lines are incorrect if velocity is adjusted. They are also incorrect if the end of the slider is not snapped. In fact, the logic added to fix slider end snapping (in https://github.com/ppy/osu/pull/20941) is so weird I can't follow it (and I wrote it). Specifically, I'm not sure on the accuracy of the FindSnappedDistance method it's calling. And that method is involving EndTime while none of the others do.
These snap provider implementations just aren't working well for me in terms of understanding usages. It may be worth culling a lot of these methods and preferring local implementations.
I think the culprits are just DistancedHitObjectComposer.DurationToDistance(...) and .DistanceToDuration(...). Both rely on .GetBeatSnapDistanceAt(...), but weren't made to specify the recently added bool parameter useReferenceSliderVelocity, which defaults to true. Adding the same parameters to .DurationToDistance(...) and .DistanceToDuration(...) and then forwarding them to .GetBeatSnapDistanceAt(...) fixed it for me. Though as you said, its default value of true isn't actually used anywhere, so just removing it altogether makes the most sense and fixes the issue entirely (if the ternary branch for true is removed as well).
Yeah, I traced it back to those methods as well. I know I can fix this particular issue, but the larger problem still remains that I don't feel confident in the method / class structure in the first place. And the remaining two issues I discovered are still at large.
I'll drop my WIP branch here in case you want to take a look or attempt to take things further. I'll probably revisit this after a couple of days break. https://github.com/ppy/osu/compare/master...peppy:osu:fix-editor-distance-snap-again-again-again?expand=1
I'm not sure if I'm referring to the same effect, but I'm seeing a "fade-in, fade-out" alpha effect on the circular distance snap (DS) grid after a non-snapped slider end. When the slider end isn't snapped to a tick, subsequent DS blueprints aren't snapped either, instead offset by 1/(beat divisor) from the non-snapped slider end. The blueprint ends up between timing ticks like the slider end. As you scroll between ticks, you scroll "over"/"past" the blueprint, which leads to a weird alpha "fade-in fade-out" effect.
This doesn't seem to me an issue with the grid alphas, but a result of DS time-snapping logic. Not sure if this is intended or not. Should subsequent objects still be a constant X distance away from a non-snapped slider end, or should the first object be closer in order to be snapped to timeline ticks? The second option is probably preferable, but it also makes the first object no longer really "distance snapped", but "distance snapped and then truncated".
Also, it seems removing the bool parameter doesn't work, because it is indirectly used in one place, SliderPlacementBlueprint, via DistancedHitObjectComposer.FindSnappedDistance(...), which then calls .DistanceToDuration(...). Removing the parameter would regress #21049. So the lazy fix (which doesn't address the clunky structure) is to further include the bool parameter in .FindSnappedDistance(...) (and probably .FindSnappedDuration(...) for uniformity), and then to do the same parameter propagation. It also makes sense to default to false, since the true value is actually only used in this one specific place.
Couldn't replicate the incorrect alphas for adjusted SV though. Maybe adjusting SV caused the slider end to become non-snapped?
Couldn't replicate the incorrect alphas for adjusted SV though. Maybe adjusting SV caused the slider end to become non-snapped?
That seems to be the case. I think when I was seeing further issues was after I applied further code changes. It's still weird though (needs a rewrite of that logic to handle such a case) – I'd expect it to work even with non-snapped ends.
@goodtrailer can you check that same branch (specifically with https://github.com/peppy/osu/commit/e961dce595b7c7d8648ae4eeab7ddfe8f937559c)? I think this should resolve the issue you pointed out and leave things in a ... mostly working state.
Apparently distance snap have some weird behavior when using slider end as anchor. It's kind of hard to explain but I uploaded the video. Looks like non-1.0x slider velocity multiplier is the cause. Although I'm not sure if this is expected behavior since sometimes slider velocity and snap distance should be same which is confusing. Maybe a new option is preferred?
https://user-images.githubusercontent.com/11626920/210062096-df0ff20c-d6e8-438d-9d27-0532c25788e6.mp4
2022.1228.0
database.log input.log legacy-ipc.log network.log performance.log performance-audio.log performance-draw.log performance-input.log performance-update.log runtime.log updater.log
Any chance this could get resolved soon? The fact that distance snap works differently on circles and sliderends is a bit of a large problem when making low diffs.
