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

Add skin/beatmap lookup hierarchy #2254

Merged
merged 12 commits into from Mar 22, 2018

Conversation

2 participants
@peppy
Member

peppy commented Mar 20, 2018

Adds back combo colours with appropriate overrides and gives fallback conditional a function, amongst other things.

peppy added some commits Mar 20, 2018

Make fallback bool into a function
Allows correct handling now that beatmap skins are also a thing.
Add GetTexture method to ISkinSource
Used to shortcut lookup checks without potentially expensive drawable creation overhead.
SampleChannel GetSample(string sampleName);
Color4? GetComboColour(IHasComboInformation comboObject);

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

I don't really like this being here since not all rulesets/things that use skins are going to use GetComboColour. How about reversing this instead by way of:

  • Add an extension method to IHasComboIndex: Color4 GetComboColour(this IHasComboIndex combo, ISkinSource skin) => ... ?? Color4.White
  • Use if (obj is IHasComboIndex combo) AccentColour = combo.GetComboColour(skin);

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

s/IHasComboIndex/IHasComboInformation

This comment has been minimized.

@peppy

peppy Mar 22, 2018

Member

would you be okay with the parameter of this method just being int index instead?

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

sure

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

Wait I think I misunderstood, of the ISkinSource method?

This comment has been minimized.

@peppy

peppy Mar 22, 2018

Member

yeah, so the interface is not passed to the method, just the index.

i figure we probably want access to these colours from other places, so would rather leave it exposed for now.

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

Give an example where you'd want access to a specific index? For the editor you'll want to have access directly to the configuration, because you need to know all combo colours and not just one index.

HitObjects as discussed.

I can't think of anywhere else where this method may be used for a single index.

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

As discussed. This cannot be an index.

Change this method into Color4? GetCustomColour(string) if you absolutely must not return the skin configuration. Though I do maintain that we'll want T GetConfigValue(string key) in the end.

base.SkinChanged(skin, allowFallback);
if (HitObject is IHasComboInformation combo)
AccentColour = skin.GetColour($"Play/Combo/{combo.ComboIndex}") ?? Color4.White;

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

This shouldn't be at a DrawableHitObject level. Should be re-implemented everywhere it's used for accent colours specifically.

Imagine a hitobject that uses IHasComboInformation but doesn't modify colour - e.g. maybe displays a different animation on the last hitobject in a combo.

This comment has been minimized.

@peppy

peppy Mar 22, 2018

Member

can't we just ignore AccentColour in that case?

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

And if we still use AccentColour to colorise the notes? E.g. mania notes, but maybe having like a super-white-colored note as the last note denoting the end of a combo.

peppy added some commits Mar 22, 2018

@smoogipoo smoogipoo merged commit 61ef635 into ppy:master Mar 22, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@peppy peppy deleted the peppy:skin--completion branch Jun 21, 2018

@Dragicafit Dragicafit referenced this pull request Sep 20, 2018

Open

Add support for skin ComboColours #3460

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