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

Split score multiplier and unranked label colours #2314

Merged
merged 13 commits into from May 31, 2018

Conversation

5 participants
@Joehuu
Copy link
Contributor

Joehuu commented Mar 24, 2018

Resolves #2313. Chose yellow for lower multiplier, the color of the diff inc section.
Edit: Chose (Unranked) label to be blue, the same colour as the special section.

@@ -13,7 +13,7 @@ public class OsuModAutopilot : Mod
public override string ShortenedName => "AP";
public override FontAwesome Icon => FontAwesome.fa_osu_mod_autopilot;
public override string Description => @"Automatic cursor movement - just follow the rhythm.";
public override double ScoreMultiplier => 0;
public override double ScoreMultiplier => 1;
public override bool Ranked => false;

This comment has been minimized.

@Aergwyn

Aergwyn Mar 25, 2018

Member

While at it you can remove this line as Ranked is false by default.

}
else
{
RankedLabel.Text = null;

This comment has been minimized.

@Aergwyn

Aergwyn Mar 25, 2018

Member

Unnecessary. RankedLabel.Text will be null at this point.

RankedLabel.Text = null;
if (!ranked)
{
RankedLabel.Text += " (Unranked)";

This comment has been minimized.

@Aergwyn

Aergwyn Mar 25, 2018

Member

= should be enough here to set. Also the leading whitespace could be removed and the label could have an appropriate distance instead.

This comment has been minimized.

@Joehuu

Joehuu Mar 25, 2018

Author Contributor

You said a conflicting review. If I do RankedLabel.Text = "(Unranked)" with no space, it overrides RankedLabel.Text = " " on the top. It's best that I replace only the += with =.

This comment has been minimized.

@Aergwyn

Aergwyn Mar 25, 2018

Member

There is no RankedLabel.Text = " " though? D: Am I blind?

This comment has been minimized.

@Joehuu

Joehuu Mar 25, 2018

Author Contributor

Also the leading whitespace could be removed and the label could have an appropriate distance instead.

Maybe I'm confused. What is this asking exactly?

This comment has been minimized.

@Aergwyn

Aergwyn Mar 25, 2018

Member

Have RankedLabel.Text = "(Unranked)"; and eventually move the label to the right with padding if it doesn't look appropriate.

This comment has been minimized.

@jorolf

jorolf Mar 25, 2018

Contributor

*margin

{
AddAssert("check for ranked", () => !modSelect.MultiplierLabel.Text.EndsWith(unranked_suffix));
AddAssert("check for ranked", () => !modSelect.RankedLabel.Text.EndsWith(unranked_suffix));

This comment has been minimized.

@Aergwyn

Aergwyn Mar 25, 2018

Member

I think we can check for Equals now that it's a dedicated label.

@@ -182,13 +182,13 @@ private void testMultiplierTextColour(Mod mod, Color4 colour)
checkLabelColor(Color4.White);
}

private void testMultiplierTextUnranked(Mod mod)
private void testRankedTextUnranked(Mod mod)

This comment has been minimized.

@Aergwyn

Aergwyn Mar 25, 2018

Member

I'd even suggest naming this method testRankedText now.

@Aergwyn

This comment has been minimized.

Copy link
Member

Aergwyn commented Mar 25, 2018

Chose yellow for lower multiplier, the color of the diff inc section.

Now that you say it. I may have a weird suggestion.
Difficulty decreasing mods are green and difficulty increasing mods are yellow. Shouldn't the multiplier colour reflect that? Right now you click the green mods and get a yellow colour and vice versa.
Maybe it would be interesting to see how it is if you swap it so below 1 is green and above 1 is yellow.

Then again I'm no UX person but the conflicting colours would be still weird imho.

@Joehuu

This comment has been minimized.

Copy link
Contributor Author

Joehuu commented Mar 25, 2018

That would make special mods that decrease confusing. I plan to move the mods on #2250, and to make your idea work, the special section would have to have the decreasing mods moved to the reduction section and (Unranked) will have to be blue. And the sections have to be renamed to Score Decrease and etc. Lastly I have another suggestion, shouldn’t the Score Multiplier: be coloured from the multiplier like on stable?

@Joehuu

This comment has been minimized.

Copy link
Contributor Author

Joehuu commented Mar 25, 2018

Nvm, your suggestion wouldn't make sense if you have a diff increase mod and reduction mod at the same time. That'll result in green(multiplier less than 1.0) with your suggestion. I plan to make (Unranked) blue from special though. And leave the multiplier < 1.0 as red.

@@ -110,14 +119,11 @@ private void updateMods()
RankedLabel.Text = null;

This comment has been minimized.

@Aergwyn

Aergwyn Mar 25, 2018

Member

Crazy idea: Have the label fixed with text/colour and just hide/show it instead of constantly changing it.

This comment has been minimized.

@Joehuu

Joehuu Mar 25, 2018

Author Contributor

How do you do that? Also, having fixed colour on the label won't show the fade animation.
Edit: Fade animation of white transitioning to blue.

This comment has been minimized.

@Aergwyn

Aergwyn Mar 26, 2018

Member

You don't need a colour transition as it'd always be blue.
RankedLabel.FadeIn(200) or RankedLabel.FadeOut(200), 200 being the time it takes to complete.

@Joehuu Joehuu changed the title Score multiplier edits Split score multiplier and unranked label colours Mar 25, 2018

{
Text = @"Score Multiplier: ",

This comment has been minimized.

@jorolf

jorolf Mar 27, 2018

Contributor

why did you move this to updateMods()?

This comment has been minimized.

@Joehuu

Joehuu Mar 27, 2018

Author Contributor

Shouldn't Score Multiplier: change color with the multiplier like on stable?

This comment has been minimized.

@Aergwyn

Aergwyn Mar 27, 2018

Member

No it shouldn't (personal opinion). I think the way it looks in lazer feels cleaner.

This comment has been minimized.

@jorolf

jorolf Mar 27, 2018

Contributor

What does color have to do with the text of a label?

if (multiplier > 1.0)
MultiplierLabel.FadeColour(HighMultiplierColour, 200);
else if (multiplier < 1.0)
MultiplierLabel.FadeColour(LowMultiplierColour, 200);
else
MultiplierLabel.FadeColour(Color4.White, 200);

RankedLabel.Text = "(Unranked)";

This comment has been minimized.

@Aergwyn

Aergwyn Mar 27, 2018

Member

You can set the text at label declaration instead.

MultiplierLabel.Text = $"{multiplier:N2}x";
if (!ranked)
MultiplierLabel.Text += " (Unranked)";
ScoreLabel.Text = "Score Multiplier:";

This comment has been minimized.

@Aergwyn

Aergwyn Mar 27, 2018

Member

This doesn't need to be set with each update. As it's static text at this point you can just set it at label declaration.

@@ -57,6 +59,7 @@ private void load(OsuColour colours, OsuGame osu, RulesetStore rulesets, AudioMa

LowMultiplierColour = colours.Red;
HighMultiplierColour = colours.Green;
RankedColour = colours.Blue;

This comment has been minimized.

@Aergwyn

Aergwyn Mar 27, 2018

Member

The reference is not needed. Just set the colour of the label.

RankedLabel.Text = "(Unranked)";
RankedLabel.FadeColour(RankedColour, 200);
RankedLabel.FadeOut(200);
if (!ranked)

This comment has been minimized.

@Aergwyn

Aergwyn Mar 27, 2018

Member

The FadeColour shouldn't be needed so only

if(ranked)
    RankedLabel.FadeOut(200);
else
    RankedLabel.FadeIn(200);

remains.

{
AddAssert("check for ranked", () => !modSelect.MultiplierLabel.Text.EndsWith(unranked_suffix));
AddAssert("check for ranked", () => !modSelect.RankedLabel.Text.Equals(unranked_suffix));

This comment has been minimized.

@Joehuu

Joehuu Mar 27, 2018

Author Contributor

How do you check for fade?

This comment has been minimized.

@jorolf

jorolf Mar 27, 2018

Contributor

Which "fade" do you mean? Colour or Alpha? Either way you can check the property

Joehuu added some commits Mar 29, 2018

@peppy peppy added regression and removed regression labels Mar 29, 2018

@peppy peppy added this to the Candidate Issues milestone Mar 29, 2018

{
AddAssert("check for ranked", () => !modSelect.MultiplierLabel.Text.EndsWith(unranked_suffix));
AddWaitStep(1, "wait for fade");
AddAssert("check for ranked", () => modSelect.RankedLabel.Alpha.Equals(0));

This comment has been minimized.

@jorolf

jorolf Mar 31, 2018

Contributor

Why didn't you use "==" 🤔

This comment has been minimized.

@Frontear

Frontear Apr 6, 2018

Contributor

Is there a difference, besides obvious syntax?

Joehuu and others added some commits Mar 31, 2018

@peppy

peppy approved these changes May 31, 2018

@peppy peppy merged commit db89b92 into ppy:master May 31, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@Joehuu Joehuu deleted the Joehuu:score-multiplier-edits branch May 31, 2018

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.