Skip to content
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 support for hit judgements #2181

Merged
merged 10 commits into from Mar 13, 2018

Conversation

2 participants
@peppy
Copy link
Member

peppy commented Mar 7, 2018

Also makes sizing of skinned drawables more flexible.

@peppy peppy added the skinning label Mar 7, 2018

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

@peppy peppy added the pending review label Mar 8, 2018

@@ -21,22 +22,42 @@ public class SkinnableDrawable<T> : SkinReloadableDrawable

private readonly string componentName;

public SkinnableDrawable(string name, Func<string, T> defaultImplementation, bool fallback = true) : base(fallback)
/// <summary>
/// Whether a user-skin drawable should be limited to the size of our parent.

This comment has been minimized.

@smoogipoo

smoogipoo Mar 12, 2018

Contributor

This xmldoc should be on the ctor parameter as well. In-fact all the ctor parameters should have xmldocs.

legacyName = "hit100";
break;
case HitResult.Great:
legacyName = "hit300";

This comment has been minimized.

@smoogipoo

smoogipoo Mar 12, 2018

Contributor

I don't like that we're polluting osu.Game with this stuff. This should be done individually for each legacy ruleset imo.

break;
}

Child = new SkinnableDrawable($"Play/osu/{legacyName}", _ => JudgementText = new OsuSpriteText

This comment has been minimized.

@smoogipoo

smoogipoo Mar 12, 2018

Contributor

How about instead exposing a virtual string GetResourceName(HitResult result) => "Play/osu/{result.ToString().ToLower()";? Two reasons:

  1. Eventually we're not going to have Play/osu/. When we move these resources to each gamemode they should exist as simply Play/.
  2. Will allow us to override this lookup further depending on additional construction parameters (eg, per-key judgements in mania).
@@ -48,7 +43,8 @@ private class LegacySkinResourceStore : IResourceStore<byte[]>
private readonly IResourceStore<byte[]> underlyingStore;

private string getPathForFile(string filename) =>
skin.Files.FirstOrDefault(f => string.Equals(Path.GetFileNameWithoutExtension(f.Filename), filename.Split('/').Last(), StringComparison.InvariantCultureIgnoreCase))?.FileInfo.StoragePath;
skin.Files.FirstOrDefault(f => string.Equals(Path.GetFileNameWithoutExtension(f.Filename), filename.Split('/').Last(), StringComparison.InvariantCultureIgnoreCase))?.FileInfo
.StoragePath;

This comment has been minimized.

@smoogipoo

smoogipoo Mar 12, 2018

Contributor

Put this back on the same line.

@smoogipoo

This comment has been minimized.

Copy link
Contributor

smoogipoo commented Mar 13, 2018

screen shot 2018-03-13 at 9 55 57 am

Known issue? The text size is too small.

@peppy

This comment has been minimized.

Copy link
Member Author

peppy commented Mar 13, 2018

Text size fixed via ppy/osu-framework#1459.

@smoogipoo
Copy link
Contributor

smoogipoo left a comment

looks ok to me

@smoogipoo smoogipoo merged commit f06f31a into ppy:master Mar 13, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@peppy peppy referenced this pull request Mar 13, 2018

Merged

Update framework #2209

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.