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

Move Msaa to component #14273

Merged
merged 14 commits into from
Jul 22, 2024

Conversation

tychedelia
Copy link
Contributor

Switches Msaa from being a globally configured resource to a per camera view component.

Closes #7194

Objective

Allow individual views to describe their own MSAA settings. For example, when rendering to different windows or to different parts of the same view.

Solution

Make Msaa a component that is required on all camera bundles.

Testing

Ran a variety of examples to ensure that nothing broke.

TODO:

  • Make sure android still works per previous comment in extract_windows.

Migration Guide

Msaa is no longer configured as a global resource, and should be specified on each spawned camera if a non-default setting is desired.

Switches `Msaa` from being a globally configured resource to a per camera view
component.

Closes bevyengine#7194
@pcwalton pcwalton self-requested a review July 10, 2024 21:14
Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

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

Looks good once you fix the failing CI.

@JMS55 JMS55 requested a review from superdump July 11, 2024 06:26
Copy link
Contributor

@NiseVoid NiseVoid left a comment

Choose a reason for hiding this comment

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

These changes look correct to me. I always found it strange that MSAA is global, especially since having different settings on different cameras could be very useful, for example having MSAA on a camera that just sees objects with sharp outlines and using something blurrier but cheaper for the rest.

Did you test this with multiple cameras with different MSAA modes? Iirc there are some pipeline properties that depend on it, so things might break if multiple levels are enabled at once.

@tychedelia
Copy link
Contributor Author

tychedelia commented Jul 11, 2024

Did you test this with multiple cameras with different MSAA modes? Iirc there are some pipeline properties that depend on it, so things might break if multiple levels are enabled at once.

Yes! Here are some foxes:

image

@BD103 BD103 added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jul 12, 2024
@tychedelia
Copy link
Contributor Author

Here's a simple patch that might help alleviate the problem of having the proliferation of main textures be confusing to users who want to run split screen setups: #14287.

@mockersf
Copy link
Member

the mobile example needs to be updated, or this will fail merging on android compilation

auto-merge was automatically disabled July 15, 2024 23:33

Head branch was pushed to by a user without write access

@tychedelia
Copy link
Contributor Author

the mobile example needs to be updated, or this will fail merging on android compilation

Should be fixed now. This PR also removes the more advanced per-window feature detection. I'd be happy to add that back in but wasn't sure where exactly it should go.

@tychedelia tychedelia requested a review from mockersf July 18, 2024 20:45
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 22, 2024
Merged via the queue into bevyengine:main with commit 03fd1b4 Jul 22, 2024
27 checks passed
@tychedelia tychedelia deleted the 7194-move-msaa-to-component branch July 22, 2024 18:51
@JMS55
Copy link
Contributor

JMS55 commented Jul 22, 2024

I think some things were missed here. Iirc in TAA plugin docs and meshlet plugin docs, it mentions that it automatically disabled MSAA, which is no longer true.

@tychedelia tychedelia mentioned this pull request Jul 22, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 22, 2024
Minor doc fixes missed in #14273
github-merge-queue bot pushed a commit that referenced this pull request Jul 24, 2024
# Objective

- #14273 changed MSAA to a component, and broke some examples

- SSAO needs MSAA to be disabled

https://github.com/bevyengine/bevy/blob/f0ff7fb5445996e561d9ea336ee353544d79fef6/crates/bevy_pbr/src/ssao/mod.rs#L495

- `AlphaMode::AlphaToCoverage` needs MSAA to be not off to do something

https://github.com/bevyengine/bevy/blob/f0ff7fb5445996e561d9ea336ee353544d79fef6/examples/3d/transparency_3d.rs#L113-L117

# Solution

- change MSAA in those examples
@re0312 re0312 mentioned this pull request Jul 25, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 25, 2024
# Objective

- meshlet example has broken since #14273

## Solution

- disable msaa in meshlet example

Co-authored-by: François Mockers <[email protected]>
m-edlund added a commit to m-edlund/bevy that referenced this pull request Oct 17, 2024
richchurcher added a commit to richchurcher/bevy_ecs_tilemap that referenced this pull request Oct 25, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 31, 2024
# Objective

#14273 changed `Msaa` to be a component rather than a resource. However,
the documentation still says that it is a resource. This tripped me up
during migration to 0.15 until I looked at the type definition.

Additionally, the docs have some unnecessary repetition and some grammar
mistakes, and they don't link to camera documentation.

## Solution

Fix up the docs!
mockersf pushed a commit that referenced this pull request Nov 5, 2024
# Objective

#14273 changed `Msaa` to be a component rather than a resource. However,
the documentation still says that it is a resource. This tripped me up
during migration to 0.15 until I looked at the type definition.

Additionally, the docs have some unnecessary repetition and some grammar
mistakes, and they don't link to camera documentation.

## Solution

Fix up the docs!
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-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move Msaa to a component, instead of a resource
7 participants