Tracked one-shot systems

[ItsDoot]

Objective

#24087 introduces scene templating for SystemIds, however it can result in a memory leak if a scene is re-constructed multiple times:

#24087 (comment)

This was proposed basically 1:1 in #24026 (this was later changed though). The issue is that it's unclear who owns these systems, that is who is responsible for unregistering them once they are no longer needed. Given that recreating the template will spawn the system again this basically becomes a memory leak.

#24087 (comment)

Hm, are you sure this is the case even tho in build_template it only registers the system the first time its called, switching over to storing the SystemId after the first call?

If you recreate the template (e.g. you call my_scene() again) then you will create a new instance of the system. And since the system is not scoped to the scene once the scene is despawned the system entity will be leaked.

Essentially, we need a way to connect the lifetime of the registered system to the lifetime of the scene.

Solution

This is a purely additive / opt-in / backwards-compatible version of #24114

Introducing: SystemHandles

pub enum SystemHandle<I: SystemInput = (), O = ()> {
    /// A strong handle keeps the system entity alive as long as the handle
    /// (and any clones of it) exist.
    Strong(Arc<StrongSystemHandle>),
    /// A weak handle does not keep the system entity alive.
    Weak(SystemId<I, O>),
}

pub struct StrongSystemHandle {
    entity: Entity,
    drop_sender: crossbeam_channel::Sender<Entity>,
}

Similar to bevy_asset::Handles,SystemHandle's custom Drop implementation enqueues the registered system entity into a message channel. The system despawn_unused_registered_systems pulls from the other end of this message channel and despawns the registered system entities.

World::register_tracked_system and World::register_tracked_boxed_system are the only functions that return SystemHandles.

Testing

• Added a test to ensure that despawn_unused_registered_systems does its job
• Added a test to ensure that the default app will automatically call despawn_unused_registered_systems

Future work

• SystemId scene templating #24087 will use this PR as a base

[chescock]

Looks good! I left some comments, but they're just style nits.

[chescock]

Since we already have a dependency on concurrent-queue, you might want to use ConcurrentQueue instead of crossbeam-channel. A queue is a little more lightweight since it doesn't support blocking, which you aren't using here.

[chescock]

You could use If<Res<_>> instead of Option<Res<_>> to skip the system instead of needing this if let.

[chescock]

It's too bad that you have to repeat the cfg so much. Would it make sense to put this type in a mod so that you only need one cfg? Or, does a bevy_platform::cfg::std! block work here?

[chescock]

This whole function is already in a cfg, right?

Suggested change

#[cfg(feature = "std")]

[chescock]

... and here

Suggested change

#[cfg(feature = "std")]

[chescock]

Do we need to worry about this monomorphizing two versions of this function?

[chescock]

The PartialEq and Hash impls also need to be manual so that Weak == Strong for the same Entity, right?

[chescock]

The "keeps the entity alive" phrasing might be a little misleading here, since this is the thing inside the Arc.

Suggested change

	/// A strong handle for a registered system that keeps the system entity alive
	/// as long as the handle (and any clones of it) exist.
	/// A strong handle for a registered system that despawns the entity when dropped.

[chescock]

You can simplify this a bit with run_system_once or run_system_cached.

Suggested change

	let mut cleanup = IntoSystem::into_system(despawn_unused_registered_systems);
	cleanup.initialize(&mut world);
	cleanup.run((), &mut world).unwrap();
	world
	.run_system_cached(despawn_unused_registered_systems)
	.unwrap();