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

Fix CAS toggle broken by retained render world #16533

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

kristoff3r
Copy link
Contributor

Objective

Fixes #16531

I also added change detection when creating the pipeline, which technically isn't needed but it felt weird leaving it as is.

Solution

Remove the pipeline if CAS is disabled. The uniform was already being removed, which caused flickering / weirdness.

Testing

Tested the anti_alias example by toggling CAS a bunch on/off.

@kristoff3r kristoff3r added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Nov 27, 2024
@kristoff3r kristoff3r added this to the 0.15 milestone Nov 27, 2024
Copy link
Contributor

@Elabajaba Elabajaba left a comment

Choose a reason for hiding this comment

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

Tested it and it works.

@mockersf
Copy link
Member

Would that work correctly if the CasUniform component is removed and re-added in the same frame?

@kristoff3r
Copy link
Contributor Author

Would that work correctly if the CasUniform component is removed and re-added in the same frame?

No, that would break. But that can't happen with ExtractComponentPlugin afaik, so you'd have to add a custom extraction or render system just to break it.

I think it just works if we run removals first, because in the "added then removed" case the query would be empty.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Oh hey, this is a situation where @cart's "required components work on removal" would have solved that bug. Anyways, this is fine, merging.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 27, 2024
Merged via the queue into bevyengine:main with commit 3d72e08 Nov 27, 2024
28 checks passed
@kristoff3r kristoff3r deleted the cas-fix branch November 28, 2024 10:19
mockersf pushed a commit that referenced this pull request Nov 28, 2024
# Objective

Fixes #16531

I also added change detection when creating the pipeline, which
technically isn't needed but it felt weird leaving it as is.

## Solution

Remove the pipeline if CAS is disabled. The uniform was already being
removed, which caused flickering / weirdness.

## Testing

Tested the anti_alias example by toggling CAS a bunch on/off.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Dec 2, 2024
# Objective

Fixes bevyengine#16531

I also added change detection when creating the pipeline, which
technically isn't needed but it felt weird leaving it as is.

## Solution

Remove the pipeline if CAS is disabled. The uniform was already being
removed, which caused flickering / weirdness.

## Testing

Tested the anti_alias example by toggling CAS a bunch on/off.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
# Objective

Fixes bevyengine#16531

I also added change detection when creating the pipeline, which
technically isn't needed but it felt weird leaving it as is.

## Solution

Remove the pipeline if CAS is disabled. The uniform was already being
removed, which caused flickering / weirdness.

## Testing

Tested the anti_alias example by toggling CAS a bunch on/off.
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-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Toggling sharpening on then off results in a massively oversharpened image despite sharpening being disabled.
4 participants