Change distance snap to never account for slider velocity #20850
Conversation
This is a nuanced detail that was implemented incorrectly from the outset. When mapping, generally a mapper chooses the distance spacing with no regard to the SV. It has always been common to have a lower or higher distance spacing than SV, but with the way the lazer editor has worked, the SV was multiplied into the distance snap grid display, incorectly changing its spacing depending on the "reference object" (which is usually the previous hitobject chronologically).
|
Hasn't osu!(stable) always functioned like that? I also recall a discussion thread surrounding this and it was agreed that slider velocity should be applied to the distance snap, but I can't find it at the moment. |
|
No, this is fixing things to work like stable. |
|
On checking with stable, slider velocity does affect the distance snap (known as "Slider Velocity Multiplier" in control points list window). Stable: CleanShot.2022-10-21.at.18.44.34.mp4Lazer (this PR): CleanShot.2022-10-21.at.18.51.06.mp4That said, on a quick Discord discussion, it appears on master the slider velocity isn't actually applied correctly to the distance snap (if a hitcircle was to be the reference object), and lazer has the concept of per-object slider velocity rather than stable which stores it in "control points", so maybe best to just not have it account for now. |
|
@peppy confirm whether this is still ok to get in? The discord conversation linked above doesn't seem very conclusive as I read it. |
|
I think it's probably still preferable because of the hidden nature of the SV adjustment source without it. We can iterate on it if people have issue with it going forward. That said, if you're going to merge please wait until I make the current release tag as I don't want any other changes merged in for that (ETA 10 minutes). |
This regressed with ppy#20850 because the function was used in other places which expect it to factor slider velocity into the equation. Rather than reverting, I've added a new argument, as based on the method naming alone it was hard to discern whether SV should actually be considered. The reason for the change in ppy#20850 was to avoid the SV coming in from a reference object which may not have a correct SV in the first place. In such cases, passing `false` to the function will give the expected behaviour.
This regressed with ppy#20850 because the function was used in other places which expect it to factor slider velocity into the equation. Rather than reverting, I've added a new argument, as based on the method naming alone it was hard to discern whether SV should actually be considered. The reason for the change in ppy#20850 was to avoid the SV coming in from a reference object which may not have a correct SV in the first place. In such cases, passing `false` to the function will give the expected behaviour.
This is a nuanced detail that was implemented incorrectly from the outset.
When mapping, generally a mapper chooses the distance spacing with no regard to the SV. It has always been common to have a lower or higher distance spacing than SV, but with the way the lazer editor has worked, the SV was multiplied into the distance snap grid display, incorrectly changing its spacing depending on the "reference object" (which is usually the previous hitobject chronologically).
This was pointed out to me on twitter by a mapper. Surprised it went unnoticed until now.