Arcify one-shot systems to enable SystemId templating#24072
Arcify one-shot systems to enable SystemId templating#24072ItsDoot wants to merge 4 commits intobevyengine:mainfrom
Arcify one-shot systems to enable SystemId templating#24072Conversation
i.e. they can't use one-shot systems until a rust feature is stabilized
laundmo
left a comment
There was a problem hiding this comment.
I'm pretty sure the fact all registered systems have changed over from BoxedSystem to SystemArc is gonna cause some performance regressions.
See the comments for an idea which would not require this, tho i'm not sure how doable it is.
| fn build_template(&self, context: &mut TemplateContext) -> Result<Self::Output> { | ||
| match self { | ||
| Self::Id(id) => Ok(*id), | ||
| Self::System(system) => Ok(context | ||
| .entity | ||
| .world_scope(|world| world.register_system_arc(system.clone()))), | ||
| } | ||
| } | ||
|
|
||
| fn clone_template(&self) -> Self { | ||
| match self { | ||
| Self::Id(id) => Self::Id(*id), | ||
| Self::System(system) => Self::System(system.clone()), | ||
| } | ||
| } |
There was a problem hiding this comment.
To me, this reads like each time clone_template and then build_template are called, a new SystemId would be registered. I don't know if this has any negative effects, as the Arc means it'll be the same underlying dyn System, but at least it produces more SystemIds than i think should be necessary.
Idea: Would it be possible to store something like a Either<BoxedSystem, SystemId> in SystemIdTemplate::System (behind a mutex or similar), such that the SystemId from the first time this system is registered can be re-used for subsequent times this system gets registered?
If that works, it would, i think, remove the necessity for SystemArc by moving the Arc<Mutex> needed to make a system clone-able for clone_template into the SystemIdTemplate scope.
Another idea i had was only allowing a set kind of system in Self::System, or having 2-3 variants for different kinds of systems, as some like FunctionSystem implement Clone
| // Wrap the system locking in a block to ensure it gets dropped before we flush commands. | ||
| // This is needed to allow systems to recursively call themselves. | ||
| let result = { | ||
| let mut system = system.lock(); |
There was a problem hiding this comment.
This is one of the places i fear a performance regression especially for heavy users of one-shot systems might happen
| pub(crate) struct RegisteredSystem<I: SystemInput + 'static, O: 'static> { | ||
| system: SystemArcDyn<I, O>, | ||
| } |
There was a problem hiding this comment.
I'm concerned that completely switching over every RegisteredSystem to SystemArc will have performance implications even during on-shot system calls.
|
Closed in favor of #24087 |
Objective
I have a bunch of
Bundle-style code that I want to replace with the newbsn!Scene-style macro:However, there is currently no simple way to implement a
SystemIdTemplatebecause systems are not cloneable.Solution
SystemArc<S: System + ?Sized>, a wrapper around anArc<Mutex<S>>SystemArcs instead ofBoxedSystemsSystemIdTemplatethat makes use of the newSystemArctypesystem_valuefunction for wrapping system functions (see Future Work for potentially removing the need)Note: Until
#[derive(CoercePointee)](or some other rust feature that gives user-landCoerceUnsized) is stabilized, one-shot systems cannot be used on no-std-portable-atomic becauseportable_atomic_util::Arccan not doArc<T>toArc<dyn Trait>conversions likestd::sync::Arccan.Testing
SystemIdTemplate.callbacksexample demonstrating how to spawnSystemIds via BSN scenes.Future work
system_valueby introducingSuperFrom/SuperIntotraits a la Dioxus and using it in thebsn!macros in-place of the implicit.into()s.SystemArclays a bit of the foundation forArcifyingSchedules.Showcase
You can now spawn components containing
SystemIds viabsn!macros: