Add countdown settings to beatmap info #14480
Conversation
bdach
added
area:beatmap-parsing
bdach
changed the title
| osu file format v14 | ||
|
|
||
| [General] | ||
| Countdown: 2 | ||
| CountdownOffset: 3 |
There was a problem hiding this comment.
This beatmap file is implicitly used for test coverage in LegacyBeatmapEncoderTest.TestEncodeDecodeStability (it uses all .osu files from test resources).
| @@ -95,6 +94,9 @@ public class BeatmapInfo : IEquatable<BeatmapInfo>, IJsonSerializable, IHasPrima | |||
| public bool WidescreenStoryboard { get; set; } | |||
| public bool EpilepsyWarning { get; set; } | |||
|
|
|||
| public CountdownType Countdown { get; set; } = CountdownType.Normal; | |||
There was a problem hiding this comment.
Hmm, not sure this actually needs to be at a BeatmapInfo level. It could potentially just exist in Beatmap. The question is how we want to handle this kind of beatmap-level settings going forward I guess, and whether they should all exist in the database or only at parse-time.
Same can be said for many other variables in this class, so it might not need immediate action – just mentioning it while we're here.
There was a problem hiding this comment.
It doesn't necessarily need to be on BeatmapInfo, true. I was mostly following precedent at this point (the fact that there was a countdown setting there already, and even ignoring that, these two would be the only non-databased settings on the setup screen side from combo colours).
I won't claim to have any insight on which way will be best eventually. Technically an implementation of the countdown (which doesn't exist yet) shouldn't need this databased, decoding would do just fine.
There was a problem hiding this comment.
I think eventually we'd see only properties that are relevant outside of gameplay in BeatmapInfo, but as you say, it's probably fine to leave this here for now and move them all in bulk.
| @@ -95,6 +94,9 @@ public class BeatmapInfo : IEquatable<BeatmapInfo>, IJsonSerializable, IHasPrima | |||
| public bool WidescreenStoryboard { get; set; } | |||
| public bool EpilepsyWarning { get; set; } | |||
|
|
|||
| public CountdownType Countdown { get; set; } = CountdownType.Normal; | |||
| public int CountdownOffset { get; set; } | |||
There was a problem hiding this comment.
We should probably start xmldoc'ing these. I have no idea what this means, and there doesn't seem to be a usage of it yet which makes it hard to infer meaning. I'm guessing this is the number of... beats to offset the countdown by? Not sure on directionality though.
There was a problem hiding this comment.
Correct, this setting moves the start of the countdown N beats back. I'll add xmldoc to these if they don't end up slated for removal from the db model after all (as per the other comment).
By which I mean the countdown type (currently there's only an on-off bool) as well as the offset. For reference, see wiki.
PRing this separately due to sheer line count (97% of which is the autogenerated database migration).
With regard to the countdown type, I started off with the existing bool + a separate
CountdownSpeed?field, but it was kind of awkward in handling in the decoder and encoder, so in this variant the two are just merged into a singleCountdownType(which is something stable also does).When it comes to the
bool -> intchange ofCountdownon the database side, sqlite doesn't have booleans anyway, so it was anINTEGERall along. It should not be a breaking change, and I've tested that it doesn't break, but if it's considered too risky I'll split back into two fields.All that said I recommend exercising caution and making database backups just in case if someone is to test this. The reason I'm saying that is because I accidentally nuked my local db by switching between the first and second iteration of this pull and forgetting to tear down the migration from v1 before running v2. I would PR a change that makes the game copy out
client.dbto a backup file before nuking if migrations fail on launch, but I'm unsure whether that's wasted effort on EF at this point.