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 beatmap information wedge design implementation #22116

Merged
merged 44 commits into from
Sep 6, 2023

Conversation

mk56-spn
Copy link
Contributor

@mk56-spn mk56-spn commented Jan 10, 2023

Another step towards : #18890 being done.

Adds the updated wedge design ( exclusively as a test scene component for now, given it removes a large amount of information, which will be implemented in its own component, as such implementing it directly is unfeasible)

In the file I marked down two issues that are out of my control to fix, namely:

  • The mismatched shadow type on the text ( to my knowledge diffused text shadows are not possible currently, given the text is a sprite instead of an SVG and there is no pre rendered diffused shadow i think)).

  • And the fact that i have as of yet not heard back from Flyte on how to source the colours for the updated wedge background gradient ( https://www.figma.com/file/DXKwqZhD5yyb1igc3mKo1P?node-id=2980:3361#340801912 )

As a final note I plan to do the implementation of the playcount count present in the design separately if possible, due to it seeming to have more implementation detail than I'd expected.

updated2

@bdach
Copy link
Collaborator

bdach commented Jan 10, 2023

A few basic things from just a visual inspection:

  • The stars on the right don't seem to match - design has them centred vertically (and seems to round down to full stars), current implementation has them stuck to top
  • As the background in the test is black, there's no way to even tell how this design will work with actual backgrounds
  • The designs don't seem to have map status anywhere - you seem to have added them in an educated guess and it seems maybe fine, but input from designer-san would be appreciated if possible

@mk56-spn
Copy link
Contributor Author

A few basic things from just a visual inspection:

* The stars on the right don't seem to match - design has them centred vertically (and seems to round down to full stars), current implementation has them stuck to top

* As the background in the test is black, there's no way to even tell how this design will work with actual backgrounds

* The designs don't seem to have map status anywhere - you seem to have added them in an educated guess and it seems maybe fine, but input from designer-san would be appreciated if possible

1: this is due to it being the old star counter design which has small stars going up to 10, instead of the new one which cuts off wherever it needs to, old design was left in after frenzy mentioned not being a big fan https://discord.com/channels/188630481301012481/188630652340404224/1049958910720737310 till further discussion is had.

2: black background isnt ideal i agree. will look into

3: yep, but i asked ppy on discord and he said if something is already there that isnt in the drafts it should generally be left there. https://discord.com/channels/188630481301012481/188630652340404224/1050002698885398528

@peppy peppy changed the title New BeatmapInfoWedge design implementation. New BeatmapInfoWedge design implementation Jan 11, 2023
@bdach bdach mentioned this pull request Jan 14, 2023
@Joehuu Joehuu self-requested a review January 16, 2023 04:01
Joehuu
Joehuu previously requested changes Jan 16, 2023
osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs Outdated Show resolved Hide resolved
@frenzibyte frenzibyte changed the title New BeatmapInfoWedge design implementation New beatmap information wedge design implementation Jan 16, 2023
@peppy peppy self-requested a review January 24, 2023 07:26
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.

prelim review there's a lot wrong

CornerRadius = corner_radius;

// We want to buffer the wedge to avoid weird transparency overlaps between the colour bar and the background.
Child = bufferedContent = new BufferedContainer
Copy link
Member

Choose a reason for hiding this comment

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

No, you really do not want to buffer the whole thing. Have you run the tests and looked at the quality of things in general? Using a buffered container on things like text causes texel misalignment and makes everything more blurry. There's a reason on the original info wedge it was only applied to the background.

You can work around this by using

Suggested change
Child = bufferedContent = new BufferedContainer
Child = bufferedContent = new BufferedContainer(pixelSnapping: true)

but note that if the wedge is ever moved around on the screen using a transform, this will cause it to look less smooth.

Keep in mind there's an inherent overhead with every usage of BufferedContainer, too.

Copy link
Member

Choose a reason for hiding this comment

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

@mk56-spn did you miss this feedback?

Copy link
Contributor Author

@mk56-spn mk56-spn Feb 2, 2023

Choose a reason for hiding this comment

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

@mk56-spn did you miss this feedback?

Nope. Working on a fix. Been a tad busy. All should be addressed and done within the day - 2days max

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, i do see a path forward which does not require pixel snapping by separating the text and background into container only one of which is nested in the buffered container, i however did not opt for this since i felt that the pixel snapping look good here ( i tried it on the carousel to get a feel for the different applications and it look god awful because it can be moved by the user and it looked choppy as hell ) and benefited the wedge anyhow by making the line between the colourbar and the background less muddy

osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs Outdated Show resolved Hide resolved
}
}

public override bool IsPresent => base.IsPresent || DisplayedContent == null; // Visibility is updated in the LoadComponentAsync callback
Copy link
Member

Choose a reason for hiding this comment

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

What?

This needs reconsideration (yes I know this is in the original code, but it cannot be blindly copied into a newer version).

osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs Outdated Show resolved Hide resolved

float starAngle = (float)(Math.Atan(shear_width / wedge_height) * (180 / Math.PI));

// Applying the rotation directly to the StarCounter distorts the stars, hence it is applied to the child container
Copy link
Member

Choose a reason for hiding this comment

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

This cannot.

Copy link
Member

Choose a reason for hiding this comment

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

and this? if you are unsure how to solve, please say so and I will look into a solution.

Copy link
Contributor Author

@mk56-spn mk56-spn Feb 2, 2023

Choose a reason for hiding this comment

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

and this? if you are unsure how to solve, please say so and I will look into a solution.

I am unsure but mainly I've been a bit busy for a couple of days. A hint would definitely be appreciated though

Copy link
Member

Choose a reason for hiding this comment

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

Put it this way: as a general rule, a drawable should not be interacting with the children of a nested drawable.

The most raw solution would be to fix rotation to work correctly (inside the StarCounter). Or make a new derived / new version of the star counter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be fixed now

@Joehuu
Copy link
Member

Joehuu commented Sep 5, 2023

Have pushed changes to address most of the problems + code quality.

Here's a WIP branch with the wedge in-game (to test with how it behaves, not taking designs into account yet (i.e. width is longer, but is set in a grid container): master...Joehuu:osu:wedge-redesign

@peppy peppy self-requested a review September 5, 2023 06:32
@peppy
Copy link
Member

peppy commented Sep 5, 2023

This is looking pretty decent from a code perspective.

One remaining hope I have is to move the star rating out of the reloadable content so it doesn't awkwardly flash on beatmap change:

osu.Game.Tests.2023-09-05.at.07.00.50.mp4

Maybe the rank status can also be moved out (although it would require some consideration as to its animation, but that can come later).

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.

As commented

While it is true that there is a transparency overlap, it is only seen when entering/exiting song select. And there are other components game-wide that exhibit the same issue when transparency changes. Should be reconsidered another time.
@Joehuu
Copy link
Member

Joehuu commented Sep 5, 2023

Like this?

osu._1kfK00tuS8.mp4

The rolling is enabled because it would look bad without it (and it's not reconstructed every time anymore).

Moving the star rating display really improves code quality, so pushing the changes anyway soon.

@peppy
Copy link
Member

peppy commented Sep 6, 2023

Yep, that looks perfect.

@peppy peppy self-requested a review September 6, 2023 05:43
peppy added a commit to peppy/osu that referenced this pull request Sep 6, 2023
@peppy peppy merged commit 1ca713f into ppy:master Sep 6, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants