feat!: Replace Pathfinding with FlexiblePathfinding (part 2) #89

skaldarnar · 2022-01-13T22:34:34Z

Follow-up to #88.

Integrates the state of Terasology-Archived/FlexibleMovement@e722d75 into this module.

restructure packages to match module guidelines
fixup! restructure packages to match module guidelines
feat!: integrate classes from FlexibleMovement
fixup! feat!: integrate classes from FlexibleMovement
chore!: remove left-over actions using Pathfinding
feat!: add assets from FlexibleMovement

Follow-ups

make use of @In (if possible) or initialization in construct() consistent
formatting of assets (JSON)
formatting of Java files
removing dead and commented code (or revive if necessary)
assess and iterate naming of actions and behaviors
slim down test assets (testcharacter.prefab itself, duplicates in children of parent character prefabs)
align behavior node prefabs for condition, jump, log, setTargetLocalPlayer, setTargetToNearbyBlock with the ones for findPathTo, moveAlongPath and moveTo

No assets have been moved over/back yet, this needs to be done in a second step.

jdrueckert · 2022-01-18T19:32:28Z

@DarkWeird I removed the following behavior node prefabs:

findPathTo.prefab
moveAlongPath.prefab
moveTo.prefab

For each of them you seem to have created a replacement in FlexibleMovement that's copied over here in this PR:

findpathtonode.prefab
movealongpathnode.prefab
movetonode.prefab

Can you tell me what these prefabs are used for?

Also, I noticed the following changes when comparing the originals and their replacements:

removal of action field - why is this not needed anymore?
removal of displayName field - why is this not needed anymore?
replacement of name field value with displayName field value - is this the new "displayName" field?
introduction of type field - where does this come from?

DarkWeird · 2022-01-18T20:20:46Z

@DarkWeird I removed the following behavior node prefabs:

findPathTo.prefab

moveAlongPath.prefab

moveTo.prefab

For each of them you seem to have created a replacement in FlexibleMovement that's copied over here in this PR:

findpathtonode.prefab

movealongpathnode.prefab

movetonode.prefab

Can you tell me what these prefabs are used for?

Also, I noticed the following changes when comparing the originals and their replacements:

removal of action field - why is this not needed anymore?

removal of displayName field - why is this not needed anymore?

replacement of name field value with displayName field value - is this the new "displayName" field?

introduction of type field - where does this come from?

Seems this prefabs using for BehaviorsEditor only.
I don't feel any problem... When haven't thm

skaldarnar · 2022-01-18T21:09:56Z

assets/prefabs/behaviorNodes/findpathtonode.prefab

+{
+  "BehaviorNode": {
+    "type": "FindPathToNode",
+    "name": "Find Path",
+    "category": "movement",
+    "shape": "rect",
+    "description": "Finds a path to target using FlexiblePathfinding",
+    "color": [180, 180, 180, 255],
+    "textColor": [0, 0, 0, 255]
+  }
+}


Should we just remove all of these prefabs and link them in a central issue to bring them back with support for the behavior tree editor? Having half of them might be more confusing than helpful...

Or are they actually complete (or only a few missing)?

There still is the ensureTargetPresent.prefab which can be removed as we remove the node.
Apart from that we still only have the "old ones" for:

condition

jump

log

setTargetLocalPlayer

setTargetToNearbyBlock

Would be nice if @DarkWeird could comment on my questions on the differences/changes between old originals and new replacements. Migrating the above-listed remaining ones in a similar fashion should be easy, but I'd like to understand the changes first before applying them to the rest.

skaldarnar · 2022-01-18T21:11:17Z

assets/prefabs/characters/testcharacter.prefab

+{
+  "MinionMove": {
+    "movementTypes": ["walking", "leaping"]
+  },
+  "NameTag": { "text": "testcharacter" },
+  "Behavior" : {
+    "tree" : "naiveMoveTo"
+  },
+  "persisted" : true,
+  "location" : {},
+  "Character": {},
+  "CharacterHeldItem" : { },
+  "Network" :{},
+  "Health" : {
+    "maxHealth": 1,
+    "currentHealth": 1,
+    "excessSpeedDamageMultiplier": 0,
+    "destroyEntityOnNoHealth" : true
+  },
+  "Trigger" : {
+    "detectGroups" : ["engine:debris", "engine:sensor"]
+  },
+  "CreatureNameGenerator" : {
+    "genderRatio" : 0.5,
+    "nobility" : 0.5,
+    "theme": "DWARF"
+  },
+  "AliveCharacter": {},
+  "CharacterMovement" : {
+    "groundFriction" : 16,
+    "speedMultiplier" : 0.3,
+    "distanceBetweenFootsteps" : 0.2,
+    "distanceBetweenSwimStrokes" : 2.5,
+    "height" : 0.9,
+    "radius" : 0.3,
+    "jumpSpeed" : 12
+  }
+}


This thing has so many properties - I hope we can slim it down a bit later

skaldarnar · 2022-01-18T21:12:23Z

assets/ui/debug.ui

@@ -0,0 +1,4 @@
+{
+  "type": "FlexibleMovementDebugLayout",


The code of this class is commented out. Either one the first things to revive, or to throw out in a follow up and write down an issue for instead.

skaldarnar · 2022-01-18T21:15:29Z

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

+            MinionMoveComponent flexibleMovementComponent1 = actor.getComponent(MinionMoveComponent.class);
+            flexibleMovementComponent1.running = false;
+            if (path == null || path.size() == 0) {
+                actor.save(flexibleMovementComponent1);
+                return;


This has the changes around MinionMoveComponent#running which were added in an attempt to understand what's happening and to fix potential issues. It was not there in the original code from FlexibleMovement, so maybe we should revert d136af9 (#89).

skaldarnar · 2022-01-18T21:16:05Z

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

    }

    @Override
    public BehaviorState modify(Actor actor, BehaviorState result) {
-        final MinionMoveComponent moveComponent = actor.getComponent(MinionMoveComponent.class);
-        if (moveComponent.path == null) {
+        // this an action node, so it will always be called with BehaviorState.UNDEFINED


This part is true, though - I'd like to keep this comment as help for further work on this.

skaldarnar · 2022-01-18T21:18:52Z

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

+        if (result == BehaviorState.RUNNING) {
+            // this can never happen o.O
+            return result;
+        }


this code looks like this action should sometimes return RUNNING, but I'm not sure when exactly/how to check whether the pathfinding process is currently running or not....

@DarkWeird any clue on this?

skaldarnar · 2022-01-18T21:21:31Z

src/main/java/org/terasology/module/behaviors/components/MinionMoveComponent.java

+    // current index along path above
+    private int pathIndex = 0;
+
+    public boolean running = false;


This should not stay in like it is.

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

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

src/main/java/org/terasology/module/behaviors/debug/DebugMoveToSystem.java

src/main/java/org/terasology/module/behaviors/debug/FlexibleMovementDebugLayout.java

src/main/java/org/terasology/module/behaviors/systems/MinionMoveSystem.java

…to MinionMoveComponent

Respective action has been removed in Terasology/Behaviors#89

jdrueckert · 2022-01-30T12:43:42Z

FlexibleMovement as a replacement for PathFinding

- 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.

skaldarnar requested a review from jdrueckert January 13, 2022 22:34

skaldarnar added 4 commits January 13, 2022 23:41

feat!: integrate classes from FlexibleMovement

c742ef8

No assets have been moved over/back yet, this needs to be done in a second step.

fixup! feat!: integrate classes from FlexibleMovement

f2becfd

chore!: remove left-over actions using Pathfinding

32b13a5

feat!: add assets from FlexibleMovement

c1be8ba

skaldarnar force-pushed the feat/merge-flexible-movement-2 branch from d62a6b4 to c1be8ba Compare January 13, 2022 22:42

fixup! feat!: add assets from FlexibleMovement

a0d933d

jdrueckert mentioned this pull request Jan 17, 2022

Roadmap for replace Pathfinding with FlexiblePathfinding MovingBlocks/Terasology#4981

Closed

14 tasks

chore: remove behavior node prefabs that got a replacement

196d537

skaldarnar added 5 commits January 18, 2022 21:41

chore: remove dead code and broken references

6dd089b

test: add descriptions to BehaviorTests

eec78a5

test!: limit MovementTests to a single test case (revert later!)

0dae9be

fix?: poking with a stick in the dark...

d136af9

test: fix test by waiting for chunk generation

1a6ec41

skaldarnar marked this pull request as ready for review January 18, 2022 21:06

skaldarnar commented Jan 18, 2022

View reviewed changes

jdrueckert reviewed Jan 18, 2022

View reviewed changes

skaldarnar added 2 commits January 18, 2022 22:41

chore: rename all remaining occurrences of FlexibleMovementComponent …

7d00b98

…to MinionMoveComponent

revert: partially revert d136af9

e5f39bf

skaldarnar added Status: Needs Testing Requires to be tested in-game Topic: AI Requests, Issues and Changes related to pathfinding, behaviors, etc. Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness labels Jan 18, 2022

skaldarnar requested review from casals and DarkWeird January 18, 2022 22:02

skaldarnar added 3 commits January 21, 2022 17:38

Apply suggestions from code review

01bd638

doc: fix doc reference to MinionMoveComponent

b00bfea

chore: rename (unused) flexibleMovementSystem member in ValidatePath

fba0d49

jdrueckert approved these changes Jan 21, 2022

View reviewed changes

skaldarnar merged commit 317dd65 into develop Jan 21, 2022

skaldarnar deleted the feat/merge-flexible-movement-2 branch January 21, 2022 17:03

skaldarnar added a commit to Terasology-Archived/GooKeeper that referenced this pull request Jan 21, 2022

fix: remove reference to ensure_target_present

cd771fc

Respective action has been removed in Terasology/Behaviors#89

skaldarnar added a commit to Terasology/WildAnimalsGenome that referenced this pull request Jan 21, 2022

fix: remove reference to ensure_target_present

b6e6b9d

Respective action has been removed in Terasology/Behaviors#89

jdrueckert mentioned this pull request Jan 21, 2022

chore: clean up test prefabs #90

Merged

This was referenced Jan 22, 2022

fix: FlexiblePathfinding migration follow-ups #92

Merged

chore: FlexiblePathfinding migration follow-ups Terasology/FlexiblePathfinding#26

Merged

chore: FlexiblePathfinding migration follow-ups Terasology/WildAnimals#94

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Replace Pathfinding with FlexiblePathfinding (part 2) #89

feat!: Replace Pathfinding with FlexiblePathfinding (part 2) #89

skaldarnar commented Jan 13, 2022 •

edited by jdrueckert

Loading

jdrueckert commented Jan 18, 2022

DarkWeird commented Jan 18, 2022

skaldarnar Jan 18, 2022

jdrueckert Jan 18, 2022

skaldarnar Jan 18, 2022

skaldarnar Jan 18, 2022

skaldarnar Jan 18, 2022

skaldarnar Jan 18, 2022

skaldarnar Jan 18, 2022

skaldarnar Jan 18, 2022

jdrueckert commented Jan 30, 2022

feat!: Replace Pathfinding with FlexiblePathfinding (part 2) #89

feat!: Replace Pathfinding with FlexiblePathfinding (part 2) #89

Conversation

skaldarnar commented Jan 13, 2022 • edited by jdrueckert Loading

Follow-ups

jdrueckert commented Jan 18, 2022

DarkWeird commented Jan 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert commented Jan 30, 2022

skaldarnar commented Jan 13, 2022 •

edited by jdrueckert

Loading