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

[Merged by Bors] - Better bloom default settings #6546

Closed
wants to merge 5 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Nov 10, 2022

Objective

  • Use better defaults for bloom

Solution

  • Divide the intensity by 3. It's still noticeable
  • Change the mip level? (not sure about that change, it's from a discussion with @superdump)

bloom example

main:
Screenshot 2022-11-11 at 01 09 26
this pr:
Screenshot 2022-11-11 at 01 08 00

bistro scene

main:
Screenshot 2022-11-11 at 01 16 42
this pr:
Screenshot 2022-11-11 at 01 15 12

mockersf and others added 2 commits November 11, 2022 00:47
Co-Authored-By: Robert Swain <302146+superdump@users.noreply.github.com>
@mockersf mockersf added the A-Rendering Drawing game state to the screen label Nov 10, 2022
@mockersf mockersf added this to the Bevy 0.9 milestone Nov 10, 2022
mockersf and others added 2 commits November 11, 2022 01:00
Co-Authored-By: JMS55 <47158642+JMS55@users.noreply.github.com>
Co-Authored-By: JMS55 <47158642+JMS55@users.noreply.github.com>
@superdump
Copy link
Contributor

superdump commented Nov 11, 2022

Reducing the maximum mip level is about limiting the minimum resolution that a mip can be to avoid what I think would be considered an aliasing artifact. If we go below the shortest edge being 8 pixels for the highest mip level then there are large scale artifacts where the bloom tracks with the camera.

Before:

IMG_0043.MOV

After:

IMG_0044.MOV

I guess it’s a bit like having too few local dimming zones on a monitor backlight.

@superdump
Copy link
Contributor

@mockersf I’m not sure if -3 is enough so it would be good if it were configurable. As a human I don’t relate to mip levels directly, instead I think it could be good to have a setting for minimum shortest edge resolution in pixels for the maximum mip level and make the maths work for that. Default to 8 or 16.

@mockersf
Copy link
Member Author

mockersf commented Nov 11, 2022

Default to 8 or 16.

    In Device::create_texture
      note: label = `bloom_texture_a`
    Texture descriptor mip level count 16 is invalid, maximum allowed is 11

This can cause crashes if the user can specify any number they want.

I think I would prefer to stick with this -3 for now, and think about adding a setting for it after the release

@superdump
Copy link
Contributor

Default to 8 or 16.

    In Device::create_texture
      note: label = `bloom_texture_a`
    Texture descriptor mip level count 16 is invalid, maximum allowed is 11

This can cause crashes if the user can specify any number they want.

I think I would prefer to stick with this -3 for now, and think about adding a setting for it after the release

What change did you do to get that? I’ll come up with some maths in a bit. :)

@JMS55
Copy link
Contributor

JMS55 commented Nov 11, 2022

I’m not sure if -3 is enough so it would be good if it were configurable. As a human I don’t relate to mip levels directly, instead I think it could be good to have a setting for minimum shortest edge resolution in pixels for the maximum mip level and make the maths work for that. Default to 8 or 16.

Imo we should leave it as -3, and see if it's actually a problem that anyone brings up before preemptively adding a setting. That way we can also get feedback on whether -3 is a good default or not, and what would be better.

I also think keeping it in terms of mip levels is better, because it makes more sense for users to adjust a number +/- 1 until it looks correct, than changing between 2/4/8/16/32 etc.

@cart
Copy link
Member

cart commented Nov 11, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 11, 2022
# Objective

- Use better defaults for bloom

## Solution

- Divide the intensity by 3. It's still noticeable
- Change the mip level? (not sure about that change, it's from a discussion with @superdump)


### bloom example
main:
<img width="1392" alt="Screenshot 2022-11-11 at 01 09 26" src="https://user-images.githubusercontent.com/8672791/201232996-20d6cf65-2511-41bc-979b-f2c193e4e4e6.png">
this pr:
<img width="1392" alt="Screenshot 2022-11-11 at 01 08 00" src="https://user-images.githubusercontent.com/8672791/201232987-b1ebad2a-4ebf-4296-a91b-aab898544a9d.png">


### bistro scene
main:
<img width="1392" alt="Screenshot 2022-11-11 at 01 16 42" src="https://user-images.githubusercontent.com/8672791/201233028-526999a3-0060-44f7-b0dd-f78666b06c1d.png">
this pr:
<img width="1392" alt="Screenshot 2022-11-11 at 01 15 12" src="https://user-images.githubusercontent.com/8672791/201233044-50201034-e881-40e1-8455-76cabc621a9b.png">
@cart
Copy link
Member

cart commented Nov 11, 2022

(Feel free to continue discussing the merits of configuring -3, but I think this is sufficient for 0.9 / the changes here are necessary)

@bors bors bot changed the title Better bloom default settings [Merged by Bors] - Better bloom default settings Nov 12, 2022
@bors bors bot closed this Nov 12, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Use better defaults for bloom

## Solution

- Divide the intensity by 3. It's still noticeable
- Change the mip level? (not sure about that change, it's from a discussion with @superdump)


### bloom example
main:
<img width="1392" alt="Screenshot 2022-11-11 at 01 09 26" src="https://user-images.githubusercontent.com/8672791/201232996-20d6cf65-2511-41bc-979b-f2c193e4e4e6.png">
this pr:
<img width="1392" alt="Screenshot 2022-11-11 at 01 08 00" src="https://user-images.githubusercontent.com/8672791/201232987-b1ebad2a-4ebf-4296-a91b-aab898544a9d.png">


### bistro scene
main:
<img width="1392" alt="Screenshot 2022-11-11 at 01 16 42" src="https://user-images.githubusercontent.com/8672791/201233028-526999a3-0060-44f7-b0dd-f78666b06c1d.png">
this pr:
<img width="1392" alt="Screenshot 2022-11-11 at 01 15 12" src="https://user-images.githubusercontent.com/8672791/201233044-50201034-e881-40e1-8455-76cabc621a9b.png">
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants