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

Allow LN tail to have an origin of TopCentre #27726

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ namespace osu.Game.Rulesets.Mania.Tests.Skinning
public abstract partial class ManiaSkinnableTestScene : SkinnableTestScene
{
[Cached(Type = typeof(IScrollingInfo))]
private readonly TestScrollingInfo scrollingInfo = new TestScrollingInfo();
protected readonly TestScrollingInfo ScrollingInfo = new TestScrollingInfo();

[Cached]
private readonly StageDefinition stage = new StageDefinition(4);
Expand All @@ -30,7 +30,7 @@ public abstract partial class ManiaSkinnableTestScene : SkinnableTestScene

protected ManiaSkinnableTestScene()
{
scrollingInfo.Direction.Value = ScrollingDirection.Down;
ScrollingInfo.Direction.Value = ScrollingDirection.Down;

Add(new Box
{
Expand All @@ -43,16 +43,16 @@ protected ManiaSkinnableTestScene()
[Test]
public void TestScrollingDown()
{
AddStep("change direction to down", () => scrollingInfo.Direction.Value = ScrollingDirection.Down);
AddStep("change direction to down", () => ScrollingInfo.Direction.Value = ScrollingDirection.Down);
}

[Test]
public void TestScrollingUp()
{
AddStep("change direction to up", () => scrollingInfo.Direction.Value = ScrollingDirection.Up);
AddStep("change direction to up", () => ScrollingInfo.Direction.Value = ScrollingDirection.Up);
}

private class TestScrollingInfo : IScrollingInfo
protected class TestScrollingInfo : IScrollingInfo
{
public readonly Bindable<ScrollingDirection> Direction = new Bindable<ScrollingDirection>();

Expand Down
21 changes: 21 additions & 0 deletions osu.Game.Rulesets.Mania.Tests/Skinning/TestSceneHoldNote.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
using osu.Game.Beatmaps.ControlPoints;
using osu.Game.Rulesets.Mania.Objects;
using osu.Game.Rulesets.Mania.Objects.Drawables;
using osu.Game.Rulesets.UI.Scrolling;
using osu.Game.Skinning;

namespace osu.Game.Rulesets.Mania.Tests.Skinning
{
Expand Down Expand Up @@ -37,6 +39,25 @@ public void TestFadeOnMiss()
});
}

[Test]
public void TestTailOrigin()
{
AddStep("set tail origin to regular", () =>
{
foreach (var holdNote in CreatedDrawables.SelectMany(d => d.ChildrenOfType<DrawableHoldNote>()))
((Bindable<HoldNoteTailOrigin>)holdNote.Tail.TailOrigin).Value = HoldNoteTailOrigin.Regular;
});
AddStep("change direction to down", () => ScrollingInfo.Direction.Value = ScrollingDirection.Down);
AddStep("change direction to up", () => ScrollingInfo.Direction.Value = ScrollingDirection.Up);
AddStep("set tail origin to inverted", () =>
{
foreach (var holdNote in CreatedDrawables.SelectMany(d => d.ChildrenOfType<DrawableHoldNote>()))
((Bindable<HoldNoteTailOrigin>)holdNote.Tail.TailOrigin).Value = HoldNoteTailOrigin.Inverted;
});
AddStep("change direction to down", () => ScrollingInfo.Direction.Value = ScrollingDirection.Down);
AddStep("change direction to up", () => ScrollingInfo.Direction.Value = ScrollingDirection.Up);
}

private IEnumerable<DrawableHoldNote> holdNotes => CreatedDrawables.SelectMany(d => d.ChildrenOfType<DrawableHoldNote>());

protected override DrawableManiaHitObject CreateHitObject()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,12 @@ protected override void Update()
// Position and resize the body to lie half-way under the head and the tail notes.
// The rationale for this is account for heads/tails with corner radius.
bodyPiece.Y = (Direction.Value == ScrollingDirection.Up ? 1 : -1) * Head.Height / 2;
bodyPiece.Height = DrawHeight - Head.Height / 2 + Tail.Height / 2;
bodyPiece.Height = DrawHeight - Head.Height / 2;

if (Tail.TailOrigin.Value == HoldNoteTailOrigin.Inverted)
bodyPiece.Height -= Tail.Height / 2;
else
bodyPiece.Height += Tail.Height / 2;

if (Time.Current >= HitObject.StartTime)
{
Expand Down
34 changes: 34 additions & 0 deletions osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNoteTail.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@

#nullable disable

using osu.Framework.Bindables;
using osu.Framework.Graphics;
using osu.Framework.Input.Events;
using osu.Game.Rulesets.Mania.Skinning;
using osu.Game.Rulesets.Scoring;
using osu.Game.Rulesets.UI.Scrolling;
using osu.Game.Skinning;

namespace osu.Game.Rulesets.Mania.Objects.Drawables
{
Expand All @@ -18,6 +22,10 @@ public partial class DrawableHoldNoteTail : DrawableNote

protected internal DrawableHoldNote HoldNote => (DrawableHoldNote)ParentHitObject;

private readonly Bindable<HoldNoteTailOrigin> tailOrigin = new Bindable<HoldNoteTailOrigin>();

public IBindable<HoldNoteTailOrigin> TailOrigin => tailOrigin;

public DrawableHoldNoteTail()
: this(null)
{
Expand All @@ -30,6 +38,12 @@ public DrawableHoldNoteTail(TailNote tailNote)
Origin = Anchor.TopCentre;
}

protected override void LoadComplete()
{
base.LoadComplete();
tailOrigin.BindValueChanged(_ => updateTailOrigin(), true);
}

public void UpdateResult() => base.UpdateResult(true);

protected override void CheckForResult(bool userTriggered, double timeOffset) =>
Expand All @@ -52,5 +66,25 @@ protected override HitResult GetCappedResult(HitResult result)
public override void OnReleased(KeyBindingReleaseEvent<ManiaAction> e)
{
}

protected override void ApplySkin(ISkinSource skin, bool allowFallback)
{
base.ApplySkin(skin, allowFallback);
tailOrigin.Value = skin.GetConfig<ManiaSkinConfigurationLookup, HoldNoteTailOrigin>(new ManiaSkinConfigurationLookup(LegacyManiaSkinConfigurationLookups.HoldNoteTailOrigin))?.Value ?? HoldNoteTailOrigin.Regular;
}

protected override void OnDirectionChanged(ValueChangedEvent<ScrollingDirection> e)
{
base.OnDirectionChanged(e);
updateTailOrigin();
}

private void updateTailOrigin()
{
if (Direction.Value == ScrollingDirection.Up)
Origin = tailOrigin.Value == HoldNoteTailOrigin.Inverted ? Anchor.BottomCentre : Anchor.TopCentre;
else
Origin = tailOrigin.Value == HoldNoteTailOrigin.Inverted ? Anchor.TopCentre : Anchor.BottomCentre;
}
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this needs to be bindable flow when it's only ever changed in one place, and not bound to?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regular variable is probably fine, this is only used in DrawableHoldNote(Tail) anyway

Copy link
Author

@longnguyen2004 longnguyen2004 Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized they're used in tests also, will try to find a way around it.

}
}
1 change: 0 additions & 1 deletion osu.Game.Rulesets.Mania/Objects/Drawables/DrawableNote.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ protected override void OnApply()
protected override void OnDirectionChanged(ValueChangedEvent<ScrollingDirection> e)
{
base.OnDirectionChanged(e);

headPiece.Anchor = headPiece.Origin = e.NewValue == ScrollingDirection.Up ? Anchor.TopCentre : Anchor.BottomCentre;
}

Expand Down
11 changes: 11 additions & 0 deletions osu.Game/Skinning/HoldNoteTailOrigin.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

namespace osu.Game.Skinning
{
public enum HoldNoteTailOrigin
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the first time we're throwing a per-ruleset enum in this place? Not quite sure about this. Might be better to dump it in LegacyManiaSkinConfigurationLookup.cs?

Or alternatively naming it similar to LegacyNoteBodyStyle, ie. LegacyNoteTailOrigin.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine to me.
Talking about that, I was wondering why there were mania-specific stuff inside osu.Game.Skinning, and not osu.Game.Rulesets.Mania.Skinning. I sometimes got confused when searching for stuff and having to switching between the 2 namespaces. Putting them all in osu.Game.Rulesets.Mania.Skinning would be nice, but there might be other reasons that I don't know.

{
Regular = 0,
Inverted = 1
}
}
1 change: 1 addition & 0 deletions osu.Game/Skinning/LegacyManiaSkinConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public class LegacyManiaSkinConfiguration : IHasCustomColours
public bool KeysUnderNotes;
public int LightFramePerSecond = 60;

public HoldNoteTailOrigin HoldNoteTailOrigin;
public LegacyNoteBodyStyle? NoteBodyStyle;

public LegacyManiaSkinConfiguration(int keys)
Expand Down
1 change: 1 addition & 0 deletions osu.Game/Skinning/LegacyManiaSkinConfigurationLookup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public enum LegacyManiaSkinConfigurationLookups
HoldNoteBodyImage,
HoldNoteLightImage,
HoldNoteLightScale,
HoldNoteTailOrigin,
WidthForNoteHeightScale,
ExplosionImage,
ExplosionScale,
Expand Down
5 changes: 5 additions & 0 deletions osu.Game/Skinning/LegacyManiaSkinDecoder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ private void flushPendingLines()
currentConfig.LightFramePerSecond = lightFramePerSecond > 0 ? lightFramePerSecond : 24;
break;

case "HoldNoteTailOrigin":
if (Enum.TryParse<HoldNoteTailOrigin>(pair.Value, out var tailOrigin))
currentConfig.HoldNoteTailOrigin = tailOrigin;
break;

case string when pair.Key.StartsWith("Colour", StringComparison.Ordinal):
HandleColours(currentConfig, line, true);
break;
Expand Down
3 changes: 3 additions & 0 deletions osu.Game/Skinning/LegacySkin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,9 @@ protected override void ParseConfigurationStream(Stream stream)

return SkinUtils.As<TValue>(new Bindable<float>(existing.ColumnWidth[maniaLookup.ColumnIndex.Value] / LegacyManiaSkinConfiguration.DEFAULT_COLUMN_SIZE));

case LegacyManiaSkinConfigurationLookups.HoldNoteTailOrigin:
return SkinUtils.As<TValue>(new Bindable<HoldNoteTailOrigin>(existing.HoldNoteTailOrigin));

case LegacyManiaSkinConfigurationLookups.KeyImage:
Debug.Assert(maniaLookup.ColumnIndex != null);
return SkinUtils.As<TValue>(getManiaImage(existing, $"KeyImage{maniaLookup.ColumnIndex}"));
Expand Down
Loading