-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Fix the ordering of the systems introduced in #18734. #18825
Conversation
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.
There was a problem hiding this 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.
There was a problem hiding this 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 :/
|
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.
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:
The
late_sweep_material_instances
system was never scheduled. This PR fixes the problem by scheduling that system.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. Thechain()
added during the review process in PR UnifyRenderMaterialInstances
andRenderMeshMaterialIds
, and fix an associated race condition. #18734 broke this logic. This PR reverts that and fixes the ordering by introducing a newSystemSet
that contains all material extraction systems.I also took the opportunity to switch a manual reference to
AssetId::<StandardMaterial>::invalid()
to the newDUMMY_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.