Skip to content

Fix panic when specializing materials for entities spawned in PostUpdate #19064

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

Open
grind086 wants to merge 1 commit into
base: main
Choose a base branch
from
+8 −12
Open

Fix panic when specializing materials for entities spawned in PostUpdate #19064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions crates/bevy_pbr/src/material.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::meshlet::{
};
use crate::*;
use bevy_asset::prelude::AssetChanged;
use bevy_asset::{Asset, AssetEvents, AssetId, AssetServer, UntypedAssetId};
use bevy_asset::{Asset, AssetId, AssetServer, UntypedAssetId};
use bevy_core_pipeline::deferred::{AlphaMask3dDeferred, Opaque3dDeferred};
use bevy_core_pipeline::prepass::{AlphaMask3dPrepass, Opaque3dPrepass};
use bevy_core_pipeline::{
Expand Down Expand Up @@ -286,12 +286,11 @@ where
.add_plugins((RenderAssetPlugin::<PreparedMaterial<M>>::default(),))
.add_systems(
PostUpdate,
(
mark_meshes_as_changed_if_their_materials_changed::<M>.ambiguous_with_all(),
check_entities_needing_specialization::<M>.after(AssetEvents),
)
mark_meshes_as_changed_if_their_materials_changed::<M>
.ambiguous_with_all()
.after(mark_3d_meshes_as_changed_if_their_assets_changed),
);
)
.add_systems(Last, check_entities_needing_specialization::<M>);
Comment on lines 287 to +293
Copy link
Contributor

@Henauxg Henauxg May 6, 2025

Choose a reason for hiding this comment

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

With option 2. described above, this would look like:

       .add_systems(
                PostUpdate,
                (
                    mark_3d_meshes_as_changed_if_their_assets_changed,
                    mark_meshes_as_changed_if_their_materials_changed::<M>.ambiguous_with_all(),
                    check_entities_needing_specialization::<M>,
                )
                    .chain()
                    .after(AssetEvents)
                    .after(VisibilitySystems::CheckVisibility),
            );

The chaining is not strictly necessary, we could just add the new aftercondition to check_entities_needing_specialization.
But that's also a point I'd like to discuss. Currently check_entities_needing_specialization needlessly? checks for

 Or<(
                Changed<Mesh3d>,
                AssetChanged<Mesh3d>,
                Changed<MeshMaterial3d<M>>,
                AssetChanged<MeshMaterial3d<M>>,
            )>,

But that is literally what mark_3d_meshes_as_changed_if_their_assets_changed and mark_meshes_as_changed_if_their_materials_changed seem to be there for (marking Mesh3d as changed) ? So chaining, and just checking for Changed<Mesh3d> in check_entities_needing_specialization would probably be simpler and achieve the expected goal.

Also:

  • with the current PR mark_meshes_as_changed_if_their_materials_changed and mark_3d_meshes_as_changed_if_their_assets_changed are not properly scheduled after AssetEvents
  • on main, mark_meshes_as_changed_if_their_materials_changed is not either, which could expain why all the Or parameters were introduced in check_entities_needing_specialization

Copy link
Contributor Author

@grind086 grind086 May 7, 2025

Choose a reason for hiding this comment

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

I think you may have misread something here. mark_3d_meshes_as_changed_if_their_assets_changed is from bevy_render, and isn't added here. The only ordering constraint related to it in either main or this PR should be mark_meshes_as_changed_if_their_materials_changed.after(mark_3d_meshes_as_changed_if_their_assets_changed) unless there's some other interaction happening.

The minimal change from main to ensure ordering relative to CheckVisibility would just be:

.add_systems(
    PostUpdate,
    (
        mark_meshes_as_changed_if_their_materials_changed::<M>.ambiguous_with_all(),
        check_entities_needing_specialization::<M>
            .after(AssetEventSystems)
            .after(VisibilitySystems::CheckVisibility),
    )
        .after(mark_3d_meshes_as_changed_if_their_assets_changed)
);

mark_3d_meshes_as_changed_if_their_assets_changed is also already explicitly ordered before asset events here:

bevy/crates/bevy_render/src/mesh/mod.rs

Lines 74 to 76 in 4051465

mark_3d_meshes_as_changed_if_their_assets_changed
.ambiguous_with(VisibilitySystems::CalculateBounds)
.before(AssetEventSystems),

