`BeatmapConverter` does not clone hit objects if they are already concrete

[frenzibyte]

Note that this issue only affects tests, but I'm opening it because it's totally not what I expect a BeatmapConverter to do.

This first came up in https://github.com/ppy/osu/pull/30731 which adds a mod that modifies the states of the beatmap's hit objects, and as per https://github.com/ppy/osu/pull/30731#issuecomment-2487082189, a bug occurred in the mod's test scene wherein the beatmap is affected indefinitely, and running the test a second time applies the mod on the beatmap after the mod was already applied on it, and the test fails due to that.

This is happening because of this specific conditional in BeatmapConverter: https://github.com/ppy/osu/blob/b33e54d0647916c5f37743f6bcf4b115c04aa62d/osu.Game/Beatmaps/BeatmapConverter.cs#L86-L90

Since the beatmap written in the tests had the hit object specs in their concrete types already (TaikoHitObjects), the beatmap converter was passing them verbatim to the "playable beatmap" where the mod is applied, rather than recreating each one of them.

This highlighted conditional above goes against the general expectations of BeatmapConverter, which is to produce a beatmap that is completely detached from the original one. https://github.com/ppy/osu/pull/25467 may be related, but it's solving things at a completely higher scale, whereas this one seems to be easily applicable to the current codebase.

[frenzibyte]

cc @ppy/team-client I guess since that's the main point of opening this issue, gathering feedback on whether the suggestion above makes sense or not.

[bdach]

25467 may be related, but it's solving things at a completely higher scale, whereas this one seems to be easily applicable to the current codebase.

It's really not. One does not just clone an arbitrary hitobject. You'd need at least a significant part of that pull.

I'm not really super willing to spend time thinking about this issue unless everyone else is.

[smoogipoo]

Definitely not super important for now, I agree.

I'm not sure that I agree with the sentiment that BeatmapConverter is/should be the one responsible for detaching/cloning beatmaps. Maybe something to address in the future - imo the first step is to have new Beatmap(IBeatmap other) that copies things, along with non-realm models.

[frenzibyte]

This is just meant in compliance with how the component works currently, which is literally cloning everything about the beatmap, so those lines I highlighted came across as extremely weird.

Either way I'll just leave this thread as-is.