Skip to content

Fix: Add SyncComponents for SSAO, ensuring SSAOResources is synced#24086

Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom
kfc35:24084_ssao_fix
May 4, 2026
Merged

Fix: Add SyncComponents for SSAO, ensuring SSAOResources is synced#24086
alice-i-cecile merged 2 commits intobevyengine:mainfrom
kfc35:24084_ssao_fix

Conversation

@kfc35
Copy link
Copy Markdown
Contributor

@kfc35 kfc35 commented May 2, 2026

Objective

Solution

  • Add SSAOResources (and other related components) to the sync component target for SSAO to ensure that adding/removing SSAO also adds/removes resources. This ensures that checking for SSAOResources later in mesh_view_bindings logic is valid.

Testing

  • cargo run --example ssao : toggling “Off” no longer crashes.

@kfc35 kfc35 changed the title Fix: Check for SSAO component before configuring mesh pipeline Fix: Check for SSAO component before configuring mesh pipeline for it May 2, 2026
@kfc35 kfc35 requested a review from beicause May 2, 2026 20:40
@kfc35 kfc35 added C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 2, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in Rendering May 2, 2026
@kfc35 kfc35 added this to the 0.19 milestone May 2, 2026
Copy link
Copy Markdown
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Comment seems kinda redundant, but approved.

@kfc35
Copy link
Copy Markdown
Contributor Author

kfc35 commented May 3, 2026

I’ll remove the comment, I can be excessively verbose 😅

@kfc35 kfc35 force-pushed the 24084_ssao_fix branch from 438c094 to 15dce5d Compare May 3, 2026 00:27
@beicause
Copy link
Copy Markdown
Member

beicause commented May 3, 2026

The issue means ScreenSpaceAmbientOcclusionResources is not removed if ScreenSpaceAmbientOcclusion is removed.

Could you try adding extract_component_sync_target((Self, ScreenSpaceAmbientOcclusionResources, SsaoPipelineId, SsaoBindGroups))?

#[derive(Component, ExtractComponent, Reflect, PartialEq, Clone, Debug)]
#[reflect(Component, Debug, Default, PartialEq, Clone)]
#[require(DepthPrepass, NormalPrepass)]
#[doc(alias = "Ssao")]
pub struct ScreenSpaceAmbientOcclusion {

@kfc35 kfc35 force-pushed the 24084_ssao_fix branch from 7e348ad to 83a6c65 Compare May 3, 2026 02:03
@kfc35 kfc35 requested a review from JMS55 May 3, 2026 02:03
@kfc35
Copy link
Copy Markdown
Contributor Author

kfc35 commented May 3, 2026

Yep that works, so I switched to using sync components as opposed to checking for the regular component.

I had to make the bindgroups and pipeline id pub to accommodate. There are other places where such structs are also pub so I figured it doesn’t hurt.

@kfc35 kfc35 changed the title Fix: Check for SSAO component before configuring mesh pipeline for it Fix: Add SyncComponents for SSAO, ensuring SSAOResources also exists May 3, 2026
@kfc35 kfc35 changed the title Fix: Add SyncComponents for SSAO, ensuring SSAOResources also exists Fix: Add SyncComponents for SSAO, ensuring SSAOResources is synced May 3, 2026
@kfc35 kfc35 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 3, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 3, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 3, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 3, 2026
Merged via the queue into bevyengine:main with commit d00313e May 4, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in Rendering May 4, 2026
pull Bot pushed a commit to orzogc/bevy that referenced this pull request May 4, 2026
…e#24089)

# Objective

- Alongside bevyengine#24086, helps with bevyengine#24084, although I think we should double
check any other added conditionals for bind group entries to make sure
they are accurate.

## Solution
So, originally the `SCREEN_SPACE_TRANSMISSION` was enabled with
`key.intersects(MeshPipelineKey::SCREEN_SPACE_SPECULAR_TRANSMISSION_RESERVED_BITS)`.
However, a low quality transmission would make this false, since low’s
MeshPipelineKey is configured like this: `const
SCREEN_SPACE_SPECULAR_TRANSMISSION_LOW = 0 <<
Self::SCREEN_SPACE_SPECULAR_TRANSMISSION_SHIFT_BITS;`. So, a
ScreenSpaceTransmission with Low Quality would break rendering (since
another if-block merely checks that the `ScreenSpaceTransmission`
component exists)

Making it so that the low transmission truly does *not* include the
view_transmission_textures makes the transmission render not properly -
the spheres disappear!

So, I think the proper fix here is to remove the gating around
transmission textures.
Edit: Another potential fix is to change the condition of the
`intersects` but I’m not sure how to encode what we want unless we want
to add another bit for `ScreenSpaceTransmission` component exists
essentially? Happy to close this PR if that is an acceptable direction.

## Testing

`cargo run --example transmission` works for all quality levels.
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-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants