big-brain icon indicating copy to clipboard operation
big-brain copied to clipboard

Should `Score` be spawning on a new child entity?

Open OvermindDL1 opened this issue 3 years ago • 4 comments

Looking through the code and I notice Score is being spawned on a new child entity of what's actually performing the AI, why not instead make Score generic (via a PhantomData) over the Picker (which yes means propagating the generic up through to the Thinker, making Thinker generic over the Picker, but that seems reasonable and would allow removal of another allocation by letting you get rid of the Arc around the Picker, and thus also getting rid of the dynamic pick call as well), or is it this way for another reason that I'm not seeing in the code? By putting the Picker-generic-Score on the same entity as the AI as well means you can get rid of another query in the systems and have it all in the same query, meaning no dynamic entity lookup is needed either, which saves even more processing time in tight loops. Potentially you could even reuse the same score the same pickers that may exist on multiple thinkers if someone does that as well, saving even more processing.

OvermindDL1 avatar Aug 19 '22 17:08 OvermindDL1

I'm confused. How do you picture the Score being generic over the picker? Scores must be 1:1 associated with individual Scorers, and you can, and usually do, have multiple Score instances associated with a single Thinker. They can't be parallelized or reused like this, and I don't see how trying to do that could possibly work.

zkat avatar Aug 19 '22 20:08 zkat

Oh I don't see what I was saying with the Picker, I meant Scorers looking over it again, which would require a type on the thinker as well to disambiguate it, but is doable...

What I'm trying to accomplish is just removing the cost of having to perform an indirect get on another query as it's significantly more costly. If the entire thinker and each component managed by it were type tagged to be a unique type then children entities wouldn't be needed and it would be a pure table lookup in that case, basically as fast as it can get, especially compared to jumping around memory (though would necessitate needing to create empty types to 'tag' the things in the usage, which is quite fine for me but is definitely a bit more usability overhead).

But if things were type tagged then a ThinkerBuilder could take a phantom type (to make for a unique ActionState that could be stored straight on the main entity instead of as another entity. Basically ActionBuilder::spawn_action (and ScorerBuilder) could be reduced to just the build step onto the actor, ditto with the Scores, etc...

Although this would run substantially faster than the current method it would mean you couldn't runtime conditionally add/remove thinker choices and such, which I'm not sure if that's done that couldn't just be it's own thinker in that case as well? Hmmm... could still virtual dispatch the builders, the scorer systems would need a minor change to type the information, basically changing, say the thirst example's scorer system from what it original is typed as of:

pub fn thirsty_scorer_system(
    thirsts: Query<&Thirst>,
    mut query: Query<(&Actor, &mut Score), With<Thirsty>>,
) {

To instead be something like:

pub fn thirsty_scorer_system(
    mut query: Query<(&Thirst, &mut Score<Thirsty>)>,
) {

And likewise the action sytem would change from being this in the thirst example:

fn drink_action_system(
    time: Res<Time>,
    mut thirsts: Query<&mut Thirst>,
    mut query: Query<(&Actor, &mut ActionState, &Drink)>,
) {

To instead be like:

fn drink_action_system(
    time: Res<Time>,
    mut query: Query<(&mut Thirst, &mut ActionState<Thirsty>, &Drink)>,
) {

That removes multiple levels of indirection and allows for vastly more optimizations for the query loop as well since it doesn't need to perform the slow indirect thirsts.get_mut(*actor) call anymore...

Is it worth the extra type tag? I think so? Hmm...

OvermindDL1 avatar Aug 19 '22 21:08 OvermindDL1

This would mean that the built-in scorers like FixedScore and so forth would also need a type tag to be unambiguous, which would mean they'd need their own systems registered as well for each variant that would be used, that's just an extra setup call for them on program load so that's simple enough.

(Likewise for the steps_system and concurrent_system as well, but there's probably a way to simplify those accesses, perhaps by completely typing a Thinker and all the children (in a tuple, like how queries are done), and that would act as a registration for systems as well as a way to register it into an entity as well.)

OvermindDL1 avatar Aug 19 '22 21:08 OvermindDL1

ah, I see what you mean now.

How do you handle parameters on the Scorers themselves? The thirst example keeps it simple, but sometimes you actually want to actually pass parameters in. All the built-in Scorers do this:

#[derive(Component, Debug)]
pub struct AllOrNothing {
    threshold: f32,
    scorers: Vec<ScorerEnt>,
}

impl AllOrNothing {
    pub fn build(threshold: f32) -> AllOrNothingBuilder {
        AllOrNothingBuilder {
            threshold,
            scorers: Vec::new(),
        }
    }
}

pub fn all_or_nothing_system(query: Query<(Entity, &AllOrNothing)>, mut scores: Query<&mut Score>) {
  ...
}

zkat avatar Aug 20 '22 00:08 zkat

I'd imagine that would just be another typed component on the entity for fast lookup?

OvermindDL1 avatar Sep 27 '22 22:09 OvermindDL1

on the actor? Then you'd only be able to have a single set of parameters for a single scorer of that type on the actor, period, as opposed to reusing a parameterizable component.

So, you wouldn't be able to have more than one AllOrNothing scorer across the entire Thinker, which isn't ok.

zkat avatar Sep 27 '22 22:09 zkat