Fix timeline snapping ignoring nearby timing points #29435
Conversation
|
|
||
| if (time >= list[^1].Time) | ||
| return list[^1]; | ||
| int index = BinarySearchUtils.BinarySearch(list, time, c => c.Time, EqualitySelection.Rightmost); |
There was a problem hiding this comment.
Just for a bit of commentary, I think this matches osu!stable better (ControlPointAt / BeatLengthAt), but I will run a diffcalc to be sure.
There is ControlPointAtBin which appears to only be used to play sounds in mania, that isn't doing right-most, but I would say it's unlikely to cause any differences.
|
!diffcalc |
|
!diffcalc |
|
!diffcalc |
|
!diffcalc |
osu.Game/Utils/BinarySearchUtils.cs
Outdated
| /// <param name="termFunc">Function that maps an item in the list to its index property.</param> | ||
| /// <param name="equalitySelection">Determines which index to return if there are multiple exact matches.</param> | ||
| /// <returns>The index of the found item. Will return the complement of the index of the first item greater than the search query if no exact match is found.</returns> | ||
| public static int BinarySearch<T, T2>(IReadOnlyList<T> list, T2 searchTerm, Func<T, T2> termFunc, EqualitySelection equalitySelection = EqualitySelection.FirstFound) |
There was a problem hiding this comment.
Not a huge fan of the decision to make this have two typeparams. Would prefer one, and doing the necessary data projection (termFunc) in usages before calling this. It just complicates this method for no reason as far as I'm concerned.
I'd understand if the projection was expensive, then there'd be an argument that it's reducing the number of invocations of termFunc, but no, it's just a .Time access. If anything you may get better results by dumping the times off to a list before passing them to this method (waves hands about cache locality).
Either that or just move the method back inline where it used to live. I don't really get why generalise this if it's gonna have one caller ever.
There was a problem hiding this comment.
The purpose of termFunc is to make the method generic while keeping it simple for the caller and not increasing time complexity.
If I were to go without termFunc then the caller would have to do data projection before calling the binary search which can be slow because you have to create a new array with all the time values. Creating a data projection in linear time is no better than just using linear search. Or the caller has to manage an array with the time values and keep it in sync with the items which is way more complicated.
Therefore I think termFunc is the best solution for a generic binary search.
I understand your argument about not generalising. If its only ever going to be used with timing points then there's no need to generalise the function. But are we really going to never use binary search again in the future?
There was a problem hiding this comment.
We probably will and we even already are, but many places just do it locally and that is probably fine. Run a grep for BinarySearch on master and you'll see.
After having stepped on similar rakes too many times in the past I want to see that something will generalise before allowing someone to generalise. No generalising without at least two usages, preferably more.
Also in general not huge on generics these days either. Generics as implemented in C# introduce a lot of rigidity that is very difficult to root out later, so the less they are used the better in my book.
There was a problem hiding this comment.
Ok I see we use the built in binary search for lists a lot. I'll move the method back inline.
|
I can't see any issues in manual inspection, but the binary search implementation has visibly changed, so out of an overabundance of caution--- !diffcalc (I'll probably do only one just as a sanity check) |
closes #22720
Now objects in the timeline will consider the next timing point as a tick to snap to.
This implementation does a slight modification to the binary search logic that finds timing points. If there are multiple timing points at the same exact time, the new implementation will get the rightmost element (highest index) of all the exact matches. The old implementation would return the first match found, which is kind of undefined behaviour which I can't have for the
TimingPointAftermethod.