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

osu!direct (basic logic and layout) #805

Merged
merged 72 commits into from May 24, 2017

Conversation

4 participants
@DrabWeb
Contributor

DrabWeb commented May 19, 2017

API integration to come in a follow up PR.

DrabWeb added some commits May 17, 2017

Show outdated Hide outdated osu.Game/Overlays/DirectOverlay.cs
namespace osu.Game.Overlays
{
public class DirectOverlay : WaveOverlayContainer

This comment has been minimized.

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

Is it discussed to be an overlay?
There is an screen for direct existing.

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

Is it discussed to be an overlay?
There is an screen for direct existing.

This comment has been minimized.

@DrabWeb

DrabWeb May 19, 2017

Contributor

It's assumed due to the way that it's displayed, it overlays the content below it instead of moving to a new screen.

@DrabWeb

DrabWeb May 19, 2017

Contributor

It's assumed due to the way that it's displayed, it overlays the content below it instead of moving to a new screen.

Show outdated Hide outdated osu.Game/Overlays/DirectOverlay.cs
{
var p = new List<DirectPanel>();
foreach (BeatmapSetInfo b in BeatmapSets)

This comment has been minimized.

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

linq can be used here

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

linq can be used here

Show outdated Hide outdated osu.Game/osu.Game.csproj
@@ -450,6 +460,7 @@
<ItemGroup />
<ItemGroup />
<ItemGroup />
<ItemGroup />

This comment has been minimized.

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

These empty item groups should be removed.

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

These empty item groups should be removed.

Show outdated Hide outdated osu.Game/Overlays/Direct/FilterControl.cs
public FilterControl()
{
RelativeSizeAxes = Axes.X;
Height = 35 + 32 + 30 + (padding * 2); // search + mode toggle buttons + sort tabs + padding

This comment has been minimized.

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

avoid magic numbers and use sum of consts
why not auto size?

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

avoid magic numbers and use sum of consts
why not auto size?

Show outdated Hide outdated osu.Game/Overlays/Direct/FilterControl.cs
Origin = Anchor.TopRight,
Spacing = new Vector2(10f, 0f),
Direction = FillDirection.Horizontal,
Margin = new MarginPadding { Top = Height - 25 - padding, Right = DirectOverlay.WIDTH_PADDING },

This comment has been minimized.

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

magic number here

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

magic number here

Show outdated Hide outdated osu.Game/Overlays/Direct/DirectListPanel.cs
protected override void OnHoverLost(InputState state)
{
icon.ScaleTo(1, 500, EasingTypes.OutElastic);

This comment has been minimized.

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

uniform 1 and 1f

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

uniform 1 and 1f

Show outdated Hide outdated osu.Game/Overlays/Direct/DirectListPanel.cs
Anchor = Anchor.TopRight,
Origin = Anchor.TopRight,
TextSize = 14,
Alpha = SetInfo.Metadata.Source == @"" ? 0 : 1,

This comment has been minimized.

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

string.IsNullOrEmpty (and other places)

@huoyaoyuan

huoyaoyuan May 19, 2017

Contributor

string.IsNullOrEmpty (and other places)

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy May 20, 2017

Member

I think there may be one or two more locations you can apply the ReverseDepthFillFlowContainer to by the way. I remember adding one recently.

Member

peppy commented May 20, 2017

I think there may be one or two more locations you can apply the ReverseDepthFillFlowContainer to by the way. I remember adding one recently.

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy May 20, 2017

Member

On a visual inspection:

  • Ruleset icons should be either toggle or radio selection style. Let's go with radio for now (one selected maximum at a time). Actually, thinking about this they may not even be required any more as we have the global mode selector in the toolbar, but let's go with radio for now 🚢 .
  • The textbox should maintain focus.
  • We'll need to see how the non-anchoring search area goes with more results. It may make sense to anchor it in a similar way to the settings search area.

Will go through the code soon, this was just from checking VisualTest. Looking pretty good otherwise.

Member

peppy commented May 20, 2017

On a visual inspection:

  • Ruleset icons should be either toggle or radio selection style. Let's go with radio for now (one selected maximum at a time). Actually, thinking about this they may not even be required any more as we have the global mode selector in the toolbar, but let's go with radio for now 🚢 .
  • The textbox should maintain focus.
  • We'll need to see how the non-anchoring search area goes with more results. It may make sense to anchor it in a similar way to the settings search area.

Will go through the code soon, this was just from checking VisualTest. Looking pretty good otherwise.

Show outdated Hide outdated osu.Game/Overlays/Direct/DirectGridPanel.cs
RelativeSizeAxes = Axes.X,
AutoSizeAxes = Axes.Y,
Direction = FillDirection.Vertical,
Padding = new MarginPadding { Top = vertical_padding, Bottom = vertical_padding, Left = horizontal_padding, Right = horizontal_padding },

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

i'd expand this out to multiple lines, getting too long for single-line

@peppy

peppy May 22, 2017

Member

i'd expand this out to multiple lines, getting too long for single-line

Show outdated Hide outdated osu.Game/Overlays/Direct/FilterControl.cs
public ResultCounts ResultCounts
{
get { return resultCounts; }
set { resultCounts = value; updateResultCounts(); }

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

split this out into multiple lines

@peppy

peppy May 22, 2017

Member

split this out into multiple lines

Show outdated Hide outdated osu.Game/Overlays/Direct/DirectPanel.cs
return new AsyncLoadWrapper(new Sprite
{
FillMode = FillMode.Fill,
Texture = new OnlineWorkingBeatmap(SetInfo.Beatmaps.FirstOrDefault(), textures, null).Background,

This comment has been minimized.

@peppy

peppy May 22, 2017

Member

This is evaluating GetBackground outside an async context. Check out BeatmapBackgroundSprite instead.

@peppy

peppy May 22, 2017

Member

This is evaluating GetBackground outside an async context. Check out BeatmapBackgroundSprite instead.

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy May 23, 2017

Member

This is going to be a minor pain to implement, but right now it looks really bad when you scroll through results. The counts should scroll along with the panels:

image

Member

peppy commented May 23, 2017

This is going to be a minor pain to implement, but right now it looks really bad when you scroll through results. The counts should scroll along with the panels:

image

Show outdated Hide outdated osu.Game/Overlays/Direct/DirectListPanel.cs
{
new OsuSpriteText
{
Text = @"mapped by ",

This comment has been minimized.

@peppy

peppy May 24, 2017

Member

remove @ prefix from any strings which need future localisation

@peppy

peppy May 24, 2017

Member

remove @ prefix from any strings which need future localisation

This comment has been minimized.

@smoogipoo

smoogipoo May 24, 2017

Contributor

Would avoid using them at all for now in general.

@smoogipoo

smoogipoo May 24, 2017

Contributor

Would avoid using them at all for now in general.

This comment has been minimized.

@peppy

peppy May 24, 2017

Member

we should be using them, so when we get to adding localisation it can be added automatically

@peppy

peppy May 24, 2017

Member

we should be using them, so when we get to adding localisation it can be added automatically

public FilterControl()
{
RelativeSizeAxes = Axes.X;
Height = HEIGHT;

This comment has been minimized.

@peppy

peppy May 24, 2017

Member

is there a reason we can't autosize this?

@peppy

peppy May 24, 2017

Member

is there a reason we can't autosize this?

This comment has been minimized.

@peppy

peppy May 24, 2017

Member

oh, the dropdown?

@peppy

peppy May 24, 2017

Member

oh, the dropdown?

Show outdated Hide outdated osu.Game/Overlays/Direct/FilterControl.cs
}
}
public class ResultCounts

This comment has been minimized.

@peppy

peppy May 24, 2017

Member

either move these to their own file or nest inside class (we're trying to maintain one object per file)

@peppy

peppy May 24, 2017

Member

either move these to their own file or nest inside class (we're trying to maintain one object per file)

@peppy

peppy approved these changes May 24, 2017

@peppy peppy merged commit d0e280d 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