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

Replace local footer in existing sheared overlays (e.g. mod select & first-run setup) with ScreenFooter #28683

Merged
merged 17 commits into from
Jul 12, 2024

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Jun 30, 2024

Preview:

CleanShot.2024-06-30.at.07.34.43-converted.mp4
CleanShot.2024-06-30.at.07.19.03.mp4

…erlays and improve UX

With this new order, the logo can be easily moved to display in front of the footer in `SongSelectV2` without breaking experience when footer-based overlays are present. Such overlays (i.e. mod select overlay) will also be dimmed alongside the current screen when a game-wide overlay is open (e.g. settings).
@peppy peppy self-requested a review July 10, 2024 06:08
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.

Tests need fixing too

@@ -290,6 +297,8 @@ public void CloseAllOverlays(bool hideToolbar = true)
if (hideToolbar) Toolbar.Hide();
}

public void ChangeLogoDepth(bool inFrontOfFooter) => ScreenContainer.ChangeChildDepth(logoContainer, inFrontOfFooter ? float.MinValue : 0);
Copy link
Member

Choose a reason for hiding this comment

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

I dunno about this. I'd rather just see it using Parent access similar to other usages in LogoTrackingContainer...

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow what you mean by "Parent access".

Copy link
Member

Choose a reason for hiding this comment

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

Parent.ChangeChildDepth rather than OsuGame method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well...the drawable which its depth is being changed here is the parent of OsuLogo, meaning that this method is the equivalent of logo.Parent.Parent.ChangeChildDepth(logo.Parent, float.MinValue), and not to mention that I can't access ChangeChildDepth directly without direct-casting to Container first. That's why I settled for an OsuGame method to ease my head.

Copy link
Member

Choose a reason for hiding this comment

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

Right. It's still not great. See if you can find a nicer way of doing this, else I'll take a look during next review pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have already spent hours before making this PR trying to figure out how to fix this issue of the logo depth being a nuisance in the user's experience, but I'll try once more and see if I have a clever idea this time.

Copy link
Member Author

Choose a reason for hiding this comment

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

In case it helps, here's a description for the purpose of this code.

By default, this is how I intend these drawables to be ordered depth-wise from back to front:

  1. The screen content
  2. The osu! logo
  3. Footer-based overlays (e.g. mod select overlay / first-run setup overlay)
  4. The screen footer
  5. Screen-wide overlays (basically all overlays)

The logo is intentionally placed behind footer-based overlays and the screen footer, so that opening first-run setup in main menu does not get covered by the logo in main menu (visually and/or input-wise).

Now, this does not work well in new song select, because I want the logo to appear in front of the footer, and that is where the nuisance is.

So to simplify matters, I chose to define a method at OsuGame that changes the depth of the logo to appear in front of footer stuff, and use it when wanting to attach the logo to the footer.

Proxying does not help as it only changes the depth of the logo visually, it does not affect order in input queue (supposedly intentional).

osu.Game/Overlays/FirstRunSetupOverlay.cs Outdated Show resolved Hide resolved
…er content

Identified by tests. See https://github.com/ppy/osu/actions/runs/9869382635/job/27253010485 & https://github.com/ppy/osu/actions/runs/9869382635/job/27253009622.

This change also prevents the initial `PopOut` call in overlays from calling `clearActiveOverlayContainer`, since it's not in the update thread and it's never meant to be called at that point anyway (it's supposed to be accompanied by a previous `PopIn` call adding the footer content).
@frenzibyte
Copy link
Member Author

I've applied a minor refactor on the footer content flow between ShearedOverlayContainer and ScreenFooter to prevent an overlay from incorrectly clearing footer content of another displayed overlay.

@frenzibyte frenzibyte self-assigned this Jul 10, 2024
@peppy peppy self-requested a review July 11, 2024 09:12
@peppy
Copy link
Member

peppy commented Jul 11, 2024

@frenzibyte this is still failing tests. please run them locally.

@frenzibyte
Copy link
Member Author

Right, my apologies.

@peppy
Copy link
Member

peppy commented Jul 12, 2024

Gonna get this in. It seems okay enough to move forward with.

@peppy peppy merged commit eb3f480 into ppy:master Jul 12, 2024
9 of 17 checks passed
@frenzibyte frenzibyte deleted the footer-v2-integration branch July 12, 2024 13:53
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.

2 participants