Fix player being overlayed on ScalingContainer's background when storyboard is fully dimmed and replaces background

[cdwcgt]

When the player uses scale mode, the beatmap have storyboard and enable it, background(DimContent) will hide because storyboard enabled, and ScalingBackgroundScreen will show at playfield.
like this:

disable storyboard

and this pr force show DimContent when dim set to 100% to fix this problem

[frenzibyte]

I'm failing to understand what this PR is trying to achieve, and the description is not helping at all. The fact that you're essentially displaying the background when user sets 100% dim raises multiple question marks in my head. As a first step, can you show us a screenshot of where the issue is on master and how it looks with your PR?

[bdach]

@frenzibyte did you miss the screenshots in the first post? they show the issue pretty clearly to me.

1. go to settings, change scaling mode to 'everything' - this causes a second dimmed copy of the background to be displayed behind the scaled content
2. pick random map, choose 100% dim, ensure storyboard is enabled
3. in gameplay, the player background isn't shown, but the dimmed background behind scaled content is still there behind the gameplay

that said i'm also uncertain of the direction. doing what this PR does makes it so that a lot of pixels need to be filled black in front of the scaled background, even for players that aren't using screen scaling. i'd probably argue that this should not be the case, and that the background behind the scaled content should also have its dim % settable via settings (as much as i hate to add new settings). or it should use the same setting as gameplay background dim? or that doing this black box workaround should be based on the scaling setting rather than the storyboard dim %? dunno. yet another PR opened without an attempt of even asking us what the right thing to do would be.

edit: reproduction requires a specific beatmap that has this logic enabled:

osu/osu.Game/Storyboards/Storyboard.cs

Lines 71 to 88 in 1262c44

	/// <summary>
	/// Whether the beatmap's background should be hidden while this storyboard is being displayed.
	/// </summary>
	public bool ReplacesBackground
	{
	get
	{
	string backgroundPath = BeatmapInfo.Metadata.BackgroundFile;
	if (string.IsNullOrEmpty(backgroundPath))
	return false;
	// Importantly, do this after the NullOrEmpty because EF may have stored the non-nullable value as null to the database, bypassing compile-time constraints.
	backgroundPath = backgroundPath.ToLowerInvariant();
	return GetLayer("Background").Elements.Any(e => e.Path.ToLowerInvariant() == backgroundPath);
	}
	}

example: https://osu.ppy.sh/beatmapsets/1016398#osu/2359368

i'm probably more fine with this PR knowing that, given that it doesn't affect as many beatmaps as i thought it would. i would still probably use screen scaling as the trigger for this logic to kick in rather than DimLevel == 1.

[frenzibyte]

After a brief conversation in discord, I can understand the issue now. As demonstrated in the comment above after the edit, this only happens on beatmaps with a storyboard element that replaces the beatmap background. And put simply, on 100% dim the storyboard element becomes hidden and the beatmap background is already hidden due to the StoryboardReplacesBackground bool, so at this point there's nothing backing the player screen and it becomes incorrectly overlayed on the background displayed by the game's scaling container.

