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

Menu flashes #812

Merged
merged 16 commits into from May 24, 2017

Conversation

3 participants
@ColdVolcano
Contributor

ColdVolcano commented May 20, 2017

Prereqs:

@smoogipoo smoogipoo requested review from smoogipoo and removed request for smoogipoo May 22, 2017

@smoogipoo

This comment has been minimized.

Show comment
Hide comment
@smoogipoo

smoogipoo May 22, 2017

Contributor

Please merge #828 into here and update this with the latest framework master.

Contributor

smoogipoo commented May 22, 2017

Please merge #828 into here and update this with the latest framework master.

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
Width = 300,
Alpha = 0,
BlendingMode = BlendingMode.Additive,
ColourInfo = ColourInfo.GradientHorizontal(new Color4(255, 255, 255, box_max_alpha), Color4.Transparent),

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Using transparent black here looks much better (new Color4(0, 0, 0, 0)). Would also suggest putting TransparentBlack into OsuColour as I foresee it being used quite a bit.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Using transparent black here looks much better (new Color4(0, 0, 0, 0)). Would also suggest putting TransparentBlack into OsuColour as I foresee it being used quite a bit.

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
[BackgroundDependencyLoader]
private void load(OsuGameBase game)
{
beatmap = game.Beatmap;

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Don't use game's bindable like this. Create a local private one and do beatmap.BindTo(game.Beatmap).

@smoogipoo

smoogipoo May 22, 2017

Contributor

Don't use game's bindable like this. Create a local private one and do beatmap.BindTo(game.Beatmap).

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

weird part is that a local one already exists. make that one readonly and do ^

@peppy

peppy May 22, 2017

Member

weird part is that a local one already exists. make that one readonly and do ^

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
protected override void OnNewBeat(int newBeat, double beatLength, TimeSignatures timeSignature, bool kiai)
{
if (!beatmap?.Value?.Track?.IsRunning ?? false)

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2017

Contributor

I don't think this is ever going to be true, given the logic inside BeatSyncedContainer. You wouldn't want to do it this way anyway though - if you need to speed up the fade out it's better to bind to beatmap.ValueChanged.

@smoogipoo

smoogipoo May 22, 2017

Contributor

I don't think this is ever going to be true, given the logic inside BeatSyncedContainer. You wouldn't want to do it this way anyway though - if you need to speed up the fade out it's better to bind to beatmap.ValueChanged.

This comment has been minimized.

@ColdVolcano

ColdVolcano May 22, 2017

Contributor

I changed many things locally (why aren't they pushed?)

@ColdVolcano

ColdVolcano May 22, 2017

Contributor

I changed many things locally (why aren't they pushed?)

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
leftBox.FadeOut(50);
rightBox.FadeOut(50);
}
else if (newBeat >= 0)

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2017

Contributor

With the above comment applied this can be inverted.

@smoogipoo

smoogipoo May 22, 2017

Contributor

With the above comment applied this can be inverted.

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
TrackAmplitudes amp = beatmap.Value.Track.PeakAmplitudes;
bool nextIsLeft = newBeat % 2 == 0;
if (kiai ? nextIsLeft : newBeat % (int)timeSignature == 0)

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Can you combine this with nextIsLeft? It also seems like kiai mode should be a special "blinding mode" that always gets full brightness regardless of amplitude.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Can you combine this with nextIsLeft? It also seems like kiai mode should be a special "blinding mode" that always gets full brightness regardless of amplitude.

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
if (kiai ? nextIsLeft : newBeat % (int)timeSignature == 0)
{
leftBox.ClearTransforms();
leftBox.FadeTo(Math.Max(0, (amp.LeftChannel - amplitude_dead_zone) / alpha_multiplier), 65);

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Isn't this going to be > 1 if amp.LeftChannel = 1? Is this intended?

@smoogipoo

smoogipoo May 22, 2017

Contributor

Isn't this going to be > 1 if amp.LeftChannel = 1? Is this intended?

This comment has been minimized.

@ColdVolcano

ColdVolcano May 22, 2017

Contributor

in fact it is 0.55

@ColdVolcano

ColdVolcano May 22, 2017

Contributor

in fact it is 0.55

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
using (leftBox.BeginDelayedSequence(box_fade_in_time))
leftBox.FadeOut(beatLength, EasingTypes.In);
leftBox.DelayReset();
}

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Newline after brace.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Newline after brace.

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
protected override void OnNewBeat(int newBeat, double beatLength, TimeSignatures timeSignature, bool kiai)
{
if (newBeat < 0)
return;

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Newline after return.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Newline after return.

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
if (newBeat < 0)
return;
TrackAmplitudes amp = beatmap.Value.Track.CurrentAmplitudes;
if (newBeat % (kiai ? 2 : (int)timeSignature) == 0)

This comment has been minimized.

@smoogipoo

smoogipoo May 22, 2017

Contributor

Can this be if (kiai ? newBeat % 2 == 0 : newBeat % (int)timeSignature == 0)? Just want to keep it consistent with the other if statement

@smoogipoo

smoogipoo May 22, 2017

Contributor

Can this be if (kiai ? newBeat % 2 == 0 : newBeat % (int)timeSignature == 0)? Just want to keep it consistent with the other if statement

@smoogipoo

This comment has been minimized.

Show comment
Hide comment
@smoogipoo

smoogipoo May 22, 2017

Contributor

Looks good after requested changes.

Contributor

smoogipoo commented May 22, 2017

Looks good after requested changes.

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
Anchor = Anchor.CentreLeft,
Origin = Anchor.CentreLeft,
RelativeSizeAxes = Axes.Y,
Width = 300,

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

constant

@peppy

peppy May 22, 2017

Member

constant

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
Width = 300,
Alpha = 0,
BlendingMode = BlendingMode.Additive,
ColourInfo = ColourInfo.GradientHorizontal(new Color4(255, 255, 255, box_max_alpha), Color4.Black.Opacity(0)),

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

make statics for each of the two colours as they are used in two places.

private static readonly gradient_bright = ...
private static readonly gradient_dark = ...
@peppy

peppy May 22, 2017

Member

make statics for each of the two colours as they are used in two places.

private static readonly gradient_bright = ...
private static readonly gradient_dark = ...
Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
TrackAmplitudes amp = beatmap.Value.Track.CurrentAmplitudes;
if (kiai ? newBeat % 2 == 0 : newBeat % (int)timeSignature == 0)
{
leftBox.ClearTransforms();

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

find a way to not duplicate this code twice. hint: a method.

@peppy

peppy May 22, 2017

Member

find a way to not duplicate this code twice. hint: a method.

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
if (kiai ? newBeat % 2 == 1 : newBeat % (int)timeSignature == 0)
{
rightBox.ClearTransforms();
rightBox.FadeTo(Math.Max(0, (amp.RightChannel - amplitude_dead_zone) / (kiai ? kiai_multiplier : alpha_multiplier)), 65);

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

65 should be a constant

@peppy

peppy May 22, 2017

Member

65 should be a constant

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
rightBox.FadeTo(Math.Max(0, (amp.RightChannel - amplitude_dead_zone) / (kiai ? kiai_multiplier : alpha_multiplier)), 65);
using (rightBox.BeginDelayedSequence(box_fade_in_time))
rightBox.FadeOut(beatLength, EasingTypes.In);
rightBox.DelayReset();

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

this line is unnecessary

@peppy

peppy May 22, 2017

Member

this line is unnecessary

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
if (kiai ? newBeat % 2 == 1 : newBeat % (int)timeSignature == 0)
{
rightBox.ClearTransforms();

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

this line is unnecessary

@peppy

peppy May 22, 2017

Member

this line is unnecessary

Show outdated Hide outdated osu.Game/Screens/Menu/MenuSideFlashes.cs
if (newBeat < 0)
return;
TrackAmplitudes amp = beatmap.Value.Track.CurrentAmplitudes;

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

why not build this in to the beatsync container? it feels so wrong to access beatmap here when nothing else relies on beatmap

@peppy

peppy May 22, 2017

Member

why not build this in to the beatsync container? it feels so wrong to access beatmap here when nothing else relies on beatmap

This comment has been minimized.

@ColdVolcano

ColdVolcano May 22, 2017

Contributor

@smoogipooo

@ColdVolcano

ColdVolcano May 22, 2017

Contributor

@smoogipooo

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy May 23, 2017

Member

Let's not merge this until #831 is ready.

Member

peppy commented May 23, 2017

Let's not merge this until #831 is ready.

@smoogipoo

This comment has been minimized.

Show comment
Hide comment
@smoogipoo

smoogipoo May 23, 2017

Contributor

Looks better 👍

Contributor

smoogipoo commented May 23, 2017

Looks better 👍

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy May 23, 2017

Member

Don't have permission to push to remote, so can't fix this.

Please

  • Resolve conflicts
  • Merge upstream master
  • Fix method signature changes in BeatSyncedContainer
Member

peppy commented May 23, 2017

Don't have permission to push to remote, so can't fix this.

Please

  • Resolve conflicts
  • Merge upstream master
  • Fix method signature changes in BeatSyncedContainer
@peppy

as cited

@peppy

peppy approved these changes May 24, 2017

@peppy peppy merged commit 3af7f89 into ppy:master May 24, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment