bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.12k stars 3.45k forks source link

Hang when running custom system impl inspired by bevy's combinator systems, regression from 0.9 #8339

Open inodentry opened 1 year ago

inodentry commented 1 year ago

Bevy version

0.10.1

Regression from 0.9.1

System

OS: macOS 13.1 Ventura CPU: Apple M1 Pro

What you did

I have the following code. This is a custom system combinator akin to Bevy's PipeSystem, with the difference that the first system returns an Option<T> and the second system is only called if the first system returned Some.

I have carried this code forward over the past few versions of Bevy. It used to work. Now in 0.10 it no longer does. This is interesting, because I looked at Bevy's combinator system implementation (https://github.com/bevyengine/bevy/blob/v0.10.1/crates/bevy_ecs/src/system/combinator.rs#L124) for reference and made sure that my code mirrors that. I expect my thing to work, given that it is adapted from something that is already included in Bevy itself. With previous versions of Bevy, I used the implementation of PipeSystem as my reference to learn from.

Code ```rust pub struct ChainOptionalSystem { system_in: SystemIn, system_some: SystemSome, name: Cow<'static, str>, component_access: Access, archetype_component_access: Access, } impl>, SystemSome: System> System for ChainOptionalSystem { type In = SystemIn::In; type Out = O; fn name(&self) -> Cow<'static, str> { self.name.clone() } fn type_id(&self) -> std::any::TypeId { std::any::TypeId::of::() } fn archetype_component_access(&self) -> &Access { &self.archetype_component_access } fn component_access(&self) -> &Access { &self.component_access } fn is_send(&self) -> bool { self.system_in.is_send() && self.system_some.is_send() } fn is_exclusive(&self) -> bool { self.system_in.is_send() || self.system_some.is_send() } unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { if let Some(t) = self.system_in.run_unsafe(input, world) { self.system_some.run_unsafe(t, world) } else { O::default() } } fn apply_buffers(&mut self, world: &mut World) { self.system_in.apply_buffers(world); self.system_some.apply_buffers(world); } fn initialize(&mut self, world: &mut World) { self.system_in.initialize(world); self.system_some.initialize(world); self.component_access .extend(self.system_in.component_access()); self.component_access .extend(self.system_some.component_access()); } fn update_archetype_component_access(&mut self, world: &World) { self.system_in.update_archetype_component_access(world); self.system_some.update_archetype_component_access(world); self.archetype_component_access .extend(self.system_in.archetype_component_access()); self.archetype_component_access .extend(self.system_some.archetype_component_access()); } fn check_change_tick(&mut self, change_tick: u32) { self.system_in.check_change_tick(change_tick); self.system_some.check_change_tick(change_tick); } fn get_last_change_tick(&self) -> u32 { self.system_in.get_last_change_tick() } fn set_last_change_tick(&mut self, last_change_tick: u32) { self.system_in.set_last_change_tick(last_change_tick); self.system_some.set_last_change_tick(last_change_tick); } fn default_system_sets(&self) -> Vec> { let mut default_sets = self.system_in.default_system_sets(); default_sets.append(&mut self.system_some.default_system_sets()); default_sets } } pub trait IntoChainOptionalSystem: IntoSystem<(), Option, ParamIn> + Sized where SysSome: IntoSystem, { fn chain_optional(self, system: SysSome) -> ChainOptionalSystem; } impl IntoChainOptionalSystem for SysIn where SysIn: IntoSystem<(), Option, ParamIn>, SysSome: IntoSystem, { fn chain_optional(self, system: SysSome) -> ChainOptionalSystem { let system_in = IntoSystem::into_system(self); let system_some = IntoSystem::into_system(system); ChainOptionalSystem { name: Cow::Owned(format!("ChainOptional({} -> {})", system_in.name(), system_some.name())), system_in, system_some, archetype_component_access: Default::default(), component_access: Default::default(), } } } ```

I am using this thing as a building block for handling Bevy UI buttons, among other things.

