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

Change default clear color to black #9540

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rdrpenguin04
Copy link
Contributor

Objective

Black is an easier color to fill for GPUs (if by a small margin), and the choice of clear color really isn't necessary for most games, where 3D games will choose skyboxes and 2D games will choose background images (and most 2D games ideally wouldn't use rgb(0.4, 0.4, 0.4) as their background color of choice anyways). Thus, choosing black would enable a small optimization.

Solution

Adjusted ClearColor::default


Changelog

Changed: the default clear color is now black.

Migration Guide

Any applications relying on the default clear color being the original shade of dark grey should change them. Meanwhile, anyone who set the clear color to black, either as an optimization or for a space-related game, can now change their color back to the default.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Aug 22, 2023
@alice-i-cecile
Copy link
Member

I don't think I agree with this decision. The performance argument isn't convincing at all to me without data, and this will make quick experiments and examples less useful since you won't be able to see black lines that are drawn.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 22, 2023
@rdrpenguin04
Copy link
Contributor Author

rdrpenguin04 commented Aug 22, 2023

Not being able to see black lines isn't a convincing argument to me, since white-on-black is easier to see than black-on-dark-grey or white-on-dark-grey.

On the performance side, check https://gpuopen.com/learn/rdna-performance-guide/#clears; while I don't have performance metrics, I was directed here.

@JMS55
Copy link
Contributor

JMS55 commented Aug 22, 2023

While I agree with this in principal, I think we should delay it until we add procedural skyboxes. Probably sometime in 0.13 assuming I don't run into major issues.

@rdrpenguin04
Copy link
Contributor Author

The thing is, if someone wanted a non-black background, chances are they set it manually anyway. The default color is almost certainly not used in production ever except by accident. I don't think procedural skyboxes are needed for a simple change that affects almost nobody; if someone wants to use a non-black clear color, there's nothing stopping them from doing so. All this does is change the default.

@superdump
Copy link
Contributor

I also agree with this in principal and I think @JMS55 point about skyboxes is a good one.

I tend to think that hidden costs are much more difficult to be aware of / discover than things that just don't work.

When lots of things will use some kind of skybox or background of some form which will replace all the clear colour, it would be difficult to be aware of the lost performance from slow-path clearing of textures and from what I recall from @HackerFoo 's work on the Noumenal mobile app, things like clearing buffers has significant impact on mobile in particular. AMD/NVIDIA also recommend using fast paths for clearing/initialising textures if possible.

On the other hand, not being able to see black lines on a black background is immediate and clear friction that is actionable.

@rdrpenguin04
Copy link
Contributor Author

rdrpenguin04 commented Aug 23, 2023

I'm still somewhat failing to see the problem with black lines on a black background; if the background is obviously black, why isn't it trivial to make the lines white?

Edit: Oh right, that's your point: it is trivial to fix, but clear color is less obvious -- oh, that makes more sense, and I completely agree.

@alice-i-cecile
Copy link
Member

Yep :) Note that this will have an impact on more complex rendering examples too: things like the examples that demonstrate the effects of lighting and PBR materials will look quite different on a black rather than neutral-gray background!

I'm fine to defer to the tastes of the rendering experts here, but we should verify that all of the visual examples still look fine as part of this PR and manually set the clear color (or change the line etc. color) as needed.

@rdrpenguin04
Copy link
Contributor Author

Are there automated tests to check against an existing screenshot?

@dmyyy
Copy link
Contributor

dmyyy commented Aug 23, 2023

I think @mockersf was working on something like that here: #9220

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants