SpongePowered
New AI Task structure questions
Last time, Zidane mentioned when ST-DDT contributed AI docs on SpongeDocs that the whole AI system (agent goals/tasks) will be revamped. I have a few notes for these changes:
Goal class:
public interface Goal<O extends Agent> {
GoalType getType();
O getOwner();
TaskEntry<O> addTask(int priority, AITask<O> task);
Optional<TaskEntry<O>> removeTask(AITask<O> task);
Collection<TaskEntry<O>> removeTasks(AITaskType type);
List<TaskEntry<O>> getTasksByType(AITaskType type);
List<TaskEntry<O>> getTasks(); // Same above
void clear();
AITaskOccupiedFlag getUsedFlags();
interface TaskEntry<O extends Agent> implements Comparable<TaskEntry<O>> {
Goal<O> getGoal();
AITask<O> getTask();
int getPriority();
}
}
The AITask now holds the exact owner type instead of the superclass owner type which is the lowest bound of the task. Making it easier for plugins. Moreover, the task entry object allows retrieval of priority and confirm removal.
AITask class:
public interface AITask<O extends Agent> {
AITaskType getType();
Optional<Goal<O>> getGoal();
default Optional<O> getOwner() {
return getGoal().map(Goal::getOwner);
}
AITaskOccupiedFlag getOccupiedFlag(); // Something backed by bit operation like BlockChangeFlag
boolean canBeInterrupted();
}
Replaced the canRunConcurrentWith with the occupied flag. The old method is way too ambiguous. Some of the occupied flags are changed with entity states (e.g. riding a boat disables some flags)
AITaskBuilder class: (Or AbstractAITaskBuilder)
public interface AITaskBuilder<O extends Agent, R extends AITask<O>, S extends AITask<?>, B extends AITaskBuilder<O, R, S, B>> extends ResettableBuilder<S, B> {
R build(O owner);
}
O: owner
R: result built, aware of generic owner
S: sources to initialize the builder, wildcard owner
B: builder
Unfortunately, I couldn't find a way to reduce the generic parameters.
Sample builtin ai task class: (Take avoid entity for example)
public interface AvoidEntityAITask<O extends Creature> extends AITask<O> {
@SuppressWarnings("unchecked")
static <O extends Creature> Builder<O> builder() {
return (Builder<O>) (Object) Sponge.getRegistry().createBuilder(Builder.class);
}
Predicate<? super Entity> getTargetSelector();
AvoidEntityAITask<O> setTargetSelector(Predicate<? super Entity> predicate);
float getSearchDistance();
AvoidEntityAITask<O> setSearchDistance(float distance);
double getCloseRangeSpeed();
AvoidEntityAITask<O> setCloseRangeSpeed(double speed);
double getFarRangeSpeed();
AvoidEntityAITask<O> setFarRangeSpeed(double speed);
interface Builder<O extends Creature> extends AbstractBuilder<O, AvoidEntityAITask<O>, AvoidEntityAITask<?>, Builder<O>> {}
interface AbstractBuilder<O extends Creature, R extends AvoidEntityAITask<O>, S extends AvoidEntityAITask<?>, B extends AbstractBuilder<O, R, S, B>> extends AITaskBuilder<O, R, S, B> {
B targetSelector(Predicate<? super Entity> predicate);
B searchDistance(float distance);
B closeRangeSpeed(double speed);
B farRangeSpeed(double speed);
}
}
Note that every builtin ai class now carries a generic for the owner type; the owner type does not matter that much except its lower bound. This design guarantees mod compatibility in case mod ai tasks extend vanilla ones. As a result, every builtin ai class has an abstract builder and a concrete builder.
For AITaskOccupiedFlag, we can write it in a fashion similar to block change flags.
Known info from mc code snippet:
boolean flag = !(this.getControllingPassenger() instanceof EntityLiving);
boolean flag1 = !(this.getRidingEntity() instanceof EntityBoat);
this.tasks.setControlFlag(1, flag);
this.tasks.setControlFlag(4, flag && flag1);
this.tasks.setControlFlag(2, flag);
So flag 1, 2, 4 are not ridden flags and flag 4 is not riding boat (or can move?) flag
Examples: avoid entity: 1 leap at entity: 4 | 1 eat grass: 4 | 2 | 1 villager mate: 2 | 1 sit: 4 | 1 wither do nothing: 4 | 2 | 1 panic: 1 drowned go to water: 1 tempt: 2 | 1 swimming: 4 ghast look around: 2 watch closest: 2 watch closest without moving: 2 | 1 trade player: 4 | 1 ranged attack: 2 | 1 melee/zombie attack: 2 | 1 defend village: 1 move to block: 4 | 1
Appears flag 1 is for movement (in phantoms, all regular tasks have a flag of 1) flag 2 is for the action of looking (watch/ghast look)
For flag 4, it is a bit more complex mutex bit of 4: swimming mutex bit of 5: slime float in liquid trade with player jump leap at target move to block sit mutex bit of 6: (none) mutex bit of 7: eat grass wither do nothing
In conclusion, flag 4 is for liquid movement.
So, I propose an AI Task Occupied flag class as below:
public interface AITaskOccupiedFlag {
boolean moving();
boolean looking();
boolean inFluid();
AITaskOccupiedFlag withMoving(boolean moving);
AITaskOccupiedFlag withLooking(boolean looking);
AITaskOccupiedFlag setInFluid(boolean inFluid);
AITaskOccupiedFlag inverse(); // copy-pasta from BlockChangeFlag
AITaskOccupiedFlag andFlag(AITaskOccupiedFlag flag);
AITaskOccupiedFlag andNotFlag(AITaskOccupiedFlag flag);
}
And an AITaskOccupiedFlags, too. (You need to run sortfields)
@SuppressWarnings({"unused", "WeakerAccess"})
public final class AITaskOccupiedFlags {
// SORTFIELDS:ON
public static final AITaskOccupiedFlag ALL = DummyObjectProvider.createFor(AITaskOccupiedFlag.class, "ALL");
public static final AITaskOccupiedFlag IN_FLUID = DummyObjectProvider.createFor(AITaskOccupiedFlag.class, "IN_FLUID");
public static final AITaskOccupiedFlag LOOKING = DummyObjectProvider.createFor(AITaskOccupiedFlag.class, "LOOKING");
public static final AITaskOccupiedFlag LOOKING_IN_FLUID = DummyObjectProvider.createFor(AITaskOccupiedFlag.class, "LOOKING_IN_FLUID");
public static final AITaskOccupiedFlag MOVING = DummyObjectProvider.createFor(AITaskOccupiedFlag.class, "MOVING");
public static final AITaskOccupiedFlag MOVING_IN_FLUID = DummyObjectProvider.createFor(AITaskOccupiedFlag.class, "MOVING_IN_FLUID");
public static final AITaskOccupiedFlag MOVING_LOOKING = DummyObjectProvider.createFor(AITaskOccupiedFlag.class, "MOVING_LOOKING");
public static final AITaskOccupiedFlag MOVING_LOOKING_IN_FLUID = DummyObjectProvider.createFor(AITaskOccupiedFlag.class, "MOVING_LOOKING_IN_FLUID");
public static final AITaskOccupiedFlag NONE = DummyObjectProvider.createFor(AITaskOccupiedFlag.class, "NONE");
// SORTFIELDS:OFF
private AITaskOccupiedFlags() {
}
}
With BlockChangedFlags as a precedent, this should not be too hard to implement.
I initially didn't foresee me writing so much. Guess I can ping @Zidane for a look then :sweat_smile:
we can expose a factory method in game registry (most likely) for hard registration with mutex
IMO the flags should be a CatalogType, that will work similar to an enumset to resolve the contents
@st-ddt i did not make it one since block change flag is not a catalog type. might ask @gabizou on that design
The difference is that it's not possible to create custom BlockChangeFlags
The number of Mutex bits/AI flags is also limited. Some might not have a name by default, but they exists nevertheless. Unless we throw the bits in the ditch and invent something new that is a superset of MCs way, but might cause issues for mods that also want to use a different superset.
We can assign the actual bits dynamically based on calls to the normal goal. when the normal goal is called, we usually have an idea which bits are actually used and can substitute their flag objects with preset or generated entries
Changing existing bits or dynamically assigning them will cause conflicts with non Sponge plugins? How is a forge mod supposed to check whether the Sponge plugin uses the CastMagicFlag if the bit is always different.
@ST-DDT forge mods do not check flags. They only set flags they need. Sponge plugins will enable flags with entity update event listeners.
So the revamped flag class is:
public interface AITaskOccupiedFlag {
}
the flag set:
public interface AITaskOccupiedFlagSet {
boolean has(AITaskOccupiedFlag flag);
AITaskOccupiedFlagSet set(AITaskOccupiedFlag flag);
AITaskOccupiedFlagSet inverse();
AITaskOccupiedFlagSet union(AITaskOccupiedFlagSet flag);
AITaskOccupiedFlagSet distinct(AITaskOccupiedFlagSet flag);
AITaskOccupiedFlagSet intersect(AITaskOccupiedFlagSet flag);
}
goals and tasks use the flag set instead.
Mutex bits set by mods may be different from vanilla ones if they create completely custom ai. In that case it won't possible to assume the flag. It will always be a issue to recognise these bits in combination with mods, unless something gets changed in forge.
we can recognize those bits with hooks in the method where goals set their available bits and where ai tasks set their mutex bits.
IMO we should this part of the API as simple as possible (and close to vanilla) for the following reasons:
- If mods want to add more bits (beyond from whats already possible), they have to resort to a custom implementation anyway, so there is no point in registering additional values ourselves in the first place (the number of bits is limited, if mods use them for different purposes there is nothing we can do to detect that)
- Using a non primitive value has a huge performance impact:
O(1)vsO(n * (n/2))- I could understand an API wrapper for usability reasons, but then it should be a (set of) catalog types (with just getters and setters)
- I don't see a usecase for inverse, union, distinct and intersect (maybe a boolean variant for canRunConcurrentWith?)
- maybe it would be best to have a
VanillaAITaskinterface that can be used for these behavior/flag basedcanRunConcurrentlyWithchecks or just some fake methods in AbstractAITask from the API, that will be implemented at runtime.
TLDR: Have a getter/setter with a Set<VanillaAITask...Flag extends CatalogType> and a method to call to check.
(If my explanation is not clear, I can write a diff with my proposed changes regarding the flags)
@ST-DDT
- Mods do not need custom implementations. They need to do zero thing. We use transformer to add extra hook in the methods where the bits are set!
- The primitive value thing is not that important. The flags can be sorted to make comparison much faster. It is not freaking slow.
- All those methods are bitwise operations.
unionfor bitwise or,distinctfor "and not",inversefor inversing bits, andintersectfor and operation. They are definitely needed. Don't try to outdumb me. - Your vanilla ai task interface approach is definitely wicked and will only introduce more problems.
@ST-DDT I believe you have trouble understanding some of my ideas:
We can detect with injections in EntityAIBase#setMutexBits,EntityAITasks#enableControlFlag, and EntityAITasks#disableControlFlag
We will also fetch the agent type in context to determine for which types of agents these flags exist.
By the way, the new system I propose may supersede #1014
I opened that issue to track (one of) the issues of the current AI API. I will happily close that issue once we have a working variant inside the API.
We can detect with injections in EntityAIBase#setMutexBits,EntityAITasks#enableControlFlag, and EntityAITasks#disableControlFlag We will also fetch the agent type in context to determine for which types of agents these flags exist.
Wouldn't this LAZY initialization have the drawback that it might never be fully populated if certain entity types aren't spawned. What would be the difference between BIT_4 for Zombies and BIT_4 for Skeletons? Why can't I use BIT_4 for Slimes, if vanilla MC does not use it?
The primitive value thing is not that important. The flags can be sorted to make comparison much faster. It is not freaking slow.
Performance isn't my only concern there. Well AFAICT that int is the one value every custom AI mod out there is using (if they use the vanilla classes at all).
All those methods are bitwise operations. union for bitwise or, distinct for "and not", inverse for inversing bits, and intersect for and operation. They are definitely needed. Don't try to outdumb me.
Sorry, that wasn't my intention. This class can easily replaced by a Set (addAll, removeAll, retainAll), maybe the impl is backed by an int, but that doesn't mean we should avoid the java standard API for well known concepts. As for Inverse I'm not 100% sure about its definition: Is it !MOVE = NOT_MOVE or !MOVE = LOOK + SWIM + ... ( I assume you refer to the second variant). But still what would be the usecase?
IMO Our concepts might be pretty similar, that why I try to demonstrate my thoughts using pseudo code (limited to the behavior flags).
Changes to the API:
AITask:
Set<AIFlag> getAIFlags(); // optional
void setAIFlags(Set<AIFlag> flags); // even more optional
AIFlag implements CatalogType (no additional methods)
AIFlags:
public final AIFlags {
public static final AIFlag MOVE = ...;
public static final AIFlag LOOK= ...;
...
public static final AIFlag BIT_31= ...;
}
AbstractAITask:
protected final Set<AIFlag> getAIFlags() {
Set<AIFlag> flags = new ...;
return flags ;
}
protected final void setAIFlags(Set<AIFlag>flags) {
}
protected final boolean flagBasedCanRunConcurrentWith(AITask task) {
return false;
}
API changes would be 100% backward compatible.
Changes to SpongeCommon:
MixinAbstractAITask:
@InsteadOf(Set::new) // Optional
// This might use an int internally and might have extra setters/getters to set/get the mutex bits directly
private Set<AiTask> newSet() {
return new MutexSet();
}
@After(Set::new)
protected void getAIFlags(Set<AIFlag> flags) {
for (int i in 0...31) {
if (getMutexBits() & 1 << i != 0) {
flags.add(VANILLA_FLAGS[i]);
}
}
}
@Head
protected void setAIFlags(Set<AIFlag>flags) {
int mutex = 0;
for (AIFlag flag in flags) {
if (flag instanceof SpongeAITask) { // VanillaAITask
mutex |= flag.getMutexBit();
}
}
setMutexBit(mutex);
}
@Replace
protected boolean flagBasedCanRunConcurrentWith(AITask task) {
return getMutexBits() & (EntityAIBase) task.getMutexBits() == 0;
}
Usage in SpongePlugins
MyAITask:
public class MyAITask extends AbstractAITask {
// BIT_15 = Cast Spell in this plugin and MCorcery
public MyAITask () {
setAIFlags(Set.of(AIFlags.MOVE, AIFlags.LOOK, AIFlags.BIT_15);
addFlagIfExists(registry.get(AiTask.class, "spellforce:magic_invoke"));
}
public boolean canRunConcurrentlyWith(AITask task) {
return flagBasedCanRunConcurrentWith(task) && ! criticalSection;
}
}
Extension by Third-Party mods: (if some mod ever wants to do this)
MixinEntityAIBase:
public MixinEntityAIBase implements MyAIInterface {
private long myMutexBits;
private boolean noConcurrentFlagSet;
// getter + setter (as defined by the interface)
}
MyAIFlags:
public final MyAIFlags {
public static final AIFlag NO_CONCURRENT = new MyAIFlag(...);
public static final AIFlag MY_MUTEX_0 = new MyMutexAIFlag(...); // Might be spellforce:magic_invoke
...
public static final AIFlag[] FLAGS_ARRAY = { MY_MUTEX_0, ..., MY_MUTEX_63};
}
MixinAbstractAITask:
@After(Set::new)
protected final void getAIFlags(Set<AIFlag> flags) {
if (isNoConcurrentFlagSet()) {
flags.add(MyAIFlags.NO_CONCURRENT);
}
for (int i in 0...63) {
if (getMyMutexBits() & 1 << i != 0) {
flags.add(MyAIFlags.FLAGS_ARRAY[i]);
}
}
}
@Head
protected void setAIFlags(Set<AIFlag>flags) {
setNoConcurrentFlag(false);
setMyMutexBits(0);
for (AIFlag flag in flags) {
if (flag instanceOf MyAIFlag) {
(MyAIFlag) flag.apply(this);
}
}
}
@Return
protected boolean flagBasedCanRunConcurrentWith(AITask task, boolean returnValue) {
return returnValue && (MyAIInterface) this.getMyMutexBits() & (MyAIInterface) task.getMyMutexBits() == 0 && !(MyAIInterface ) this.isNoConcurrentFlagSet();
}
We can use another tracking system for sponge-added flags besides the existing mutex. Inverse should be all other flags set to true.
For your performance concerns, we can require all plugin-based ai flags to be added before a loading stage to allow putting flags in something backed by a bitset, etc.
If you fear sponge cannot spawn mobs, in fact, sponge can (by iterating over the entity registry and spawning in a dummy world)
I'd keep my ai flag set class so that people are not silly enough to feed an array-based set or something similar to the method.
We can use another tracking system for sponge-added flags besides the existing mutex.
You are not referring to a different API here, are you?
Inverse should be all other flags set to true.
What would be the usecase? When would you actually do this? This does not make any sense on an AI functional level (although it might for boolean logic).
plugin-based ai flags to be added in something backed by a bitset, etc.
It should be possible to refer to an exact bit, without knowing the using/depending on a plugin (to allow plugins to be compatible to non-Sponge plugins). Like in my example BIT_15.
If you fear sponge cannot spawn mobs, in fact, sponge can (by iterating over the entity registry and spawning in a dummy world)
This does not guarantee that plugin/mod/vanilla provided AI tasks get created/run at all. Some goals are (might be) created conditionally (killerRabbit) or externally.
I'd keep my ai flag set class so that people are not silly enough to feed an array-based set or something similar to the method.
How would you implement that in the API without exposing implementation details/internals? Well it might be possible, somehow. Thats probably something to verify during the actual PR.
@liach Are we going to address all AI API issues at once, or one at a time (to keep possibly breaking changes separate)?
PS: A few days ago I got contacted for implementation help related to AI, so the current API is definitely in use.
I am referring to a totally different, new ai system for bleeding.
the whole AI system (agent goals/tasks) will be revamped.
I am aiming to get the flags in for the new version.
To refer to exact bit, we can convert flags to catalog types and look up with arbitrary id.
The inversion is for goals. Goals need to reverse the bits.
My AITaskOccupiedFlagSet proposed already covered your last question "How would you implement that in the API without exposing implementation details/internals?"
If we could get an initial draft, then we could at least have a base for discussion. I could play with it and check whether this is easily usable by plugins and maybe also start an accompanying docs PR.