Roadmap for replace Pathfinding with FlexiblePathfinding

[DarkWeird]

Motivation

• High memory usage of Pathfinding(especially in java)
• Unnecessary PF's NavGraph usage (e.g. CoreGameplay don't needs it, but NavGraph is creates)
• We have alternative already (FPF)

Concerns

• Is there specific expertise that will be needed for this effort?
• Does this effort have dependencies on other efforts?
• Do you expect this effort to conflict with any other efforts?
• What are potential drawbacks of the effort?
• What are maintenance or continuous efforts that will persist beyond the completion of this effort?

Task Breakdown

• Extract pathfinding behaviours from behaviours (this was done in https://github.com/Terasology/FlexibleMovement and https://github.com/Terasology/PathfindingBehaviors )
• Rewrite dependency from behaviours to PF's behaviours for depending modules (most specified dependencies were unused)
• Write PF's behavours alternatives for FlexableMovement.
• Replaces PF behavours with FPF behaviours in depending modules (we actually did a near in-place migration in chore: Use FlexiblePathfinding / Remove Pathfinding Terasology/Behaviors#88 and feat!: Replace Pathfinding with FlexiblePathfinding (part 2) Terasology/Behaviors#89. therefore, we did not need to change much in modules. all relevant PRs are linked in those two PRs)
• Replace PF with FPF in omega lineup (Terasology/Index@9e0b097 and Terasology/Index@3ceb298)
• Follow-ups from feat!: Replace Pathfinding with FlexiblePathfinding (part 2) Terasology/Behaviors#89
  • check if use of @In possible

    @In
    private WorldProvider world;
    

    • it's not possible in behavior actions
  • use initialization in construct() consistently throughout behavior actions - chore: apply workaround for @In malfunction in actions Terasology/Behaviors#102

    @Override
    public void construct(Actor actor) {
        if (world == null) {
            world = CoreRegistry.get(WorldProvider.class);
        }
    }
    

    • this will work if @in has no effect, i.e., the field is initialized to null
    • this will work if @in is fixed and initializes the field correctly, making the code in construct a no-op
    • we preserve the information that we want to use @in in actions
  • formatting of assets (JSON)
    • behaviors cannot be properly JSON-formatted without changing the behavior parser
  • formatting of Java files
  • removing dead and commented code (or revive if necessary) - chore: clean up some code Terasology/Behaviors#103
    • no dead / commented code in assets
    • FlexibleMovementDebugLayout almost entirely commented out
    • ValidatePath action has some out-commented code
  • assess and iterate naming of actions and behaviors
    • this is a potential rabbit-hole and more or less endless task... should be tracked separately and specified in more detail
  • slim down test assets (testcharacter.prefab itself, duplicates in children of parent character prefabs) chore: clean up test prefabs Terasology/Behaviors#90
  • revert replacement of behavior node prefabs for findPathTo, moveAlongPath and moveTo fix: revert replacement of behavior node prefabs for findPathTo, moveAlongPath and moveTo Terasology/Behaviors#91

Optional tasks and improvements

• Move code and features from Gooyes Defence to FlexableMoving.
• Provide documentation for FPF.

[skaldarnar]

We went for a more ... straight ahead path now by integrating what was already done for https://github.com/Terasology/FlexibleMovement back into https://github.com/Terasology/Behaviors.

• https://github.com/Terasology/Behaviors#88
• https://github.com/Terasology/Behaviors#89

GooeyDefence already uses FlexiblePathfinding directly, so I guess that step is not required, but might be a good test for the new behaviors.

[skaldarnar]

Update: Current state of the migration (with a couple of local changes I'll need to push later when back home) I can start the game again, although there are still failing tests and some worrying log messages and non-optimal behavior of mobs, e.g., with WildAnimals.

• 4 failing Behaviors tests

   MovementTests. [walking, leaping]: leap
   MovementTests. [swimming]: leap
   MovementTests. [swimming]: diagonal
   MovementTests. [swimming]: straight
  

• injection @In not working for behavior actions

   18:40:23.697 [main] WARN  o.t.engine.registry.InjectionHelper - SetTargetToNearbyBlockNode wanted PluginSystem injected but CoreRegistry has none.
   18:40:23.697 [main] WARN  o.t.engine.registry.InjectionHelper - MoveToAction wanted WorldProvider injected but CoreRegistry has none.
   ...
   18:40:23.769 [main] WARN  o.t.engine.registry.InjectionHelper - FindPathToNode wanted PathfinderSystem injected but CoreRegistry has none.
  

• broken prefabs?

   18:40:23.698 [main] WARN  o.t.e.p.serializers.PrefabSerializer - Prefab 'WildAnimals:babyDeer' contains unknown component 'StrayIfIdle'
   18:40:23.505 [main] WARN  o.t.e.p.serializers.PrefabSerializer - Prefab 'Behaviors:jobBuildBlock' contains unknown component 'Work
  

• broken behavior trees or behavior actions? (IllegalArgumentException is silently swallowed... o.O)

   18:52:40.472 [main] WARN  o.t.e.physics.bullet.BulletPhysics - Can't update Trigger entity with a non-finite position/rotation?! Entity: EntityRef{id = 3169, netId = 0, prefab = 'WildAnimals:sheep'}
  
   18:11:07.457 [Test worker] DEBUG org.terasology.engine.core.ComponentSystemManager - Initialising org.terasology.engine.logic.behavior.BehaviorSystem@2ffebb03
   18:11:07.460 [Test worker] ERROR org.terasology.engine.core.ComponentSystemManager - Failed to initialise system org.terasology.engine.logic.behavior.BehaviorSystem@2ffebb03
   java.lang.IllegalArgumentException: Unknown behavior node type setup_continuous_pathfinding
  
   18:11:07.466 [Test worker] DEBUG org.terasology.engine.core.ComponentSystemManager - Initialising org.terasology.engine.logic.behavior.CollectiveBehaviorSystem@c892b6
   18:11:07.466 [Test worker] ERROR org.terasology.engine.core.ComponentSystemManager - Failed to initialise system org.terasology.engine.logic.behavior.CollectiveBehaviorSystem@c892b6
   java.lang.IllegalArgumentException: Unknown behavior node type setup_continuous_pathfinding
  

[jdrueckert]

* broken prefabs?
  ```
   18:40:23.698 [main] WARN  o.t.e.p.serializers.PrefabSerializer - Prefab 'WildAnimals:babyDeer' contains unknown component 'StrayIfIdle'
   18:40:23.505 [main] WARN  o.t.e.p.serializers.PrefabSerializer - Prefab 'Behaviors:jobBuildBlock' contains unknown component 'Work
  ```

@skaldarnar Terasology/WildAnimals#93 addresses the first of the two.

The second originates from the WorkComponent having been removed without replacement in Terasology/Behaviors#88. I don't see any usages of jobBuildBlock.prefab or jobWalkToBlock.prefab, which are the only references to this component in omega. I would propose to remove the prefabs.

[jdrueckert]

* broken behavior trees or behavior actions? (IllegalArgumentException is silently swallowed... o.O)
  ```
   18:52:40.472 [main] WARN  o.t.e.physics.bullet.BulletPhysics - Can't update Trigger entity with a non-finite position/rotation?! Entity: EntityRef{id = 3169, netId = 0, prefab = 'WildAnimals:sheep'}
  
   18:11:07.457 [Test worker] DEBUG org.terasology.engine.core.ComponentSystemManager - Initialising org.terasology.engine.logic.behavior.BehaviorSystem@2ffebb03
   18:11:07.460 [Test worker] ERROR org.terasology.engine.core.ComponentSystemManager - Failed to initialise system org.terasology.engine.logic.behavior.BehaviorSystem@2ffebb03
   java.lang.IllegalArgumentException: Unknown behavior node type setup_continuous_pathfinding
  
   18:11:07.466 [Test worker] DEBUG org.terasology.engine.core.ComponentSystemManager - Initialising org.terasology.engine.logic.behavior.CollectiveBehaviorSystem@c892b6
   18:11:07.466 [Test worker] ERROR org.terasology.engine.core.ComponentSystemManager - Failed to initialise system org.terasology.engine.logic.behavior.CollectiveBehaviorSystem@c892b6
   java.lang.IllegalArgumentException: Unknown behavior node type setup_continuous_pathfinding
  ```

This is due to setup_continuous_pathfinding being removed in Terasology/Behaviors#89 (https://github.com/Terasology/Behaviors/blob/d3600d101140c2f012aeb9ebc1d2c210f67cff02/src/main/java/org/terasology/module/behaviors/actions/SetupContinuousMoveNode.java).

[skaldarnar]

I think with merging Terasology/Behaviors#89 and related PRs we're done with the main list of tasks as described in this issue. I'll copy over the respective follow-ups from that issue description such that we can keep track of it here (will update the initial issue).

[jdrueckert]

Regarding the lacking support for dependency injection in behavior actions, I noticed, that we do have something in that direction in BehaviorTreeBuilder::addAction.
However, I don't know whether - and if not why - this is sufficient.

I wonder whether this might be related to the respective fields not being registered, yet, rather than the dependency injection not working as expected...?

If we are sure that the fields are registered and the dependency injection is implemented incorrectly, #5003 might be related.

[jdrueckert]

All documented follow-ups done, closing this issue.