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.

Sign up for GitHub

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

Jump to bottom

Round object coordinates to nearest integers on legacy export rather than truncating #31305

Merged
merged 3 commits into from
Jan 1, 2025
Merged

Round object coordinates to nearest integers on legacy export rather than truncating #31305

merged 3 commits into from
Jan 1, 2025
+105 −17

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Dec 27, 2024 •
edited
Loading

RFC. Closes #31256.

This is an RFC because of 0d16ed0. The lowdown is that the truncating was actually silently being done by LegacyBeatmapDecoder:

osu/osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs

Lines 56 to 59 in fa0d2f4

Vector2 pos =
formatVersion >= LegacyBeatmapEncoder.FIRST_LAZER_VERSION
? new Vector2(Parsing.ParseFloat(split[0], Parsing.MAX_COORDINATE_VALUE), Parsing.ParseFloat(split[1], Parsing.MAX_COORDINATE_VALUE))
: new Vector2((int)Parsing.ParseFloat(split[0], Parsing.MAX_COORDINATE_VALUE), (int)Parsing.ParseFloat(split[1], Parsing.MAX_COORDINATE_VALUE));

I very explicitly do not want to be touching decoder logic for this, I want this change to be limited to the legacy export, which leaves very few options. We do not have a cloning / "reconstruct this hitobject with this property changed" facility available (#25467 says hi again), so exposing setters is about the only escape hatch available at this stage.

There are a few other code paths in the decoder preconditioned on FIRST_LAZER_VERSION:

osu/osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs

Lines 367 to 376 in fa0d2f4

if (formatVersion < LegacyBeatmapEncoder.FIRST_LAZER_VERSION)
{
if (vertices.Length + endPointLength != 3)
type = PathType.BEZIER;
else if (isLinear(points[0], points[1], endPoint ?? points[2]))
{
// osu-stable special-cased colinear perfect curves to a linear path
type = PathType.LINEAR;
}
}

osu/osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs

Lines 403 to 404 in fa0d2f4

if (type == PathType.CATMULL && endIndex > 1 && formatVersion < LegacyBeatmapEncoder.FIRST_LAZER_VERSION)
continue;

but I'm ignoring these at least until I find out whether everyone else is going to scoff at me for trying to do this in this way or not. Most likely I'll be ignoring these until someone complains.

@bdach bdach added area:beatmap-parsing .osu file format parsing area:editor labels Dec 27, 2024
@bdach bdach self-assigned this Dec 27, 2024
@bdach bdach mentioned this pull request Dec 27, 2024
Open
@peppy peppy self-requested a review December 27, 2024 19:15
peppy
peppy approved these changes Dec 28, 2024
View reviewed changes
Copy link
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.

I'm on board with this change.

@peppy peppy requested a review from smoogipoo December 28, 2024 23:25
@smoogipoo
Copy link
Contributor

!diffcalc

@github-actions GitHub Actions
Copy link

Target: #31305
Spreadsheet: https://docs.google.com/spreadsheets/d/1r26PdRI-8z4xMbA8xMrHXUyJW-QOoGS6ab-I0AINgaY/edit

@smoogipoo smoogipoo merged commit 9da27b5 into ppy:master Jan 1, 2025
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoogipoo smoogipoo smoogipoo approved these changes

@peppy peppy peppy approved these changes

Assignees

@bdach bdach

Labels
area:beatmap-parsing .osu file format parsing area:editor size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Saving a legacy beatmap truncates decimal instead of rounding
3 participants
@bdach @smoogipoo @peppy