-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Make slider path parsing more friendly with spans #27418
Conversation
Co-authored-by: Salman Ahmed <frenzibyte@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to both my eyes and CI.
{ | ||
int startIndex = segmentsBuffer[i].StartIndex; | ||
int endIndex = segmentsBuffer[i + 1].StartIndex; | ||
controlPoints.AddRange(convertPoints(segmentsBuffer[i].Type, allPoints.Slice(startIndex, endIndex - startIndex), pointsBuffer[endIndex])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, I think this particular case is better readable with range expression.
I don't have preference on other cases though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There have been multiple occurrences of the ..
operator being used and us not liking it in review, I don't want that happening here so I changed it with the Slice
method to move the PR forward.
Requesting a second pair of eyes before proceeding with a merge, the PR is good but I'm not sure if there's something I should've checked against before approval (like triggering a diffcalc command to make sure nothing has changed with beatmap parsing or something). |
!diffcalc |
Spreadsheets show no difference as well, so let's get this in. |
In order to unblock #23282, this PR refactors slider parsing to not passing substrings around.
The majority of existing logic is preserved. The performance shouldn't change much with this PR, just ensuring there's no regression.
Before:
After: