Skip to content

Fix the ordering of the systems introduced in #18734. #18825

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

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Apr 12, 2025

There's still a race resulting in blank materials whenever a material of type A is added on the same frame that a material of type B is removed. PR #18734 improved the situation, but ultimately didn't fix the race because of two issues:

  1. The late_sweep_material_instances system was never scheduled. This PR fixes the problem by scheduling that system.

  2. early_sweep_material_instances needs to be called after every material type has been extracted, not just when the material of that type has been extracted. The chain() added during the review process in PR Unify RenderMaterialInstances and RenderMeshMaterialIds, and fix an associated race condition. #18734 broke this logic. This PR reverts that and fixes the ordering by introducing a new SystemSet that contains all material extraction systems.

I also took the opportunity to switch a manual reference to AssetId::<StandardMaterial>::invalid() to the new DUMMY_MESH_MATERIAL constant for clarity.

Because this is a bug that can affect any application that switches material types in a single frame, I think this should be uplifted to Bevy 0.16.

There's still a race resulting in blank materials whenever a material of
type A is added on the same frame that a material of type B is removed.
PR bevyengine#18374 improved the situation, but ultimately didn't fix the race
because of two issues:

1. The `late_sweep_material_instances` system was never scheduled. This
   PR fixes the problem by scheduling that system.

2. `early_sweep_material_instances` needs to be called after *every*
   material type has been extracted, not just when the material of
   *that* type has been extracted. The `chain()` added during the review
   process in PR bevyengine#18374 broke this logic. This PR reverts that and fixes
   the ordering by introducing a new `SystemSet` that contains all
   material extraction systems.

I also took the opportunity to switch a manual reference to
`AssetId::<StandardMaterial>::invalid()` to the new
`DUMMY_MESH_MATERIAL` constant for clarity.

Because this is a bug that can affect any application that switches
material types in a single frame, I think this should be uplifted to
Bevy 0.16.
@pcwalton pcwalton added the A-Rendering Drawing game state to the screen label Apr 12, 2025
@pcwalton pcwalton added this to the 0.16 milestone Apr 12, 2025
@pcwalton pcwalton added S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Bug An unexpected or incorrect behavior labels Apr 12, 2025
@pcwalton pcwalton changed the title Fix the ordering of the systems introduced in #18374. Fix the ordering of the systems introduced in #18734. Apr 12, 2025
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Makes sense, apologies for missing the missing system on the last review. Thanks for tracking this down with all the churn.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

I think it was me who made the chain comment. I didn’t realise that consequence indeed. Sorry for the lost time :/

@superdump
Copy link
Contributor

error: public documentation for `ExtractMaterialsSet` links to private item `extract_mesh_materials`
   --> crates/bevy_pbr/src/material.rs:638:41
    |
638 | /// A [`SystemSet`] that contains all [`extract_mesh_materials`] systems.
    |                                         ^^^^^^^^^^^^^^^^^^^^^^ this item is private
    |
    = note: this link resolves only because you passed `--document-private-items`, but will break without
    = note: `-D rustdoc::private-intra-doc-links` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(rustdoc::private_intra_doc_links)]`

error: could not document `bevy_pbr`

@pcwalton pcwalton 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 Apr 13, 2025
@mockersf mockersf added this pull request to the merge queue Apr 14, 2025
Merged via the queue into bevyengine:main with commit 56784de Apr 14, 2025
34 checks passed
mockersf pushed a commit that referenced this pull request Apr 14, 2025
There's still a race resulting in blank materials whenever a material of
type A is added on the same frame that a material of type B is removed.
PR #18734 improved the situation, but ultimately didn't fix the race
because of two issues:

1. The `late_sweep_material_instances` system was never scheduled. This
PR fixes the problem by scheduling that system.

2. `early_sweep_material_instances` needs to be called after *every*
material type has been extracted, not just when the material of *that*
type has been extracted. The `chain()` added during the review process
in PR #18734 broke this logic. This PR reverts that and fixes the
ordering by introducing a new `SystemSet` that contains all material
extraction systems.

I also took the opportunity to switch a manual reference to
`AssetId::<StandardMaterial>::invalid()` to the new
`DUMMY_MESH_MATERIAL` constant for clarity.

Because this is a bug that can affect any application that switches
material types in a single frame, I think this should be uplifted to
Bevy 0.16.
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 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.

5 participants