Skip to content

Add "Background scale mode" setting#36111

Open
diquoks wants to merge 17 commits intoppy:masterfrom
diquoks:quick-fix/background-fill-mode-setting
Open

Add "Background scale mode" setting#36111
diquoks wants to merge 17 commits intoppy:masterfrom
diquoks:quick-fix/background-fill-mode-setting

Conversation

@diquoks
Copy link
Contributor

@diquoks diquoks commented Dec 22, 2025

a slightly better version with localisation and code quality

@nagi-desuuu
Copy link

Reminder that this setting should also be in First run setup as well.

Comment on lines 80 to 81
fillMode = config.GetBindable<BackgroundFillMode>(OsuSetting.BackgroundFillMode);
fillMode.BindValueChanged(_ => loadBeatmapBackground(beatmap));
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change handling done here as opposed to inside BeatmapBackground?

Also, we never put BindValueChanged inside BDL methods as it is thread unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's really strange, don't remember why i did this, maybe something didn't work before that

should i use BindValueChanged inside LoadComplete instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but also consider not reloading the whole beatmap to update the mode. I think the can be removed and handled exclusively in BeatmapBackground.

{
base.LoadComplete();

fillMode.ValueChanged += _ => updateBackgroundFillMode();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now, BindValueChanged doesn't seem to work properly during and after playing beatmap
@peppy what could be the reason for this?

2025-12-26.13-53-45.mp4

@peppy peppy self-requested a review December 29, 2025 05:52
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 29, 2025
peppy
peppy previously approved these changes Dec 29, 2025
@peppy
Copy link
Member

peppy commented Dec 29, 2025

@bdach if you want to give this 5 minutes that would be appreciated, since i changed over 50%.

@diquoks diquoks changed the title Add "Background fill mode" setting Add "Background scale mode" setting Dec 29, 2025
@diquoks
Copy link
Contributor Author

diquoks commented Dec 29, 2025

since i changed over 50%.

didn't want to cause problems, i'm just not very familiar with the codebase yet 🙌

@peppy
Copy link
Member

peppy commented Dec 29, 2025

didn't want to cause problems, i'm just not very familiar with the codebase yet 🙌

it's fine, didn't mean it like that. just a standard practice that we get an extra review in such cases.

@bdach
Copy link
Collaborator

bdach commented Dec 29, 2025

This looks wonky in song select when progressing from main menu:

Screen.Recording.2025-12-29.at.12.12.23.mov

Note left side of screen, where the old background from main menu is visible where the "letterbox" would be until it insta-fades out.

@peppy
Copy link
Member

peppy commented Dec 29, 2025

Right, probably need to invoke some fake letterboxing boxes to fix that, which is going to be less fun.

@pull-request-size pull-request-size bot removed the size/M label Jan 4, 2026
@diquoks diquoks requested a review from peppy January 4, 2026 14:33
this.textureName = textureName;
RelativeSizeAxes = Axes.Both;

AddInternal(Letterbox = new Box
Copy link
Member

Choose a reason for hiding this comment

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

This results in a performance regression, due to overdrawing of layers.

  • Check something like LetterboxOverlay for how this needs to be done.
  • Should not result in any boxes unless required (ie. for fill screen it shouldn't be required).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This results in a performance regression, due to overdrawing of layers.

  • Check something like LetterboxOverlay for how this needs to be done.
  • Should not result in any boxes unless required (ie. for fill screen it shouldn't be required).

should be resolved in d2203d1

@diquoks diquoks requested a review from peppy January 12, 2026 09:42
case BackgroundScaleMode.ScaleToFit:
Sprite.FillMode = FillMode.Fit;

letterboxWidth.Value = (DrawWidth - Sprite.DrawWidth) / DrawWidth;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

couldn't get this to work with RelativeSizeAxes

@peppy
Copy link
Member

peppy commented Jan 13, 2026

I can't find a test scene covering this, how are you testing?

I specifically want a test so i can check the different scale modes and make sure there's no visible gap between the letterbox and the background.

@diquoks
Copy link
Contributor Author

diquoks commented Jan 13, 2026

I can't find a test scene covering this, how are you testing?

I specifically want a test so i can check the different scale modes and make sure there's no visible gap between the letterbox and the background.

i was just changing the setting and watching the changes in SSV2, also using ctrl+f1

i'm not quite used to using visual tests yet, but i can try to write something when i get home

@peppy
Copy link
Member

peppy commented Jan 13, 2026

Checking in song select likely hides some issues in edge case scenarios, so having a test scene would be good. Let me know if you have issues making it work.

@diquoks diquoks force-pushed the quick-fix/background-fill-mode-setting branch from 36534c8 to 7459dc0 Compare January 14, 2026 23:51
}

[Test]
public void TestBackgroundScaleModeSwitch()
Copy link
Member

Choose a reason for hiding this comment

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

The test doesn't show any changes visually. Nor does it run to completion in interactive mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nor does it run to completion in interactive mode.

it was fine for me but after some tests i realised that it might fail because of this line

AddUntilStep("default background fill mode is fill", () => getCurrentBackground()?.Sprite.FillMode == FillMode.Fill);

because the "default background fill mode" may actually change after reading the game.ini

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test doesn't show any changes visually.

there were also visual changes but now i decided to make them more visible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 218f113

@diquoks diquoks requested a review from peppy January 18, 2026 09:45
Comment on lines +36 to +87
Box leftLetterbox;
Box rightLetterbox;
Box topLetterbox;
Box bottomLetterbox;

AddInternal(new Container
{
RelativeSizeAxes = Axes.Both,
Children = new Drawable[]
{
leftLetterbox = new Box
{
Anchor = Anchor.CentreLeft,
Origin = Anchor.CentreLeft,
RelativeSizeAxes = Axes.Both,
Colour = Color4.Black,
},
rightLetterbox = new Box
{
Anchor = Anchor.CentreRight,
Origin = Anchor.CentreRight,
RelativeSizeAxes = Axes.Both,
Colour = Color4.Black,
},
topLetterbox = new Box
{
Anchor = Anchor.TopCentre,
Origin = Anchor.TopCentre,
RelativeSizeAxes = Axes.Both,
Colour = Color4.Black,
},
bottomLetterbox = new Box
{
Anchor = Anchor.BottomCentre,
Origin = Anchor.BottomCentre,
RelativeSizeAxes = Axes.Both,
Colour = Color4.Black,
}
}
});

letterboxWidth.BindValueChanged(margin =>
{
leftLetterbox.ResizeWidthTo(margin.NewValue);
rightLetterbox.ResizeWidthTo(margin.NewValue);
}, true);

letterboxHeight.BindValueChanged(margin =>
{
topLetterbox.ResizeHeightTo(margin.NewValue);
bottomLetterbox.ResizeHeightTo(margin.NewValue);
}, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't really like how it's written now but can't write any better yet

also need to figure something out with updating the letterboxes after window's resizing, as this kind of incident may occur
Image

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.

Background images don't match expected stretch mode for stable users

4 participants