Combo colour displayed incorrectly in editor timeline when reversing selection with one of default skins active

[bdach]

Type

Cosmetic

Bug description

Reproduction steps:

1. Open a catch map in editor
2. Ensure that beatmap has no combo colours defined
3. Select a group of fruits
4. Reverse the selection
5. The combo colours will correctly not change in the composer, but will incorrectly reverse in the timeline (but will fix themselves on deselect)

See video below.

This is happening because combo colour lookup is broken. TimelineHitObjectBlueprint is attempting to subscribe to changes of everything that can change in IHasComboInformation:

osu/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs

Lines 143 to 156 in 21201e6

	case IHasComboInformation comboInfo:
	indexInCurrentComboBindable = comboInfo.IndexInCurrentComboBindable.GetBoundCopy();
	indexInCurrentComboBindable.BindValueChanged(_ =>
	{
	comboIndexText.Text = (indexInCurrentComboBindable.Value + 1).ToString();
	}, true);
	comboIndexBindable = comboInfo.ComboIndexBindable.GetBoundCopy();
	comboIndexWithOffsetsBindable = comboInfo.ComboIndexWithOffsetsBindable.GetBoundCopy();
	comboIndexBindable.BindValueChanged(_ => updateColour());
	comboIndexWithOffsetsBindable.BindValueChanged(_ => updateColour(), true);
	skin.SourceChanged += updateColour;

but catch has this thing:

osu/osu.Game.Rulesets.Catch/Objects/CatchHitObject.cs

Lines 80 to 84 in 21201e6

	public int IndexInBeatmap
	{
	get => indexInBeatmap.Value;
	set => indexInBeatmap.Value = value;
	}

which TimelineHitObjectBlueprint physically cannot be aware of because it's a ruleset-only property.

I find it really weird that this sort of flow was allowed to exist in the first place:

osu/osu.Game/Skinning/SkinComboColourLookup.cs

Lines 10 to 18 in 21201e6

	/// <summary>
	/// The index to use for deciding the combo colour.
	/// </summary>
	public readonly int ColourIndex;
	/// <summary>
	/// The combo information requesting the colour.
	/// </summary>
	public readonly IHasComboInformation Combo;

@frenzibyte your name appears to be on a lot of this in blame so maybe you can look at it. Just please hold your horses with complexity. I'd even advocate for nuking that colour thing and putting that IndexInBeatmap in IHasComboInformation.

Screenshots or videos

2024-03-28.11-41-15.remuxed.mp4

Version

latest master

Logs

n/a

[frenzibyte]

Incorporating IndexInBeatmap in IHasComboInformation feels meh, especially since the logic would have be moved from CatchBeatmapProcessor to UpdateComboInformation and process it as if it's actually "combo information" while it's not. It's possible to implement, but feels quite awkward.

I was curious to check how stable behaves on catch with regards to colours, and so far it appears that stable in practice always assigns a different combo colour for each hit object regardless of any factor, unlike lazer which...incorrectly uses ComboIndexWithOffsets if beatmap skin/colours are enabled. That is a separate issue, however.

Point is, it's probably much saner to define a separate path for fruit hit objects to be assigned colours and reflect that on the timeline. I'm imagining something like this:

// implemented by osu! hit objects solely
public interface IHasColourByComboIndex : IHasComboInformation
{
    // combo index with offsets will exist here because it's purely cosmetic and doesn't make sense to exist in CatchHitObjects.
    Bindable<int> ComboIndexWithOffsetsBindable { get; }
    int ComboIndexWithOffsets { get; set; }

    Color4 GetColour(ISkin skin) => skin.GetHitObjectColour(ComboIndex, ComboIndexWithOffsets);
}

// implemented by catch hit objects solely
public interface IHasColourByIndexInBeatmap
{
    Bindable<int> IndexInBeatmapBindable { get; }
    int IndexInBeatmap { get; set; }

    // assuming colour hax applies to osu!catch, and that index in beatmap is incremented by the legacy "ConvertHitObject.ComboOffset" field.
    Bindable<int> IndexInBeatmapWithOffsetsBindable { get; }
    int IndexInBeatmapWithOffsets { get; set; }

    Color4 GetColour(ISkin skin) => skin.GetHitObjectColour(IndexInBeatmap, IndexInBeatmapWithOffsets);
}

// skin.GetHitObjectColour would be an extension method(?) that takes in two indices, one with colour hax and one without, and performs the lookup with those indices.

I will see where this structure takes me and report back about it, but I'm fine with stepping back at any point and go on with moving IndexInBeatmap to IHasComboInformation to keep everything simple, but I would need confirmation from others that it's a fine step to take for the sake of less complexity.

I was interested to see how stable has the "IndexInBeatmap" part working, but I could not easily find it.