ensemble-engine / ensemble

A rules-based AI framework for social simulation
Other
50 stars 11 forks source link

ensemble.doAction() doesn't actually work #105

Closed mkremins closed 4 years ago

mkremins commented 4 years ago

As reported by @bluestar514, the ensemble.doAction(..) API function is nothing more than a thin wrapper around actionLibrary.doAction(..), which seems to have existed in the actionLibrary module at some point but was then removed entirely. A bit of digging turns up this comment in the commented-out testDoAction() function in ActionLibraryUnitTests.js:

//TEST 1 -- An Accepted Action
//BEN: This is broken, because you commented out the 'ensemble.set' inside of ActionLibrary (due to circular dependencies.)
//You probably don't actually need this anymore, so I'm going to comment out the call to this unit test for now.
//However, when we have our new "doAction" that we need to define, maybe we'll revisit this for inspiration?
actionLibrary.doAction("reminisce", "simon", "monica", v);

...which seems to indicate that they removed the actionLibrary.doAction(..) function because it was calling ensemble.set(..), causing a circular dependency issue. I suspect that we can ultimately blame Require.js for this one.

What surprised me initially was that the doAction(..) console command in the Ensemble authoring tool seems to work just fine. But then I looked at the code that handles this command and realized that it never actually calls ensemble.doAction(..) at all! They manually reimplemented the intended behavior of doAction(..) in the authoring tool without ever adding the function back into the actionLibrary API.

My guess is that we can fix this pretty straightforwardly by transplanting parts of the code from consoleViewer.js that handles the doAction(..) console command into actionLibrary.doAction(..). I'll give that a try.

mkremins commented 4 years ago

Per @meldckn in the email thread where this bug was initially reported:

Yes, that definitely needs to be implemented and I don't know why it wasn't, but you can achieve the same effect by running ensemble.set(..) on all the effects "predicates" of the action you want to run. So something like:

var effects = action.effects; // where action came from ensemble.getAction(..) or getActions(..)
for (var i = 0; i < effects.length; i += 1){
  ensemble.set(effects[i]);
}

See also sampleGame (Lovers and Rivals) in the repo for example usage.

Maybe we can actually simplify the doAction(..) interface if we assume that the actions are always going to be taken directly from ensemble.getAction[s](..)? It looks like they used to be operating under the assumption that doAction(..) would take character names and some other junk as parameters, which seems unnecessary if the boundAction datastructures returned by getAction(..) contain all that stuff already.

mkremins commented 4 years ago

Current status of this discussion:

We think it's fine to go ahead and naïvely reimplement actionLibrary.doAction(..) as a function that just takes in a single boundAction datastructure, iterates over the effects attached to this boundAction, and calls ensemble.set(..) on each one. This kinda soft-limits it to actions returned by ensemble.getAction[s](..), which will only return actions that the characters already want to do, but that's okay with us. It's probably also possible to manually construct a boundAction object that defies character volitions, at which point you can just pass in any made-up boundAction you want and it'll still work just fine.

In the future, it'd be neat to maybe implement an additional ensemble.doActionByName(..) or ensemble.forceAction(..) function that can be used to force an action to happen with user-specified bindings, regardless of whether the characters involved would ordinarily want to do this action. This would probably take an action name and a set of role/value bindings. It would probably also override condition checking, so that the action will still run with the specified bindings even if the conditions for this action with the specified bindings would be false. Maybe it'd be even better to expose some sort of ensemble.makeBoundAction(..) function that returns a custom "synthetic"/"forced" boundAction object, which can then be passed into doAction(..) like any other?