I do agree with checking whether scaling is active instead, but caution needs to be taken on non-opaque storyboard background elements (if that's a thing). Might be best combining both conditions with a proper explanatory comment.

Alternative would be to just place a black background box behind the gameplay screen (or OsuScreens in general) and call it a day, but not sure on the performance side of things (theoretically the front-to-back pass should render this noop, but I'm not quite the expert here).

[cdwcgt]

Hum, this is 100% repro for me(maybe some kind of coincidence), so I opted to fix it directly with dimlevel==1 (and it won't affect the break time so I think it is good), now it seems that I may have misunderstood its repro condition, I will adjust my pr

[cdwcgt]

Now it seems that the problem is not only in gameplay but all screen use BeatmapBackground and BeatmapBackgroundWithStoryBoard
It may be necessary to place a black background behind some scenes
Or put a black background directly behind ScalingContainer when using the everything or excluding overlays settings
But it's hard for me to decide because I don't know which is better (for performance and code quality)
If there is a clear direction please let me know

If you look closely at this picture, you can see the background behind it.

[frenzibyte]

As we've mentioned in discord, placing a black layer behind BackgroundScreenBeatmap if scaling is active and storyboard replaces background is probably the easiest way to go here.

[peppy]

Agree with proposed direction (black layer when required). This doesn't affect many beatmaps and it's the best we can do (will be optimised away wherever front-to-back optimisations are available, too).

[cdwcgt]

I might choose to add a black background directly behind the storyboard which will replace the background
It looks good, but it is different as adding in BackgroundScreenBeatmap as mentioned before, so I want to confirm whether this direction is feasible

This solution should solve all derivative issues, because the storyboard which replace background need have black background

if (Storyboard.ReplacesBackground && Storyboard.HasDrawable)
{
    AddInternal(new Box
    {
        Colour = Color4.Black,
        RelativeSizeAxes = Axes.Both,
        Depth = float.MaxValue,
    });
}

[bdach]

I think inside the storyboard is also fine yes.

[cdwcgt]

Unfortunately, if only the drawableStoryboard is adjusted, the storyboard will be hidden under 100% dim (including the black background)
so the change of this is required?

Or the black background should be handled by all classes which use DrawableStoryboard

[peppy]

What's the reason it can't be done in BackgroundScreenBeatmap? Optimally, we want to not render the storyboard when it's fully dimmed as it reduces overheads.

[cdwcgt]

The most obvious reason is that the storyboard display problem does not only appear in BackgroundScreenBeatmap, but also BeatmapBackgroundWithStoryboard (see #22035 (comment))

and the main reason for this problem is that the storyboard design did not consider the issue of background transparency (because the background of stable is always black), so adding a black background behind the storyboard is the expected behavior (for the person who designed the storyboard, because they use alpha to achieve dimming)

For performance issues, I have written the logic. When the "replace" storyboard is enabled, the background of the map will always be 100dim (to avoid potential visual problems) and storyboard will hide when DimLevel >= 1f like before

I'll push it and you can check if it's as expected

[bdach]

why rename?

[cdwcgt]

I originally used ShowStoryboard when I chose the showReplaceBackgroundStoryboard variable name.
Because of the conflict, I added config after it, and I will restore it.

[bdach]

the changes in this class are so convoluted and self-referential for no apparent reason.

do not use the numerical dim level here. use the scaling mode itself as already requested. is there an issue with that? if yes then please explain clearly where. also remove the DimLevel override as i believe it should not be required if scaling mode is used properly.

[cdwcgt]

according to #22238, this is not only cause when scale settings set to Everything and excluding overlay, but also multi specter, so it can't just check scale settings.

for dim override, when dim set to 100% or turn into break time, it can cause particularly strange visual effects (backgroundBeatmap show undimmed background while storyboard fades out). So I force it to 100% when storyboardReplaceBackground is enabled

[cdwcgt]

let me sort out the situation
This problem will appear in

• main menu　(use BeatmapBackground or BeatmapBackgroundWithStoryboard instead of BackgroundScreenBeatmap)
• multiplayer spectate
• solo play

all these need a storyboard which is StoryboardReplacesBackground
This is because when people make the storyboard, they assume the background is black. In stable, he will not have problems.
but in lazer, the situation is different.
So I think it should add a black background directly behind the storyboard

and for why self-referential
this is because if I set dim to 100%, storyboard will disappear first before background being black
which like this:

osu.development.2023-03-06.23-16-26_output_x264.mp4

after 005e643#diff-7cb02fcd1e503e4d277ad1bde3dbd36855b0d735bddb15ffbc02254eeb8b8943R135-R136

osu.development.2023-03-06.23-24-19_output_x264.mp4

which here I use base.DimLevel, because breakLightening