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

Implement legacy beatmap encoding for all rulesets #8823

Merged
merged 14 commits into from
Apr 24, 2020

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Apr 21, 2020

Prereqs:

Biggest changes are those to mania since hitobject samples may have changed for converts. It needs to be looked into further anyway, since I'm fairly certain it's not handled correctly anyway:

#8595
#2506

@bdach
Copy link
Collaborator

bdach commented Apr 21, 2020

I read through this. A few places were a little weird (drumrolls with IHasCurve, although I do like that it was an explicit implementation, or the encoder not being able to reference LEGACY_VELOCITY_MULTIPLIER due to library separation) but after some thinking I figured this is legacy stuff anyway and doing this "properly" would probably be a headache and a half, so probably best to leave it as-is and deal with it if it ever becomes a problem (which it probably won't).

As to actual correctness of the changes I mostly defer to the tests themselves as I'm not very familiar with the osu! file format but it all looked correct as far as I could tell. History looks bad here too though, same as in the other PR.

Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't see any issues.

@peppy peppy merged commit 59bd2b3 into ppy:master Apr 24, 2020
@smoogipoo smoogipoo deleted the all-ruleset-encoders branch May 21, 2021 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants