Allow LN tail to have an origin of TopCentre
#27726
Conversation
|
Skin for testing (rename to osk): R Skin v3.1 (7k Fade LN).zip |
|
Also, is there a chance that this will get backported to stable, like the new |
That's off-topic in this PR, stable is generally feature-locked so it's unlikely that such a feature will be backported, but that should not be discussed in here anyway. |
The name `Top` and `Bottom` does not make sense in combination with `ScrollingDirection` which already flips anchor/origin vertically.
There was a problem hiding this comment.
From a code perspective, this looks acceptable to me after few tidy-ups. However, I'm not confident enough to merge and have this setting exist by myself. I'll leave that to @peppy considering this approach was proposed by him.
| 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; | ||
| } |
There was a problem hiding this comment.
Any reason this needs to be bindable flow when it's only ever changed in one place, and not bound to?
There was a problem hiding this comment.
Regular variable is probably fine, this is only used in DrawableHoldNote(Tail) anyway
There was a problem hiding this comment.
I just realized they're used in tests also, will try to find a way around it.
|
|
||
| namespace osu.Game.Skinning | ||
| { | ||
| public enum HoldNoteTailOrigin |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
longnguyen2004
force-pushed
the
hold-note-tail-topcentre
branch
from
fcf942c
to
3b279fd
Compare
|
Blocked pending #27773 merge / discussion. |
|
Since #27773 is now scrapped, how should this PR be progressed? Do we wait for the 2nd iteration of the new skinning format? |
I'm not sure it's required, but it should arrive this week so stay tuned. |
Originally discussed at #26470
This PR adds a new
skin.inisettingHoldNoteTailOrigin, with a value of 0 for Bottom and 1 for Top, which allows the user to adjust the origin of the tail piece, avoiding the huge texture hack found in current skins.(Should the setting be an array instead? Each column is independent, so theoretically it should be an array, but I'm not aware of any skins that mix and match different textures for each column)
Demo
topcentre.ln.tail.mp4
(Ignore the overlapped part, I didn't blank out the bottom half of the tail)