Skip to content

Make specialization cache cleanup respect material swaps #20993

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 13, 2025
Merged

Make specialization cache cleanup respect material swaps #20993

merged 4 commits into from
Sep 13, 2025

Conversation

tychedelia
Copy link
Member

Objective

Swapping material types can break specialization caches.

Fixes #20992.

Solution

Make sure that type erased bookkeeping (i.e. RenderMaterialInstances) runs first so that we can know whether the material was actually removed or just swapped.

Testing

Example in #20992. Tested also with 2d and didn't seem to be an issue there.

@tychedelia tychedelia 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 labels Sep 12, 2025
@tychedelia tychedelia added this to the 0.17 milestone Sep 12, 2025
@janis-bhm
Copy link
Contributor

janis-bhm commented Sep 12, 2025 •
edited
Loading

Tested also with 2d and didn't seem to be an issue there.

bevy_sprite_render doesn't have type erased materials so different materials don't clash there.

@tychedelia
Copy link
Member Author

bevy_sprite_render doesn't have type erased materials so different materials don't clash there.

Yeah, my concern was that this is kinda a race in the typed extract systems. I think (?) this wouldn't trigger if NewMaterial runs after OldMaterial... But 2d also doesn't have the late/early extract distinction that was put in place to solve this. Regardless, the long term solution here is to unify the infrastructure so we don't have to deal with the 2d/3d copy/paste churn.

mogambro
mogambro approved these changes Sep 13, 2025
View reviewed changes
Copy link
Contributor

@mogambro mogambro left a comment

Choose a reason for hiding this comment

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

I don't know much about this part of bevy at all, but going after the comments and the small change, I will still approve it, because it looks very well informed.

atlv24
atlv24 approved these changes Sep 13, 2025
View reviewed changes
Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

nice fix, cant wait to yeet those generics lol

@mockersf mockersf added this pull request to the merge queue Sep 13, 2025
Merged via the queue into bevyengine:main with commit b5e9751 Sep 13, 2025
32 checks passed
@janhohenheim janhohenheim mentioned this pull request Sep 14, 2025
Merged
@Affinator Affinator mentioned this pull request Sep 15, 2025
Open
mockersf pushed a commit that referenced this pull request Sep 15, 2025
@tychedelia @mockersf
Make specialization cache cleanup respect material swaps (#20993)
42aeca6 
# Objective

Swapping material types can break specialization caches.

Fixes #20992.

## Solution

Make sure that type erased bookkeeping (i.e. `RenderMaterialInstances`)
runs first so that we can know whether the material was actually removed
or just swapped.

## Testing

Example in #20992. Tested also with 2d and didn't seem to be an issue
there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

+2 more reviewers

@atlv24 atlv24 atlv24 approved these changes

@mogambro mogambro mogambro approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Rendering
Status: Done
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

Swapping Materials in a Component Hook panics
6 participants
@tychedelia @janis-bhm @alice-i-cecile @mockersf @atlv24 @mogambro