Copy link
Contributor

@Henauxg Henauxg May 7, 2025

Choose a reason for hiding this comment

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

You're right about mark_3d_meshes_as_changed_if_their_assets_changed, it's only used for ordering in bevy_pbr, my bad.
I'm still surprised though that mark_3d_meshes_as_changed_if_their_assets_changed is ordered before AssetEvents all the while reading the asset events. If anyone knows why I'd gladly take an explanation.
And, even a bit worse, mark_meshes_as_changed_if_their_materials_changed is also reading asset events. Its execution order is not deterministic as its only ordered after mark_3d_meshes_as_changed_if_their_assets_changed which itself is before AssetEvents. So mark_meshes_as_changed_if_their_materials_changed can end up running before or after AssetEvents depending on scheduling (and as such, marking meshes as modifed or not depending on scheduling).

Copy link
Member

@janhohenheim janhohenheim May 7, 2025

Choose a reason for hiding this comment

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

Looks to me like you just uncovered another bug

Copy link
Member

@tychedelia tychedelia May 7, 2025

Choose a reason for hiding this comment

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

Thanks for thinking about this deeply. These indeed seem to be sketchy to me. I think that both these systems were implemented before the AssetChanged filter was added and so they are ad-hoc implementations of the same thing. We should be able to simply add that filter in extract_meshes_for_gpu_building for mark_3d_meshes_as_changed_if_their_assets_changed. mark_meshes_as_changed_if_their_materials_changed is a bit more complicated because it requires the material type, so needs to stay in place with the potential for fixing post #18075 potentially.

Copy link
Member

@janhohenheim janhohenheim May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think there's a chance that this scheduling ambiguity I just ran into is also caused by this.


if self.shadows_enabled {
app.add_systems(
Expand Down
9 changes: 3 additions & 6 deletions crates/bevy_sprite/src/mesh2d/material.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ use crate::{
DrawMesh2d, Mesh2d, Mesh2dPipeline, Mesh2dPipelineKey, RenderMesh2dInstances,
SetMesh2dBindGroup, SetMesh2dViewBindGroup, ViewKeyCache, ViewSpecializationTicks,
};
use bevy_app::{App, Plugin, PostUpdate};
use bevy_app::{App, Last, Plugin};
use bevy_asset::prelude::AssetChanged;
use bevy_asset::{AsAssetId, Asset, AssetApp, AssetEvents, AssetId, AssetServer, Handle};
use bevy_asset::{AsAssetId, Asset, AssetApp, AssetId, AssetServer, Handle};
use bevy_core_pipeline::{
core_2d::{
AlphaMask2d, AlphaMask2dBinKey, BatchSetKey2d, Opaque2d, Opaque2dBinKey, Transparent2d,
Expand Down Expand Up @@ -271,10 +271,7 @@ where
.init_resource::<EntitiesNeedingSpecialization<M>>()
.register_type::<MeshMaterial2d<M>>()
.add_plugins(RenderAssetPlugin::<PreparedMaterial2d<M>>::default())
.add_systems(
PostUpdate,
check_entities_needing_specialization::<M>.after(AssetEvents),
);
.add_systems(Last, check_entities_needing_specialization::<M>);
Copy link
Contributor

@Henauxg Henauxg May 6, 2025

Choose a reason for hiding this comment

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

Going with option 2 as described above

Suggested change
.add_systems(Last, check_entities_needing_specialization::<M>);
.add_systems(
PostUpdate,
check_entities_needing_specialization::<M>
.after(AssetEvents)
.after(VisibilitySystems::CheckVisibility),
);

Copy link
Contributor Author

@grind086 grind086 May 7, 2025

Choose a reason for hiding this comment

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

This was actually my initial choice for a fix, but @tychedelia mentioned on discord that moving the system to Last might be safer if it turns out there's no benefit to keeping it in LastUpdate. I think they were going to take a look when they had time.

Copy link
Contributor

@Henauxg Henauxg May 7, 2025

Choose a reason for hiding this comment

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

(Then we should at least move check_light_entities_needing_specialization to Last in bevy_pbr)


if let Some(render_app) = app.get_sub_app_mut(RenderApp) {
render_app
Expand Down