Factors don't belong in the state

[raymoo]

From looking at the code, it looks like factors give input to states by setting fields in the state. This seems ugly and unnecessarily stateful, and the fact that the factors are nil-ed right in start after being checked suggests that they are not conceptually part of the state.

Why not pass factor parameters into the state as a parameter to start? It would remove the need for the factor = nil boilerplate and leave less room for error.

[sofar]

This was a contention in my design drafts, and I'm not sure I thought it through properly.

We want to retain possible other factors through state transitions, since we may want to carry over state information without erasing it, but perhaps we should leave the more permanent retaining of state to the driver (e.g. if something needs keeping state, have the driver allocate a table entry).

I need to look at the code (it needs better organization either way) a bit.

[sofar]

We also do want to be able to save/restore factor data. If a mob gets hit and runs out of range and gets unloaded, it should continue fleeing from it's attacker once the attacker gets close again and the entity gets loaded again.

[raymoo]

What if states can specify a serialization/deserialization callback? Or just give states a "persistent table" to work with that gets serialized with the state. It could be used for more general things than just storing the factors.

[sofar]

OK, I've got my bearings a bit again now.

2 types of factors:

1) boilerplate factor code that consider_factors runs the functions (stuff from factors.lua) 2) external functions that provide factor data to the entity

we can refactor 1) to return factordata and then we can pass this through to `self.driver:switch(driver, factordata) just fine, since that would get the data in the right place.

But how do we solve 2)? I don't think we should allow on_punch to call self.driver:switch(newdriver, factordrver) since

a) it doesn't even know what newdriver needs to be b) it doesn't even know if the factor is relevant for the current state of self

and telling people to put lots of code in special functions that does sort that out makes little sense imho.

I'm thinking of alternatives... nothing yet.

[raymoo]

How is 2 currently solved? I'm still not familiar with most of the code

[sofar]

on_punch (see init.lua) just inserts a factor record into state.factors.

https://github.com/sofar/entity_ai/blob/master/init.lua#L216

[sofar]

perhaps we just need a generic implementation that handles 1) and 2) above.

self.driver:factor(name, factordata)

then that code can do a) and b) for it.

then the entity on_punch can simply do:

self.driver:factor("got_hit", {
        puncher = puncher:get_player_name(),
        time_from_last_punch = time_from_last_punch,
        ...
})

But I still don't like having 2 locations that account for factors...

[raymoo]

To make sure I understand, with the hypothetical changes we would have:

• Factors can be registered, and these would cause a factor to apply if they return some non-nil data.
• State drivers have a method to apply a factor directly, regardless of registration.

Is this right? If the loop checking for registered factors itself used the method from the second point, maybe it would require less duplication?

[sofar]

Registered factors are part of the state model. That means that they are only considered (code is run...) when the entity is in the required state. (this is 1) boilerplate factor code)

This is done for efficiency, obviously. Less factor code to run == better.

Then there's event driven factors like getting punched, or right-clicked. These factors have a handle to self and that handle is the only way to leave data behind for those type of factors. (this is 2))

Right now the design works by directly leaving behind the factordata in self.factors. The decision to act on the factor is done in entity_ai_on_step which calls consider_factors. That function then can see factors from either 1) or 2) since they're left in self.factors irregardless of origin.

Any proposed change would have to use a method instead of some static table data. So yes, that confirms your last comment.

Looking at the code, we already have a fairly redundant entity_ai_on_step() function that calls a local consider_factors() but I think we could move this entirely into self.driver:step()

Then the only thing remaining would be for factor data to call a method in the driver directly to signal some factordata is present, but that data may as well get discarded by the driver if it's irrelevant. But at least that code can then decide to switch drivers properly if it is relevant.

[sofar]

Thanks a ton for opening the discussion. This was incredibly useful.

I've changed this according to the discussion. Please dig into the tree again since so much has changed!

5b87f4e (untested, no time right now to actually run this for a bit and test every angle)

I'm leaving this open for now, I'm fairly sure we can do more work.

[sofar]

Well, that entirely broke it.

I've spent a whole evening doing it step-by-step. At least I think I've got it working OK now, and it was worth the cleanup. A lot of code moved into driver.lua as a result, but that makes a lot of sense.

I've force-pushed the broken commits away, there was nothing useful in them.

Please take the time to review, I really appreciate the eyes on the code!