`otherwise` pre-empts further steps in sequential actions

[ndarilek]

Not sure if this is desired behavior. If I have a sequential action whose first action scorer stops returning its scoring success criteria, sequential actions keep executing. If I have an otherwise action, that otherwise action seems to run instead of the sequential actions.

I'm guessing what's happening here is that the picker returns no action. Then for some reason, when sequential actions are requested without an otherwise, the sequence runs. But if otherwise is configured, it runs instead and the sequence stops.

I have a minimal reproduction below. It does what I'd expect until the otherwise line is uncommented, then it breaks.

My actual use case is that systems tag entities with components based on their proximity to things. So if an enemy is within a certain range of a kill, it hears it. That in turn trips the scorer, which kicks off an action, which then clears whatever data tripped the scorer. This basically kicks off a whole "investigate/hunt around/return to post" cycle, but in the middle of that cycle I want the option for a new kill to be heard, thus basically failing the current investigate/hunt routine and starting it over again at the new coordinates. And I think that works, until I try adding in an idle routine to give them an at-rest behavior. Then it breaks. I don't think the at-rest behavior should win when a sequential action is running, even if no scorer trips.

Anyhow, here's the example. If in stage 1 the Increment action increments to stage 2. The InStage1 scorer then fails, but with the otherwise line commented, the subsequent LogIt action without an associated scorer triggers. Uncomment otherwise and the only action that runs is Idle.

Thanks for all your work on this library!

use bevy::prelude::*;
use big_brain::prelude::*;

fn main() {
    App::new()
        .add_plugins(MinimalPlugins)
        .add_plugin(BigBrainPlugin)
        .add_startup_system(setup)
        .add_system_to_stage(BigBrainStage::Scorers, in_stage1_scorer)
        .add_system_to_stage(BigBrainStage::Actions, increment)
        .add_system_to_stage(BigBrainStage::Actions, log_it)
        .add_system_to_stage(BigBrainStage::Actions, idle)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn().insert(Stage(1)).insert(
        Thinker::build()
            .picker(FirstToScore::new(1.))
            .when(InStage1, Steps::build().step(Increment).step(LogIt))
            // .otherwise(Idle),
    );
}

#[derive(Component, Deref, DerefMut, Debug)]
struct Stage(u8);

#[derive(Component, Default, Clone, Debug)]
struct InStage1;

fn in_stage1_scorer(
    mut actors: Query<(&Actor, &mut Score), With<InStage1>>,
    stages: Query<&Stage>,
) {
    for (Actor(actor_entity), mut score) in &mut actors {
        if let Ok(stage) = stages.get(*actor_entity) {
            if **stage == 1 {
                println!("In stage 1");
                score.set(1.);
            } else {
                score.set(0.);
            }
        } else {
            score.set(0.);
        }
    }
}

#[derive(Component, Default, Debug, Clone)]
struct Increment;

fn increment(
    mut actors: Query<(&Actor, &mut ActionState), With<Increment>>,
    mut stages: Query<&mut Stage>,
) {
    for (Actor(actor_entity), mut state) in &mut actors {
        if let Ok(mut stage) = stages.get_mut(*actor_entity) {
            match *state {
                ActionState::Requested => {
                    println!("Increment requested");
                    *state = ActionState::Executing;
                }
                ActionState::Executing => {
                    println!("Incrementing");
                    **stage += 1;
                    *state = ActionState::Success;
                }
                _ => {}
            }
        }
    }
}

#[derive(Component, Default, Debug, Clone)]
struct LogIt;

fn log_it(mut actors: Query<(&Actor, &mut ActionState), With<LogIt>>, stages: Query<&Stage>) {
    for (Actor(actor_entity), mut state) in &mut actors {
        if let Ok(stage) = stages.get(*actor_entity) {
            match *state {
                ActionState::Requested => {
                    println!("Logging requested");
                    *state = ActionState::Executing;
                }
                ActionState::Executing => {
                    println!("Stage: {:?}", stage);
                    *state = ActionState::Executing;
                }
                _ => {}
            }
        }
    }
}

#[derive(Component, Default, Debug, Clone)]
struct Idle;

fn idle(mut actors: Query<(&Actor, &mut ActionState), With<Idle>>) {
    for (Actor(actor_entity), mut state) in &mut actors {
        match &*state {
            ActionState::Requested => {
                println!("Idle requested");
                *state = ActionState::Executing;
            }
            ActionState::Executing => {
                println!("Idle executing");
                *state = ActionState::Executing;
            }
            state => {
                println!("Idle {:?}", state);
            }
        }
    }
}

[zkat]

you are correct, that this is what's happening, and you bring up a good point. Sounds like things should be changed such that, if your Picker is no longer returning a result, the current action should be cancelled. I'm not sure the inverse (letting the sequence run until completion) is a good idea because it kinda goes against the whole picking/cancellation system, tbh. It does mean you'll need to reconsider your approach to the problem a bit. For example, you can have a higher-priority scorer meant for listening to these "new" danger events.

The relevant code is here:

https://github.com/zkat/big-brain/blob/main/src/thinker.rs#L239-L257

[ndarilek]

OK, I think what you're saying is that the picker always has to return success for the entire action sequence to continue? I think that ultimately makes sense and I can get behind it. It should definitely be consistent though. :)

I'll look into PRing something in the next week or two. Thanks for the code pointer.

[zkat]

Oh a PR would be fantastic! But yes to consistency, and yes to the fact that the design is such that “this action will only continue executing as long as its condition tensions true”

[zkat]

heads-up: I'm gonna pick this up, since I'm working on big-brain a bit this weekend, and the new observability/logging stuff will make this easier to debug.