Replays cannot be viewed after map is exported from editor without any changes made

[apollo-dw]

Type

Crash to desktop

Bug description

osu.Game.Scoring.Legacy.LegacyScoreDecoder+BeatmapNotFoundException: No corresponding beatmap for the score could be found.

Steps:

1. Set a score with Strict Tracking
2. Export the map via the editor
3. Try to watch the Strict Tracking score

Does not appear to happen with HD. Creating a new difficulty beforehand crashes the map, whilst playing an existing map and exporting it seems to only give a notification.

Note that while the replay fails with ST, it does not fail with HD + ST.

Screenshots or videos

https://user-images.githubusercontent.com/83023433/160282842-e1e8c1c7-c3c3-47b9-a07f-f3097d61f2ac.mp4

Version

2022.327.0-lazer

Logs

network.log database.log updater.log legacy-ipc.log performance-audio.log performance-draw.log performance-input.log performance-update.log runtime.log performance.log

[bdach]

This is not unique to the strict tracking mod. I've just reproduced the same crash on a score playback without selecting any mod at all.

It seems that the act of exporting a map changes its md5 hash which then leads to it no longer being recognised anymore by the score decoder. Unsure of why this happens yet.

[bdach]

This is specific to the new difficulty creation flow and is caused by the fact that LegacyBeatmapEncoder does not truncate hitobject coordinates (hitcircle position, slider position) to integers, but LegacyBeatmapDecoder does, which causes the following:

• First save of beatmap writes position with decimal parts remaining
• Exiting editor and entering it again reloads the map via LegacyBeatmapDecoder, which truncates the decimal parts
• The export flow calls Save() and inadvertently causes the decimal parts to be dropped

Following test case provided for reproduction (drop in LegacyBeatmapEncoderTest to reproduce):

        [TestCase]
        public void TestEncodeDecodeStabilityWithDecimalHitObjectCoords()
        {
            var beatmap = new OsuBeatmap
            {
                HitObjects = new List<OsuHitObject>
                {
                    new HitCircle { Position = new Vector2(2.4f, 3.2f) },
                    new Slider
                    {
                        Position = new Vector2(4.4f, 9.9f),
                        Path = new SliderPath(new[]
                        {
                            new PathControlPoint(Vector2.Zero),
                            new PathControlPoint(new Vector2(8.9f, 3.2f)),
                            new PathControlPoint(new Vector2(4.3f, 9.8f))
                        })
                    }
                }
            };
            foreach (var hitObject in beatmap.HitObjects)
                hitObject.ApplyDefaults(beatmap.ControlPointInfo, beatmap.Difficulty);

            var toEncode = (beatmap, new TestLegacySkin(beatmaps_resource_store, string.Empty));

            var decodedAfterEncode = decodeFromLegacy(encodeToLegacy(toEncode), string.Empty);

            sort(beatmap);
            sort(decodedAfterEncode.beatmap);

            compareBeatmaps(toEncode, decodedAfterEncode);
        }

Stable deals with this by truncating to ints in the encoder, but lazer's copy of it doesn't do that. @smoogipoo do you recall if this may have been an intentional decision? Any reason not to add the truncations back?

[smoogipoo]

I believe the reason was only that stable seemed to handle it, so there wasn't immediately a reason to add truncation. However, I'm not opposed to adding it if necessary. Probably confirm with @peppy too.

[peppy]

I think the best way forward for now is to truncate in the encoder. We can consider whether we want the fractional portion or not when we get to making larger changes to the beatmap file format.

[peppy]

I've done the initial work on this (https://github.com/ppy/osu/compare/master...peppy:truncate-decimal-hit-object-position?expand=1) using the above test case with some modifications. It shows output in a legible manner.

There are some remaining issues with slider point construction that I can't immediately follow (path leads to https://github.com/ppy/osu/pull/12309). Not sure how to fix.

[OliBomby]

Maybe I'm missing something, but isn't it better to remove the coordinate truncation from LegacyBeatmapDecoder? Or at least for all beatmaps of file format v128 or later. That will preserve the decimal coordinates for beatmaps made in lazer, but old beatmaps (file format v14 or lower) will still get coordinates truncated like they should.

[bdach]

That is a possible alternative yes (I haven't checked if there are issues with it, though). Although to me it is a bit dubious as to whether decimal coordinates are actually... useful to keep in any way, shape or form.

[OliBomby]

it is a bit dubious as to whether decimal coordinates are actually... useful to keep in any way, shape or form.

I've spoken to many mappers which would love to have decimal coordinates in their beatmaps and were excited about the direction lazer was going in this regard, so removing decimal coordinates from lazer would be a great loss. When mapping intricate slider shapes, the rounding error in coordinates is often great enough to be clearly visible in the slider shape.

[peppy]

I do think that's out of scope of this discussion, which is aimed at fixing a compatibility issue. Can see benefits in doing so, but we also would want to figoure out the precision before officially allowing it so we don't end up with 15.99999889989999 rounding abuse aspire maps that we have to deal with for another decade.

Removing the rounding may be an okay temporary solution, but would require testing to ensure it doesn't regress anything.

[gustarmartins]

I can't reproduce this issue anymore. Would you @bdach mind checking it again to see if it still happens? Perhaps it can be closed.

[bdach]

Looks like https://github.com/ppy/osu/pull/25223 kind of fixed this by prompting to save before export yes if there were any changes made (and skipping the problematic save call if there weren't). Closing, thanks.