New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ComboColour from HitObjects #2252

Merged
merged 7 commits into from Mar 22, 2018

Conversation

2 participants
@peppy
Member

peppy commented Mar 20, 2018

Prerequisite step to having combo colours provided by external sources (skins, beatmap).

Prepares AccentColour setters to correctly update DrawableHitObject pieces.

Note that merging this will remove combo colour assignment temporarily.

@peppy peppy added this to the March 2018 milestone Mar 20, 2018

@peppy peppy referenced this pull request Mar 20, 2018

Merged

Add skin/beatmap lookup hierarchy #2254

3 of 3 tasks complete
/// <summary>
/// The offset of this hitobject in the current combo.
/// </summary>
int ComboIndex { get; set; }

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

Why is this property duplicated? (xmldoc)

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

If this is the index of the combo as a whole in the beatmap, this should be in IHasCombo

This comment has been minimized.

@peppy

peppy Mar 22, 2018

Member

hmm, I split these out based on usage. basically there are many places we mark objects as supporting combo, but don't require any knowledge beyond the bool NewCombo.

would renaming this class to IHasComboInformation help?

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

I don't see any situation where you'd want to know only of NewCombo and nothing else. There's not enough data there to tell you anything.

Example?

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Mar 22, 2018

half-approving this one, until I check further PRs

@peppy peppy merged commit 894575e into ppy:master Mar 22, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@peppy peppy deleted the peppy:accent-colour-properties branch Jun 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment