Apply combo offsets "colour hax" only on beatmap skins

[frenzibyte]

Closes #8282
Supersedes #8383

• Depends on Refactor combo colours retrieval to use skin config lookups instead #12682

[smoogipoo]

I really don't like this addition of a completely separate "legacy" pathway for not just a combo offset but the combo index in its entirety. I say "legacy" because this doesn't look legacy at all, but the standard for osu! hitobjects. This is the first time we've added legacy things to non-convert hitobjects too.

Furthermore, there's no linking between the "real" combo index and the "legacy" combo index. What happens when the editor changes the combo index? Does it save correctly? Is the editor still going to allow setting a combo offset?

This needs more encapsulation, because it's going to be very prone to failures as it is. I have no suggestions yet because this is a very complex interaction.

[frenzibyte]

I really don't like this addition of a completely separate "legacy" pathway for not just a combo offset but the combo index in its entirety. I say "legacy" because this doesn't look legacy at all, but the standard for osu! hitobjects. This is the first time we've added legacy things to non-convert hitobjects too.

Alternatively, I could add the offset back along with an index to follow for it if you think it's fine, it's just that I'm not sure it should return as their usual names, and what the special index would be named.

I could add another interface IHasBeatmapSkinComboInformation if that'll make more sense.

Furthermore, there's no linking between the "real" combo index and the "legacy" combo index. What happens when the editor changes the combo index? Does it save correctly? Is the editor still going to allow setting a combo offset?

The "real" combo index and "legacy" combo index are kept pretty internal away from the editor or anything else, the editor should only have access to changing the combo offset, if the edited beatmap is a legacy beatmap in the current case.

This needs more encapsulation, because it's going to be very prone to failures as it is. I have no suggestions yet because this is a very complex interaction.

From which side do you think it's going to be prone to failures? I've encapsulated this as much as keeping it away from anything, just an interface that objects can have it assigned to them for the LegacyBeatmapSkin to internally use combo colours on the special offset combo indices instead.

[frenzibyte]

@smoogipoo since you don't like the direction this PR is currently heading, shall I instead close it and leave the underlying issue for you to resolve when you have the time for it? or do you see this potentially being mergeable with some changes and I should leave it open?

[smoogipoo]

I've been waiting for @peppy to take a look into this PR and see if he has any better suggestions. Fine to keep open until then.

[peppy]

@frenzibyte want to bring this up-to-date if that's not too much trouble? i'll review it tomorrow.

[frenzibyte]

Conflicts were straightforward, all bumped to latest master now.

[peppy]

I've made some minor refactors and renames to this, and I think it's in a state I'm happy to move forward with.

@smoogipoo the point you made about this potentially falling apart when support is added to the editor still stands, but I think this may be able to be resolved using a simple bindable flow. I don't think it's important for the time being. Can you take a 5 minute read through this PR's current state and make sure you're willing to turn a blind eye for the time being?

[smoogipoo]

It looks fine.

[peppy]

I don't believe this is working as expected, checking with the original issue's beatmap:

This looks wrong, right?

https://dev.ppy.sh/beatmapsets/235093#osu/544994

Is actually broken?

[frenzibyte]

Turns out there was this hidden logic in HitObject that binds the ComboIndexBindable of the nested objects to itself, while one doesn't exist for ComboIndexWithOffsets and therefore the nested objects of the slider have the wrong colours.

Will fix and bundle that case with the existing test cases.