fix: FlexiblePathfinding migration follow-ups

[skaldarnar]

• 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 Terasology/Behaviors#89.

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

Contributes to MovingBlocks/Terasology#4981.

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

[jdrueckert]

I just noticed that the now pathfinding-based doRandomMove works pretty okay (including moving uphill). However, it (1) does time out rather frequently when actually moving along the path or (2) the randomly chosen target block just "doesn't make sense".

For (1) we need to investigate why it times out after having found what appears to be a valid path (and judging from the debug path visualization actually is a reasonable path). For (2) I think what we need here would be some more movement-type-based restrictions on what a "reasonable random target block" is, which should be reflected in the plugin's isReachable method. i.e. for an entity that is walking or leaping, any target position that does not have a solid block underneath doesn't make sense (even if gravity does not apply to mobs if they don't move - btw I still feel like it does when they jump, but I don't know why). on the other hand for an entity that is flying or swimming, that's totally fine. for swimming however any block that is not liquid is not a reasonable target.

[jdrueckert]

FlexibleMovement as a replacement for PathFinding

[jdrueckert]

I implemented the optimization proposed in the todo comment for findRandomNearbyBlock() in SetTargetToNearbyBlockNode.java by checking for the candidate increasing the distance to the start block in addition to it being reachable. I put it before the reachability check because I think it happens more often that a candidate would be reachable but does not "get us forward" - I'm fine with switching the order if somebody thinks it's the other way round or roughly equally likely but checking reachability is cheaper.

Also, after observing our deer a bit more, I think that something about the moveTo completion check is off. I noticed that https://github.com/Terasology/Behaviors/blob/fix/behavior-follow-ups/src/main/java/org/terasology/module/behaviors/actions/MoveToAction.java#L72 does not result in true for instance for position (-73.63, 29.00, 40.99) and target (-73, 29, 41). Considering our NPCs do have a collision box and do not move as ghosts, I would deem this as close enough to tell moveTo that it's done. Currently we don't and try to get even close to our target until we time out eventually.

Creating a Vector3i from the Vector3f position with RoundingMode.CEILING (which I think we deemed to be the default to use? correct me if wrong) would match the target:

new Vector3i(new Vector3f(-73.63f, 29.00f, 40.99f), RoundingMode.CEILING) --> (-73, 29, 41)

Block.toBlockPos uses TeraMath.floorToInt after adding 0.5f to every dimension of the vector, which does not match the target:

Blocks.toBlockPos(new Vector3f(-73.63f, 29, 40.99f)) --> (-74, 29, 41)

Do we think the Blocks utility class computation is correct? If so, should we introduce some kind of "tolerance" to the completion check in moveToAction.java?

[skaldarnar]

While testing these changes we were wondering about the exact placement of Bullet physics colliders. Since the entity just moves into the (intermediate) target block and then continues to the next target, a collision shape extending to the sides or below may very well prevent clean movement due to block collision.

We were wondering how exactly the character colliders work, where they have their origin point, and how exactly their extents look in the world. Would be nice to have some debug rendering for this (in addition to the bounding boxes as defined by shape components).

We used the following change in the BulletPhysics class:

diff --git a/engine/src/main/java/org/terasology/engine/physics/bullet/BulletPhysics.java b/engine/src/main/java/org/terasology/engine/physics/bullet/BulletPhysics.java
index 61e802e76..513a810e6 100644
--- a/engine/src/main/java/org/terasology/engine/physics/bullet/BulletPhysics.java
+++ b/engine/src/main/java/org/terasology/engine/physics/bullet/BulletPhysics.java
@@ -566,6 +566,8 @@ private CharacterCollider createCharacterCollider(EntityRef owner) {
                     "CharacterMovementComponent.");
         }
         Vector3f pos = locComp.getWorldPosition(new Vector3f());
+        // adjust the position to be at the bottom center
+        pos.sub(0, movementComp.height / 2f, 0);
         final float worldScale = locComp.getWorldScale();
         final float height = (movementComp.height - 2 * movementComp.radius) * worldScale;
         final float width = movementComp.radius * worldScale;

[jdrueckert]

• test case "up and down again" consistently fails
• the swimming-related "three dimensional movement" test case fails sporadically

[skaldarnar]

Locally, there is a single failing test case for me now:

13:23:23.542 [Test worker] INFO  o.t.module.behaviors.MovementTests - movement plugin combination: [walking, leaping, falling]

Test character is not at target position.
  start   : ( 0.000E+0  4.100E+1  0.000E+0)
  target  : ( 5.000E+0  4.100E+1  0.000E+0)
  current : ( 0.000E+0  4.100E+1  0.000E+0)

  X    X|XX XX|XXXXX
 ==> expected: <( 5.000E+0  4.100E+1  0.000E+0)> but was: <( 0.000E+0  4.100E+1  0.000E+0)>
Expected :( 5.000E+0  4.100E+1  0.000E+0)
Actual   :( 0.000E+0  4.100E+1  0.000E+0)

All the other (positive) test cases pass nicely, usually in under 3 seconds per test case, summing up to a bit less than 2 minutes of execution time. The result is the same on CI, about 50% slower than running test locally with an execution time of 2min 40s.