fix: FlexiblePathfinding migration follow-ups (#92)

- style: format `.behavior` files
- chore: add TODO ideas and questions in several places
- chore: add some debug logging
- feature/fix: small adjustments to Behaviors logic

Follow-up PR to #89.

Related PRs (extracted from this): #94, #96, #97, #99.

Contributes to MovingBlocks/Terasology#4981.

Depends on Terasology/FlexiblePathfinding#26 and Terasology/FlexiblePathfinding#27.

assets/behaviors/common/doRandomMove.behavior

@@ -1,9 +1,7 @@
 {
   sequence : [
-  {
-    set_target_nearby_block : { moveProbability: 65 }
-  },
-   move_to
+    { set_target_nearby_block : { moveProbability: 65 } },
+    { lookup: { tree: "Behaviors:naiveMoveTo" } }
   ]
 }
 

assets/behaviors/common/stray.behavior

@@ -1,31 +1,28 @@
 {
-
-      sequence : [
-        {
-          set_speed : { speedMultiplier: 0.3 }
-        },
-        {
-          animation : {
-            play: "engine:Walk.animationPool",
-            loop: "engine:Walk.animationPool"
-          }
-        },
-        {
-          lookup: { tree: "Behaviors:doRandomMove" }
-        },
-        {
-          animation : {
-            play: "engine:Stand.animationPool",
-            loop: "engine:Stand.animationPool"
-          }
-        },
-        {
-          set_speed : { speedMultiplier: 0 }
-        },
-        {
-          sleep : {
-            time : 3
-          }
-        }
-      ]
+  sequence : [
+    {
+      set_speed : { speedMultiplier: 0.3 }
+    },
+    {
+      animation : {
+        play: "engine:Walk.animationPool",
+        loop: "engine:Walk.animationPool"
+      }
+    },
+    {
+      lookup: { tree: "Behaviors:doRandomMove" }
+    },
+    {
+      animation : {
+        play: "engine:Stand.animationPool",
+        loop: "engine:Stand.animationPool"
+      }
+    },
+    {
+      set_speed : { speedMultiplier: 0 }
+    },
+    {
+      sleep : { time : 3 }
+    }
+  ]
 }

assets/behaviors/creatures/critter.behavior

@@ -1,25 +1,16 @@
 {
   dynamic: [
-      {
-        guard: {
-          componentPresent: "Behaviors:Fleeing",
-          child: {
-            sequence: [
+    {
+      guard: {
+        componentPresent: "Behaviors:Fleeing",
+        child: {
+          sequence: [
             check_flee_stop,
-            {
-              lookup: {
-                tree: "Behaviors:flee"
-              }
-            }
-            ]
-          }
-        }
-      },
-      {
-        lookup: {
-          tree: "Behaviors:stray"
+            { lookup: { tree: "Behaviors:flee" } }
+          ]
         }
       }
-
+    },
+    { lookup: { tree: "Behaviors:stray" } }
   ]
-}
+}

assets/behaviors/creatures/hostileCritter.behavior

@@ -5,11 +5,10 @@
           componentPresent: "Behaviors:FindNearbyPlayers",
           values: ["N charactersWithinRange nonEmpty"],
           child: {
-          sequence: [
-            { sleep: {time: 0.1f }},
-            followCharacter,
-            { lookup: {tree: "Behaviors:hostile" }}
-
+            sequence: [
+              { sleep: {time: 0.1f }},
+              followCharacter,
+              { lookup: {tree: "Behaviors:hostile" }}
             ]
           }
         }

assets/behaviors/naiveMoveTo.behavior

@@ -1,10 +1,6 @@
 {
   sequence : [
-    {
-      selector : [
-        find_path
-      ]
-    },
+    find_path,
     { move_along_path: {
       child : {
         move_to: {}

assets/prefabs/test/testcharacter.prefab → assets/prefabs/testCharacter.prefab

@@ -3,7 +3,7 @@
     "tree" : "naiveMoveTo"
   },
   "persisted" : true,
-  "location" : {},
+  "Location" : {},
   "Network" :{},
   "Trigger" : {
     "detectGroups" : ["engine:debris", "engine:sensor"]

src/main/java/org/terasology/module/behaviors/actions/CheckAttackStopAction.java

@@ -3,17 +3,39 @@
 package org.terasology.module.behaviors.actions;
 
 import org.joml.Vector3f;
-import org.terasology.module.behaviors.components.AttackInProximityComponent;
-import org.terasology.module.behaviors.components.AttackOnHitComponent;
-import org.terasology.module.behaviors.components.FollowComponent;
 import org.terasology.engine.logic.behavior.BehaviorAction;
 import org.terasology.engine.logic.behavior.core.Actor;
 import org.terasology.engine.logic.behavior.core.BaseAction;
 import org.terasology.engine.logic.behavior.core.BehaviorState;
 import org.terasology.engine.logic.location.LocationComponent;
+import org.terasology.module.behaviors.components.AttackInProximityComponent;
+import org.terasology.module.behaviors.components.AttackOnHitComponent;
+import org.terasology.module.behaviors.components.FollowComponent;
 import org.terasology.nui.properties.Range;
 
-
+//TODO: Should this rather be a dedicated behavior instead?
+//      This is a complex action replacing a behavior tree - is that a good idea or not?
+//
+// StopAttackIfOutOfFollowDistance.behavior
+// selector [ // condition && clear
+//    sequence: [ // AND
+//        guard:
+//            componentPresent: "LocationComponent"
+//        guard:
+//            componentPresent: "FollowComponent"
+//            values: [ "N entityToFollow exists" ]
+//        selector: [ // OR
+//            guard:
+//                componentPresent: "AttackOnHitComponent"
+//            guard:
+//                componentPresent: "AttackInProximityComponent"
+//        ]
+//        check_attack_target_in_reach
+//    ]
+//
+//    // if failed
+//    set_attack_target_clear
+// ]
 @BehaviorAction(name = "check_attack_stop")
 public class CheckAttackStopAction extends BaseAction {
 
@@ -26,26 +48,30 @@ public class CheckAttackStopAction extends BaseAction {
      */
     @Override
     public BehaviorState modify(Actor actor, BehaviorState state) {
-        BehaviorState status = getBehaviorStateWithoutReturn(actor);
-        if (status == BehaviorState.FAILURE) {
-            if (actor.hasComponent(AttackOnHitComponent.class)) {
-                AttackOnHitComponent attackOnHitComponent = actor.getComponent(AttackOnHitComponent.class);
-                attackOnHitComponent.instigator = null;
-                actor.getEntity().saveComponent(attackOnHitComponent);
-            } else if (actor.hasComponent(AttackInProximityComponent.class)) {
-                AttackInProximityComponent attackInProximityComponent = actor.getComponent(AttackInProximityComponent.class);
-                attackInProximityComponent.instigator = null;
-                actor.getEntity().saveComponent(attackInProximityComponent);
-            }
-            actor.getEntity().removeComponent(FollowComponent.class);
+        if (isTargetEntityOutOfFollowDistance(actor)) {
+            clearAttackTarget(actor);
+            return BehaviorState.FAILURE;
         }
-        return status;
+        return BehaviorState.SUCCESS;
     }
 
-    private BehaviorState getBehaviorStateWithoutReturn(Actor actor) {
+    private void clearAttackTarget(Actor actor) {
+        if (actor.hasComponent(AttackOnHitComponent.class)) {
+            AttackOnHitComponent attackOnHitComponent = actor.getComponent(AttackOnHitComponent.class);
+            attackOnHitComponent.instigator = null;
+            actor.getEntity().saveComponent(attackOnHitComponent);
+        } else if (actor.hasComponent(AttackInProximityComponent.class)) {
+            AttackInProximityComponent attackInProximityComponent = actor.getComponent(AttackInProximityComponent.class);
+            attackInProximityComponent.instigator = null;
+            actor.getEntity().saveComponent(attackInProximityComponent);
+        }
+        actor.getEntity().removeComponent(FollowComponent.class);
+    }
+
+    private boolean isTargetEntityOutOfFollowDistance(Actor actor) {
         LocationComponent actorLocationComponent = actor.getComponent(LocationComponent.class);
         if (actorLocationComponent == null) {
-            return BehaviorState.FAILURE;
+            return true;
         }
         Vector3f actorPosition = actorLocationComponent.getWorldPosition(new Vector3f());
         float distance = this.maxDistance;
@@ -58,17 +84,17 @@ private BehaviorState getBehaviorStateWithoutReturn(Actor actor) {
         float maxDistanceSquared = distance * distance;
         FollowComponent followWish = actor.getComponent(FollowComponent.class);
         if (followWish == null || followWish.entityToFollow == null) {
-            return BehaviorState.FAILURE;
+            return true;
         }
 
         LocationComponent locationComponent = followWish.entityToFollow.getComponent(LocationComponent.class);
         if (locationComponent == null) {
-            return BehaviorState.FAILURE;
+            return true;
         }
         if (locationComponent.getWorldPosition(new Vector3f()).distanceSquared(actorPosition) <= maxDistanceSquared) {
-            return BehaviorState.SUCCESS;
+            return false;
         }
-        return BehaviorState.FAILURE;
+        return true;
     }
 
 }

src/main/java/org/terasology/module/behaviors/actions/FindDummyPathTo.java

@@ -23,7 +23,8 @@ public class FindDummyPathTo extends BaseAction {
     @Override
     public BehaviorState modify(Actor actor, BehaviorState result) {
         MinionMoveComponent movement = actor.getComponent(MinionMoveComponent.class);
-        Vector3i goal = actor.getComponent(MinionMoveComponent.class).getPathGoal();
+
+        Vector3i goal = movement.getPathGoal();
 
         if (goal == null) {
             return BehaviorState.FAILURE;

src/main/java/org/terasology/module/behaviors/actions/FindPathToNode.java

@@ -4,6 +4,8 @@
 
 import org.joml.Vector3f;
 import org.joml.Vector3ic;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.terasology.engine.logic.behavior.BehaviorAction;
 import org.terasology.engine.logic.behavior.core.Actor;
 import org.terasology.engine.logic.behavior.core.BaseAction;
@@ -28,12 +30,20 @@
 @BehaviorAction(name = "find_path")
 public class FindPathToNode extends BaseAction {
 
+    private static final Logger logger = LoggerFactory.getLogger(FindPathToNode.class);
+
     @In
     transient PathfinderSystem pathfinderSystem;
 
     @In
     transient PluginSystem pluginSystem;
 
+    // TODO: how do we want to remember state in actions?
+    // 1. action field as used here
+    // 2. actor.setValue, actor.getValue as used in SleepAction
+    // 3. write to, read from component
+    // 4. ...
+    private boolean isPathfindingRunning = false;
 
     @Override
     public void construct(Actor actor) {
@@ -44,6 +54,8 @@ public void construct(Actor actor) {
             pluginSystem = CoreRegistry.get(PluginSystem.class);
         }
 
+        logger.debug("Actor {}: construct find_path Action", actor.getEntity().getId());
+
         MinionMoveComponent minionMoveComponent = actor.getComponent(MinionMoveComponent.class);
         Vector3ic start = Blocks.toBlockPos(actor.getComponent(LocationComponent.class).getWorldPosition(new Vector3f()));
         Vector3ic goal = actor.getComponent(MinionMoveComponent.class).getPathGoal();
@@ -53,10 +65,14 @@ public void construct(Actor actor) {
         config.requester = actor.getEntity();
         config.maxTime = 10f;
         config.maxDepth = 150;
-        config.goalDistance = minionMoveComponent.pathGoalDistance;
+        config.goalDistance = minionMoveComponent.goalTolerance;
         config.plugin = pluginSystem.getMovementPlugin(actor.getEntity()).getJpsPlugin(actor.getEntity());
 
+        isPathfindingRunning = true;
+
+        logger.debug("... [{}]: compute path between {} -> {}", actor.getEntity().getId(), start, goal);
         int id = pathfinderSystem.requestPath(config, (path, target) -> {
+            isPathfindingRunning = false;
             if (path == null || path.size() == 0) {
                 return;
             }
@@ -66,16 +82,27 @@ public void construct(Actor actor) {
             minionMoveComponent1.setPath(path);
             actor.save(minionMoveComponent1);
         });
+        if (id == -1) {
+            // task was not accepted
+            isPathfindingRunning = false;
+        }
     }
 
     @Override
     public BehaviorState modify(Actor actor, BehaviorState result) {
+        logger.debug("Actor {}: in find_path Action", actor.getEntity().getId());
         // this an action node, so it will always be called with BehaviorState.UNDEFINED
         if (result == BehaviorState.RUNNING) {
             // this can never happen o.O
             return result;
         }
+        if (isPathfindingRunning) {
+            logger.debug("... [{}]: ... still searching for path", actor.getEntity().getId());
+            return BehaviorState.RUNNING;
+        }
+
         MinionMoveComponent minionMoveComponent = actor.getComponent(MinionMoveComponent.class);
+        logger.debug("... [{}]: pathfinding done: {}", actor.getEntity().getId(), minionMoveComponent.getPath());
         return minionMoveComponent.getPath().isEmpty() ? BehaviorState.FAILURE : BehaviorState.SUCCESS;
     }
 }

src/main/java/org/terasology/module/behaviors/actions/LogAction.java

@@ -12,13 +12,18 @@
 import org.terasology.nui.properties.TextField;
 
 /**
- * Logs a message into the console when called and returns SUCCESS
+ * Logs a debug message when called. Always returns SUCCESS.
+ * <p>
+ * For example, you can add such a log action at the beginning of a sequence and observe whether/when it is printend.
+ * <pre>
+ *     { log: { message: "starting 'myBehavior' sequence"} }
+ * </pre>
+ * You may need to adjust the logging configuration to make logs on 'DEBUG' level from the {@link LogAction} class visible.
  */
 @API
 @BehaviorAction(name = "log")
 public class LogAction extends BaseAction {
-    public static final Logger logger = LoggerFactory.getLogger(LogAction.class
-    );
+    public static final Logger logger = LoggerFactory.getLogger(LogAction.class);
 
     @TextField
     public String message;
@@ -30,7 +35,7 @@ public void construct(Actor actor) {
 
     @Override
     public BehaviorState modify(Actor actor, BehaviorState result) {
-        logger.debug(String.format("Actor %s logs message: %s ", actor.getEntity().toString(), actor.getValue(getId())));
+        logger.debug("Actor {}: {}", actor.getEntity().getId(), actor.getValue(getId()));
         return BehaviorState.SUCCESS;
     }
 }