Code ```rust #[derive(Component)] pub struct UiDisabled; pub fn button_handler(handler: impl IntoSystem) -> impl System { on_button_interact.chain_optional(handler) } fn on_button_interact( query: Query<(&Interaction, &B), (Changed, With

It could be used like this:

Code ```rust // Example button implementation that transitions to a specific state #[derive(Component, Clone)] struct StateTransitionButton(AppState); fn setup_menu(/* ... */) { // new game button commands.spawn(( StateTransitionButton(AppState::LoadingGame), ButtonBundle { // ... }, )); // settings button commands.spawn(( StateTransitionButton(AppState::Settings), ButtonBundle { // ... }, )); // credits button commands.spawn(( StateTransitionButton(AppState::ShowCredits), ButtonBundle { // ... }, )); } fn handle_state_transition_button( In(btn_data): In, // arbitrary system params mut state: ResMut>, ) { state.set(btn_data.0); } ``` ```rust app.add_system(setup_menu.in_schedule(OnEnter(AppState::Menu))); app.add_system(button_handler(handle_state_transition_button)); ```

What went wrong

If I add the button handler system to my app, Bevy hangs when it tries to run it. The window becomes frozen and unresponsive.

Additional information

Like I said, this exact thing worked in previous versions of Bevy. I am just porting it to 0.10 now. I made my custom impl System by looking at the code of Bevy's PipeSystem, and now the new generic combinator system.

I tried running my app in lldb to try to track down what is going on. I could keep stepping long after my run_unsafe impl, past the end of the body of the task spawned by bevy's multithreaded executor. Eventually I reached here: https://github.com/bevyengine/bevy/blob/v0.10.1/crates/bevy_tasks/src/task_pool.rs#L500 and that is when the hang happened. Continuing to step the debugger only showed assembly listings from libpthread. I suspect we are getting stuck in this execute_forever thing. I do not understand this code and I have no idea what it does and why. It seems to have been introduced in #7415 .

I am stumped. Please help. This whole thing is so weird.

JoJoJet commented 1 year ago

Not sure why this is hanging, but a workaround should be possible.

I have the following code. This is a custom system combinator akin to Bevy's PipeSystem, with the difference that the first system returns an Option and the second system is only called if the first system returned Some.

This should be doable using CombinatorSystem, by implementing the Combine trait:

pub type ChainOptionalSystem<A, B> = CombinatorSystem<ChainOptionalMarker, A, B>;

#[doc(hidden)]
pub struct ChainOptionalMarker;

impl<A, B> Combine<A, B> for ChainOptionalMarker
where
     A: System<Out = Option<B::In>>,
     B: System,
{
    type In = A::In;
    type Out = B::Out;
    fn combine(
        input: A::In,
        a: impl FnOnce(A::In) -> A::Out,
        b: impl FnOnce(B::In) -> B::Oout
    ) -> B::Out {
        if let Some(value) = a(input) {
            b(value)
        } else {
            todo!()
        }
    }
}
inodentry commented 1 year ago

Well, thanks! :) Sure, I can get the job done in other ways.

I also had another similar custom System impl for Result, which calls a different secondary system depending on whether the first system returns Ok or Err. I don't think that's (directly) representable using the Combine trait, because it has 3 systems. Fortunately, I am not really using it anywhere important, so it's not a deal breaker to me that it no longer works.

It would be good to figure out why it is hanging, though. There is a chance that it might be an issue that goes deeper in bevy.

If you have any ideas for anything I could investigate, let me know! :)

JoJoJet commented 1 year ago

I think that @hymm or @james7132 would be more likely to be able to help with the problem at hand. I may be able to help with another workaround though.

If I understand your requirements correctly, your result combinator should be expressible using something like:

type ResultSystem<A, B> = CombinatorSystem<A, B, ResultCombinator>

pub struct ResultCombinator;

impl<A, B> Combine<A, B> for ResultCombinator
where
     A: System,
     B: System<Out = A::Out>,
{
    type In = Result<A::In, B::In>;
    type Out = A::Out;
    fn combine(
        input: Self::In,
        a: impl FnOnce(A::In) -> A::Out,
        b: impl FnOnce(B::In) -> B::Oout
    ) -> Self::Out {
        match input {
            Ok(x) => a(x),
            Err(e) => b(e),
        }
    }
}

fn system_a() -> Result<T, U> {}
fn system_ok(In(_): In<T>) {}
fn system_err(In(_): In<U>) {}

schedule.add_systems(system_a.pipe(ResultSystem::new(system_ok, system_err)));
hymm commented 1 year ago

is there a minimal example or public code I could use to check this?