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

New footer design implementation and rewrite of FooterButton class #20148

Closed
wants to merge 27 commits into from

Conversation

mk56-spn
Copy link
Contributor

@mk56-spn mk56-spn commented Sep 5, 2022

Tried to stick as best as i could to the designs

matched the animations and such with ShearedButton.cs for consistency and rewrote the code to more closely track with that.

Sample Vid:

2022-09-05.17-36-53.mov

will do the back button and Logo replacement seperately since they are more finnicky and lots of screens depend on them

@peppy
Copy link
Member

peppy commented Sep 5, 2022

The metrics seem really off from the design. Is there a reason for that? (ie. font size, icon size, bottom colour bar size).

@mk56-spn
Copy link
Contributor Author

mk56-spn commented Sep 5, 2022

The metrics seem really off from the design. Is there a reason for that?

some i changed purposefully because they didn't match the sized you would expect from the design if i took the values at face value, especially the text , the base button width, etc..

i tested at 1x scale on a 1080 p screen and both are in pixels i assume so im not sure why that could be. if you want i can match them exactly

@Theighlin
Copy link

@mk56-spn have you read #17940?

@mk56-spn
Copy link
Contributor Author

mk56-spn commented Sep 5, 2022

@mk56-spn have you read #17940?

nope, this explains so much, ill adjust accordingly

@peppy
Copy link
Member

peppy commented Sep 5, 2022

some i changed purposefully because they didn't match the sized you would expect from the design if i took the values at face value, especially the text , the base button width, etc..

I'd expect them to match visually if that's the case, but they don't.

Copy link
Member

@Joehuu Joehuu left a comment

Choose a reason for hiding this comment

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

FA 6 renames a lot of the icons. The links I gave in the discussion show aliases from 5.

osu.Game/Screens/Select/FooterButtonMods.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/FooterButtonRandom.cs Outdated Show resolved Hide resolved
@mk56-spn
Copy link
Contributor Author

mk56-spn commented Sep 5, 2022

some i changed purposefully because they didn't match the sized you would expect from the design if i took the values at face value, especially the text , the base button width, etc..

I'd expect them to match visually if that's the case, but they don't.

i apparently had the shear set up incorrectly which was causing a lot of the issues i saw when working on it

one thing, ive resized the bar because its 60 px tall in this iteration of design are you ok with this looking as such:
unaligned
shall i resize back to 50 for now or just make sure to get the back button pr done before this needs merging?

@Stedoss
Copy link
Contributor

Stedoss commented Sep 5, 2022

Due to the refactor to FooterButton, the Rewind action (right click on the FooterButtonRandom button) behaves a little strange in relation to animations. For example, right clicking on the button will not call the base OnMouseUp() function and won't scale back to the original size because of this early return:

protected override void OnMouseUp(MouseUpEvent e)
{
if (e.Button == MouseButton.Right)
{
rewindSearch = true;
TriggerClick();
return;
}
base.OnMouseUp(e);
}

Also of note, it seems that in the FooterButtonRandom class, the OnPressed() and OnReleased() don't call their base implementations, which means that using F2 to trigger the random button does not play the scaling animations.

@peppy peppy self-requested a review September 6, 2022 04:07
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

In addition to other things pointed out, just an initial pass.

osu.Game/Screens/Select/FooterButton.cs Outdated Show resolved Hide resolved
Comment on lines 78 to 79
Offset = new Vector2(.250f, 2),
Colour = Colour4.Black.Opacity(.25f),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Offset = new Vector2(.250f, 2),
Colour = Colour4.Black.Opacity(.25f),
Offset = new Vector2(0.25f, 2),
Colour = Colour4.Black.Opacity(0.25f),

Copy link
Member

Choose a reason for hiding this comment

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

Also interested to know where these values came from. Just visual ballparking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep because the figma shadow value gave me a shadow way stronger than that shown in the design, again sorry for all these mistakes , where should i source this stuff from for future reference ?

osu.Game/Screens/Select/FooterButton.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/FooterButton.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/FooterButton.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/FooterButton.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/FooterButton.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/FooterButtonMods.cs Outdated Show resolved Hide resolved
@peppy
Copy link
Member

peppy commented Sep 6, 2022

The tweening and animation style is completely wrong with this change, but I can fix that once the other issues are fixed. Maybe. For now, please remove the scaling of the buttons during mouse down completely. And change the buttons to be anchored to the bottom of the parent, not the top.

@arflyte please advise how you imagined mods showing in the footer, if at all. this ad-hoc implementation looks very bad:

osu.2022-09-06.at.04.18.56.mp4

@mk56-spn
Copy link
Contributor Author

mk56-spn commented Sep 6, 2022

The tweening and animation style is completely wrong with this change, but I can fix that once the other issues are fixed. Maybe. For now, please remove the scaling of the buttons during mouse down completely. And change the buttons to be anchored to the bottom of the parent, not the top.

@arflyte please advise how you imagined mods showing in the footer, if at all. this ad-hoc implementation looks very bad:

osu.2022-09-06.at.04.18.56.mp4

Waiting on the answer to this since I've restructured the nesting hierarchy for drawables ( it was awfully bloated I've come to realize) and I need to know if I can do away with the mods box before I continue

@arflyte
Copy link

arflyte commented Sep 7, 2022

That is definitely not the way to display selected mods. There's supposed to be a floating indicator on top of the button, but I haven't designed that yet.

Place the selected mod floating on top of the button for now, until I work on its fix.

@peppy
Copy link
Member

peppy commented Sep 28, 2022

Lol

osu! 2022-09-28 at 08 27 17

I don't even know what to do with this PR. I think it's probably blocked until @arflyte provides a proper design.

@mk56-spn
Copy link
Contributor Author

Lol

osu! 2022-09-28 at 08 27 17

I don't even know what to do with this PR. I think it's probably blocked until @arflyte provides a proper design.

well i was told to put it up there in my defense i agree that that overlap sucks,

@peppy peppy added the blocked Don't merge this. label Sep 28, 2022
@mk56-spn
Copy link
Contributor Author

mk56-spn commented Oct 31, 2022

if possible I'd love a quick review of my latest two commits, think the structuring of the drawable is much better now and I removed all the margin jankiness i'd done due to lack of knowledge. Not pressing at all since this is blocked for now regardless, just would be neat.

shear centering should also be better

@peppy
Copy link
Member

peppy commented Nov 1, 2022

@arflyte has there been any progress around this design?

@peppy
Copy link
Member

peppy commented Nov 23, 2022

@mk56-spn there's been some design work on the mod display it would seem (https://www.figma.com/file/DXKwqZhD5yyb1igc3mKo1P/Song-Select-Draft-2?node-id=0%3A1&t=bRPeY78djO0JkLzS-0, bottom left)

Safari 2022-11-23 at 09 24 40

That said, it's probably going to require a bit of implementation detail so I'd argue that the display should just be removed from this PR completely and added as a separate effort.

@peppy
Copy link
Member

peppy commented Nov 23, 2022

Needs non-trivial conflict resolution.

@mk56-spn
Copy link
Contributor Author

That said, it's probably going to require a bit of implementation detail so I'd argue that the display should just be removed from this PR completely and added as a separate effort.

Sounds good. Fwiw I'm willing to do this part as well. But yeah I'd rather get practice mode and the other bits I have in my pipeline out so I'll put this secondary effort on the backburner If thats ok

@mk56-spn
Copy link
Contributor Author

not for review currently, adressing that rather horrid conflict

@peppy
Copy link
Member

peppy commented Dec 1, 2022

@arflyte still missing design

osu! 2022-12-01 at 06 44 46

what do

@mk56-spn
Copy link
Contributor Author

mk56-spn commented Dec 1, 2022

@arflyte still missing design

osu! 2022-12-01 at 06 44 46

what do

i can make a semi fitting placeholder change for them if you want

in other words replace their current colours with B6/ B5 background colour, move the colour itself to a thin bar on one of the sides , add some corner radius rounding, a bit of spacing between them , make them less tall and shift them up so they dont collide with the new buttons.

and probably match the shear

@peppy
Copy link
Member

peppy commented Dec 1, 2022

I honestly think you should implement your new buttons in complete isolation. In a test scene. So you don't have to remove the multiplier display code and wait for all other design elements to be completed.

Trying to immediately slot this in amongst many other surrounding elements is, as we have seen from this PR, not a good idea.

You're welcome to suffix your component with a V2 as has been done elsewhere. Opening in a new PR is also fine if it cleans up the history.

@mk56-spn
Copy link
Contributor Author

mk56-spn commented Dec 1, 2022

yep ive upped my commit practices since making this and i totally agree this PR looks rather filthy, gonna close this in a bit in favour of a clean branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Small overlap between buttons at the bottom of song select
7 participants