-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add ability to change background dim colour in grayscale #30391
Draft
Uncomfy
wants to merge
47
commits into
ppy:master
Choose a base branch
from
Uncomfy:background_dim_colour
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
48e5d18
Added "Background Colour" setting
Uncomfy 261e5ab
Made Background.Sprite replaceable
Uncomfy 30b3798
Made a custom class for BeatmapBackground with a custom BeatmapBackgr…
Uncomfy 1d57a83
Made BeatmapBackgroundSprite use a custom shader
Uncomfy 42579a6
Added a custom DrawNode for BeatmapBackgroundSprite
Uncomfy 5b78d6f
Passing parameters to the shader
Uncomfy d088fc3
Made BeatmapBackground shader handle dimming
Uncomfy be56dd7
Added support for DimColour
Uncomfy b5156d9
Pass a full colour for DimColour instead of a single value into a shader
Uncomfy fa965fa
Changed BeatmapBackground class name to DimmableBeatmapBackground to …
Uncomfy 7a6f197
Moved DimmableBeatmapBackground to its own file
Uncomfy 4c3401f
Made DimmableBeatmapBackground inherit from BeatmapBackground, switch…
Uncomfy 9283107
Switched to using Color4 for DimColour instead of float
Uncomfy a341c3a
Made DimmableBeatmapBackgroundSprite invalidate DrawNode on DimColour…
Uncomfy b7311fd
Formatting fixes
Uncomfy 221ff9f
Changed the way colour is reported in TestSceneUserDimBackgrounds.cs
Uncomfy 8b1264b
Added DimColour to user settings in TestSceneUserDimBackgrounds.cs
Uncomfy 1d696ba
Updated TestSceneUserDimBackgrounds.cs to test for colour offset caus…
Uncomfy c99dd87
Handle IgnoreUserSettings for DimColour
Uncomfy b7531d1
Updated the way TestSceneUserDimBackgrounds.cs checks if background i…
Uncomfy e1b06d3
Integrated DimmableBeatmapBackground into BeatmapBackground
Uncomfy bab981e
Use existing methods to update shader variables
Uncomfy 8f95210
Made background receive correct dim values when it's set
Uncomfy 3281b88
Disable DimColour when user settings are ignored
Uncomfy d1fe76b
Made BufferedContainer handle dimming when it's present
Uncomfy 20c2619
Added default values for BeatmapBackgroundSprite
Uncomfy eaa1476
Added explanations on why and how colour is split into two components
Uncomfy e73efd7
Updated the way colours are computed and checked in TestSceneUserDimB…
Uncomfy b62a044
Added forgotten Dispose's
Uncomfy 983243f
Moved BeatmapBackground creation into a virtual function to help with…
Uncomfy 892bd41
Created TestBeatmapBackground class and moved colour computations int…
Uncomfy 20c1141
Added an interface to simplify access to current dimmable object in B…
Uncomfy cf88f97
Formatting fixes
Uncomfy eada120
Get DrawColour directly from object drawing the BG in TestSceneUserDi…
Uncomfy a4a43a9
Changed the way Background.Sprite is replaced by BeatmapBackground
Uncomfy 0312490
Changed the way Background.bufferedContainer is replaced by BeatmapBa…
Uncomfy ec22de7
Added test to verify that both dimming handlers work as intended
Uncomfy 0712230
Made TestSceneUserDimBackgrounds compute current colour using Colored…
Uncomfy 356a0f3
Added Background colour to visual settings
Uncomfy 62567f2
Changed BackgroundColour localisable string into BackgroundDimColour
Uncomfy c8d4dac
Changed the way ParentDrawColour is computed
Uncomfy fbe05ea
Updated the source of DrawColour when computing expected/target colour
Uncomfy 65d9dbe
Formatting fixes
Uncomfy f04c4b0
Removed dependency on osuTK
Uncomfy 96b7d97
Preload BeatmapBackground shader
Uncomfy c5867b6
Small formatting fixes
Uncomfy 93c3fac
Test if BufferedContainer's framebuffer is redrawn on dim changes
Uncomfy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
281 changes: 265 additions & 16 deletions
281
osu.Game.Tests/Visual/Background/TestSceneUserDimBackgrounds.cs
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't
TextureShader
responsible for rendering the framebuffer texture of the buffered container? If so, isn't that pointless? Isn't it enough to apply the custom "BeatmapBackground" shader on the underlying beatmap background sprite? Would that cause issues with blurring? I can't imagine it will.In other words, do
Sprite.TextureShader = "BeatmapBackground"
instead (alongside the extra logic for the uniform buffer). That will save exposingTextureShader
framework-side and also possibly "save" shader computation because the overall effect will become buffered (don't quote me on this).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it will require the redraw of the framebuffer texture on each DimLevel change, which will also force the blur to be redrawn, and I feared that this might introduce unnecessary overhead during (for example) breaks, if player has background blur enabled. If that is not a concern - yeah, keeping the custom shader only on the sprite makes complete sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it's pointless, at least in the current implementation.
Changing the dim parameters requires DrawNode invalidation, which forces the framebuffer to be redrawn, so there are no benefits. This does mean that DimLevel transitions are way more expensive now (at least on my rather old and slow hardware).
Here are the results of my benchmarks for average frametime during different parts of the beatmap.
With user blur set to 0%:
With user blur set to 100%:
"Baseline" is osu! 2024.1009.1
"Coloured dim" is commit c5867b6
Details about the benchmark
Made a map with 64 gameplay sections and 63 breaks for this benchmark, total duration is 3:09.
"Fade in" refers to frames rendered during gameplay-to-break transition, "Fade out" refers to break-to-gameplay transition.
"Gameplay" and "Breaks" columns are just for sanity-checking, as they should be same.
Total amount of recorded frames:
osu! settings:
System info:
So I guess now there are two ways to go - ditch the shader replacement on the BufferedContainer and have slower transitions, or modify the BufferedContainer somehow to avoid redrawing the framebuffer when shader parameters are changed.
Which one is more preferable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, this is a bit of a pesky situation. You can control whether the framebuffer gets redrawn by overriding
GetDrawVersion()
and make that point to a custom invalidation ID that is normally incremented but not when dim properties are changed. SeePath
in osu!framework, it might actually be doing exactly what you're looking for.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test case for it, but solving it requires a change in the framework: ppy/osu-framework#6404
With that change applied benchmarks look a lot better.
With user blur set to 0%:
With user blur set to 100%:
"Baseline" is osu! 2024.1009.1
"Coloured dim" is commit 93c3fac with ppy/osu-framework#6404 applied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these benchmark numbers represent the true impact because as far as I read you're measuring over a long period of time, while when the blur re-renders it's going to be very detrimental only to the local frames (so the overall "increase" is much smaller than the actual perceived difference to the user).
Of course, as long as the numbers are going down this doesn't matter so much, so carry on I guess.
A better way of benchmarking would be to force the redraw every frame then just take a reading of the average frame time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've double checked, and without ppy/osu-framework#6404 my implementation triggered a blur redraw on every frame during transition, but didn't affect frames during gameplay or break.
This means that the stats computed in the "Fade in" and "Fade out" columns only take into account frames where redraw happened.
My goal was to show that extra redraws were triggered and that it affected the performance during specific parts of the beatmap, so I couldn't use this approach