Create OnDeferred to allow hooking into every ApplyDeferred action.

[andriyDev]

Objective

• This is a step towards Assets as Entities #11266. Any entities we create remotely (e.g., loading through an AssetServer), we'd like to initialize their asset entity as soon as possible (with metadata like its asset type). This allows us to create a hook to poll a channel on every apply deferred, allowing us to limit the latency from getting a handle to seeing its entity alive.

Solution

• Create a resource to which users can add their hook that will be executed on the next ApplyDeferred.

Testing

• Added tests.

---

Showcase

See release notes for more.

app.init_resource::<OnDeferred>();
app.world_mut().resource_mut::<OnDeferred>().add(|world: &mut World| {
    // Do command stuff.
});

[andriyDev]

I'm not sure this is the best API, but it's really direct.

In regards to Assets-as-entities, we can essentially add a hook like:

let (sender, receiver) = crossbeam_channel::unbounded();
app.world_mut().resource_mut::<OnDeferred>().add(move |world: &mut World| {
    for asset_handle in receiver.try_iter() {
        let command = InitializeAsset(asset_handle);
        command.apply(world);
    }
});
// Stick the sender in `AssetServer` or something. Whenever we give out a new handle, send a
// message on this channel to initialize the entity ASAP.

[urben1680]

Do I understand the intention correctly, for example this crossbeam channel has no way to trigger these actions itself so you wait for the next best sync point to "pull" the events form the channel and apply them?

Does this need to happen at every sync point?

[urben1680]

I would prefer that this test does not potentially block so a bug would not cause deadlocking the test here. Generally even try_lock is not the best way to ensure the order is correct as, in theory, both the system and the closure could run in parallel and then the test would only randomly fail.

I know, they cannot run in parallel because of the exclusive world access of the closure you add, but just the other day we realized we mistakenly allow &mut World in observers. So better the test is sturdy against other bugs.

Same at the other lock below and the single threaded test.

My suggestion is to put the vector here in a resource.

[urben1680]

Suggested change

	of as a command that executes at every sync point.
	of as a command that executes once at every sync point.

In contrast to "proper" commands that can run multiple times at a single sync point if they get queued again by other commands/hooks/observers.

[urben1680]

Don't you want to run this at the end of the sync point so the closures see the effects of commands/hooks/observers? Or do you want it to be the other way around, that these see the effect of OnDeferred?

Might be worth renaming this into either PreDeferred or PostDeferred.

[ElliottjPierce]

I haven't reviewed yet, just trying to think this through conceptually.

What we need is this pipeline for assets as entities on a load:

• A load is requested.
• The asset entity is allocated.
• The asset entity is constructed with asset metadata: eg load progress, asset type, asset path, etc.
• Bevy tasks polls the load to completion.
• The final asset is inserted.

The first three steps sound like a job for Commands. The fourth one is already being done somehow. And the last step sounds like it could done just by making the task return a command that spawns the asset instead of returning the asset itself. That would allow more flexibility with how the asset is loaded. Ex: modifying the asset after load but before insert, etc.

It sounds like this pr just creates a way to do this without commands. Bringing in commands would require a new system param though and would be a migration pain.

If migration weren't a thing, I think there would be better ways to do this. And I would caution against this route if the primary goal here is easier migration. But that's definitely not my decision to make. (For good reason.)

If we do want to do assets as entities without needing to migrate from Res, I think this is a very good design. But it would be easier to migrate before 1.0. I'll do a full review soon, but that's my initial thoughts.

[chescock]

I think you need to update SimpleExecutor, too. It doesn't have a separate apply_deferred step because it uses System::run to apply buffers immediately when a system runs. So you probably want to call OnDeferred::execute(world); after each system is run.

On the other hand, SimpleExecutor is deprecated and will probably be removed in 0.18, so it might not be a big deal if it doesn't support this.

[chescock]

The init and get could be combined with World::get_resource_or_init to make this a one-liner:

Suggested change

	app.init_resource::<OnDeferred>();
	app.world_mut().resource_mut::<OnDeferred>().add(|world: &mut World| {
	app.world_mut().get_resource_or_init::<OnDeferred>().add(|world: &mut World| {

[chescock]

Why won't the more standard approaches work for assets? I would have expected assets to be used at known points in each frame (maybe during extract?) and that scheduling a system to read a queue right before that would be sufficient.

[ElliottjPierce]

This all looks good to me. I'm still not sure if this is the best approach for assets as entities, but it's definitely a good implementation. And it might be useful for other things too. Hooking into sync points is really powerful. For me, it's just the "why not commands?" that gives me pause, but there may be good reasons for assets as entities and other situations too. IDK.