Skip to content
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

Use a stable sort for hitobjects #2563

Merged
merged 4 commits into from May 16, 2018
Merged

Use a stable sort for hitobjects #2563

merged 4 commits into from May 16, 2018

Conversation

@smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented May 16, 2018

Would change the order of hitobjects which would change the output of mania beatmap conversion.

A stable sort is intended here, see: https://github.com/peppy/osu-stable/blob/master/osu!/GameplayElements/HitObjectManager.cs#L822-L832

Brings osu!lazer and osu!stable diffcalc within 1 d.p. of each other:

Beatmap Mods Expected (strain) Actual (strain)
1023967 None 7.56702 7.56223
1023967 DT 10.4857 10.4810

Further accuracy isn't really possible due to osu!stable doing an unstable sort with a different algorithm than what .NET Core does, just before difficulty calculation.

@smoogipoo smoogipoo added this to the May 2018 milestone May 16, 2018
@peppy
Copy link
Member

@peppy peppy commented May 16, 2018

Let's add a comment explaining what we consider to be stable sorting here.

@smoogipoo
Copy link
Contributor Author

@smoogipoo smoogipoo commented May 16, 2018

On a further look inside mania beatmap conversion, I've come to the conclusion that this ordering doesn't really matter for mania-specific beamtaps since no RNG is done, and non-mania-specific beatmaps generally don't have multiple hitobjects at the same time, so I'm going to hold off on this for now in favour of less memory usage/faster sorting.

@smoogipoo smoogipoo closed this May 16, 2018
@smoogipoo
Copy link
Contributor Author

@smoogipoo smoogipoo commented May 16, 2018

Nope lol this totally matters in mania diff calculation :p

@smoogipoo smoogipoo reopened this May 16, 2018
@smoogipoo
Copy link
Contributor Author

@smoogipoo smoogipoo commented May 16, 2018

Hold off on this for a bit, there may be some sorting applied elsewhere that affects objects further in diff calculation.

@smoogipoo smoogipoo force-pushed the smoogipoo:fix-hitobject-order branch from aa4bbdc to 5aadc35 May 16, 2018
@peppy
peppy approved these changes May 16, 2018
@peppy peppy merged commit 7765a1a into ppy:master May 16, 2018
2 checks passed
2 checks passed
CodeFactor No issues found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@smoogipoo smoogipoo deleted the smoogipoo:fix-hitobject-order branch Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.