fix: FlexiblePathfinding migration follow-ups by skaldarnar · Pull Request #92 · Terasology/Behaviors

skaldarnar · 2022-01-22T16:14:32Z

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.

Formatted `critter`, `doRandomMove`, `hostileCritter`, and `stray` behavior files with indent of 2 spaces.

The structure of having a guarded behavior lookup (`flee`) and a fallback behavior lookup (`stray`) to me looks like we're good with running nodes until we find the first one that is successful.

Applies simple refactorings such as renaming or inversion of return values to make the code defining the action more readable. In addition, added a comment to discuss whether this should be a (complex) action or rather a behavior.

In addition, adds a couple of debug logs to investigate the control flow when running the action. These debug lines revealed that exceptions during the execution of an action are treated as failed actions. The behavior tree runner logs a warning, but it is easy to miss...

…path action

Annotate methods of MinionMoveComponent with Javadoc (and questions) to understand the internals of that component better. Improve logging of MoveToAction and SetTargetToNearbyBlockNode.

…Action

jdrueckert · 2022-01-23T12:02:11Z

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.

- add log messages when entering a behavior: critter (root), doRandomMove, naiveMoveTo, stray

Now prints out messages in the format: ``` 17:43:24.551 [main] DEBUG o.t.m.behaviors.actions.LogAction - Actor 2502: ---- critter ---- ```

…ehavior" This reverts commit f4444b3.

The pathfinding plugins assume that entities are not affected by gravity. To match this assumption, we chose a movement mode that neglects gravity, but works similar to the regular walking mode: FLYING.

Without this check, we can end up with target blocks that are not walkable, screwing up the pathfinding logic. This small addition will fail the behavior action when the target is not walkable, in the hope that we'll just give it another try. Several ideas on how to further improve this behavior action are added as TODO comments.

This is a special movement plugin that will enable gravity on entities while they are using the falling movement mode.

jdrueckert · 2022-01-30T12:44:14Z

FlexibleMovement as a replacement for PathFinding

- only accept candidate if it increases the distance to the start block

…x/behavior-follow-ups

skaldarnar · 2022-02-12T12:33:03Z

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.

Extract many tests cases from Terasology/Behaviors#92 to tests on the pathfinding itself. Most importantly, this allows us to run these tests as unit tests instead of long-running integration tests using MTE.

…#96) Extracted from #92

…w-ups

jdrueckert · 2022-03-06T13:41:14Z

assets/behaviors/common/doRandomMove.behavior

-    set_target_nearby_block : { moveProbability: 65 }
-  },
-   move_to
+    { log: { message: "in doRandomMove Behavior"} },


do we want to keep this and similar log messages in behavior trees and also in code in for the time being and clean them up later or should we remove them before merging this so we don't forget about them?

I'll add some doc comment to LogAction and remove those debug statements.

See 34b61b8

assets/behaviors/creatures/critter.behavior

skaldarnar

Overall, this PR now does the following:

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

None of the remaining debug log statements does any heavy computation, so it's fine to keep them in (for now) as they are not logged with the default configuration but might prove useful for debugging purposes.

LGTM

skaldarnar · 2022-03-13T19:18:54Z

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

            Vector3ic[] allowedBlocks = Iterators.toArray(
                    Iterators.transform(Iterators.filter(neighbors.iterator(),
-                                    (candidate) -> plugin.isReachable(currentBlock, candidate)),
+                                    (candidate) -> candidate.distanceSquared(startBlock) > currentDistance


This condition is probably not very good as it easily runs into local dead ends, but it at least prevents us from jumping back and forth while searching... We have a TODO below to improve on this.

skaldarnar added 5 commits January 22, 2022 17:04

chore: format (some) behavior files

f13a948

Formatted `critter`, `doRandomMove`, `hostileCritter`, and `stray` behavior files with indent of 2 spaces.

fix?: use plain selector instead of dynamic for critter behavior

f4444b3

The structure of having a guarded behavior lookup (`flee`) and a fallback behavior lookup (`stray`) to me looks like we're good with running nodes until we find the first one that is successful.

chore: formatting and code style improvements

b757a1d

skaldarnar requested a review from jdrueckert January 22, 2022 16:14

jdrueckert and others added 7 commits January 22, 2022 20:00

feat: use pathfinding-based naiveMoveTo instead of direct move_to

c9d654c

fix: write target into MinionMoveComponent.pathGoal required by find_…

6d22410

…path action

fix: mark behavior running while pathfinding is not completed yet

9c87d4e

docs: improve documentation and logging

9dc909d

Annotate methods of MinionMoveComponent with Javadoc (and questions) to understand the internals of that component better. Improve logging of MoveToAction and SetTargetToNearbyBlockNode.

fix: workaround for @In in SetTargetToNearbyBlockAwayFromInstigator…

7530506

…Action

chore: use non-deprecated constructor for CharacterMoveInputEvent

41a1e5f

docs: add more Javadoc and TODOs to MinionMoveComponent

a7fcb86

skaldarnar added 10 commits January 23, 2022 15:29

fix: only render debug overlay when debug mode is enabled

d22ac67

chore: format lure behavior

5f13b30

debug: add more logging to behaviors and actions

e96e40c

- add log messages when entering a behavior: critter (root), doRandomMove, naiveMoveTo, stray

debug: change logging format of LogAction

61c29b3

Now prints out messages in the format: ``` 17:43:24.551 [main] DEBUG o.t.m.behaviors.actions.LogAction - Actor 2502: ---- critter ---- ```

debug: debug log messages for various Actions

8def00e

Revert "fix?: use plain selector instead of dynamic for critter b…

cf88a15

…ehavior" This reverts commit f4444b3.

fix: use FLYING movement mode in Walking and Leaping

5e65041

The pathfinding plugins assume that entities are not affected by gravity. To match this assumption, we chose a movement mode that neglects gravity, but works similar to the regular walking mode: FLYING.

feat: add FallingMovementPlugin

91c750a

This is a special movement plugin that will enable gravity on entities while they are using the falling movement mode.

debug: add more debug logs to movement plugins

81b8fa9

Merge branch 'develop' into fix/behavior-follow-ups

5076d4f

jdrueckert mentioned this pull request Jan 30, 2022

NPCs sometimes ported to (0, 0, 0) on opening in-game menu MovingBlocks/Terasology#4995

Open

jdrueckert added 3 commits January 30, 2022 16:23

feat: optimize findRandomNearbyBlock

55747a9

- only accept candidate if it increases the distance to the start block

Merge remote-tracking branch 'origin/fix/behavior-follow-ups' into fi…

be0cf1f

…x/behavior-follow-ups

chore: clean up comment

11c9a88

test: pretty-print failing tests

9f5a645

jdrueckert added 2 commits February 12, 2022 13:59

test: add down and up again for WalkLeapFall combination

ee2629e

chore: disable failing tests until we fix the associated pathfinding

afb9080

skaldarnar mentioned this pull request Feb 13, 2022

test: add basic pathfinding test cases Terasology/FlexiblePathfinding#27

Merged

This was referenced Feb 19, 2022

chore: add debug logs for movement plugins #94

Merged

feat: simulate gravity-less movement, add movement plugin for falling #96

Merged

refactor: use EntityBuilder for creating test character #97

Merged

jdrueckert added a commit that referenced this pull request Feb 27, 2022

feat: simulate gravity-less movement, add movement plugin for falling (…

7d9a98e

…#96) Extracted from #92

Merge branch 'develop' into fix/behavior-follow-ups

d5a688e

jdrueckert mentioned this pull request Mar 6, 2022

test: add basic movement tests #99

Merged

Merge remote-tracking branch 'origin/develop' into fix/behavior-follo…

0dd9aae

…w-ups

jdrueckert marked this pull request as ready for review March 6, 2022 15:24

chore: reset MovementTests to develop state

975b60b

jdrueckert reviewed Mar 13, 2022

View reviewed changes

jdrueckert and others added 6 commits March 13, 2022 15:04

chore: add empty line

ff46b15

Merge branch 'develop' into fix/behavior-follow-ups

05e184f

doc: explain usage of LogAction; remove debug logs from behaviors

34b61b8

chore: remove unnecessary sequence in critter behavior

dd3b3b0

chore: improve TODO comment for debug rendering

890928d

chore: improve some TODO comments and docstrings

07e123b

skaldarnar commented Mar 13, 2022

View reviewed changes

jdrueckert approved these changes Mar 13, 2022

View reviewed changes

skaldarnar merged commit e14e58d into develop Mar 13, 2022

skaldarnar deleted the fix/behavior-follow-ups branch March 13, 2022 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: FlexiblePathfinding migration follow-ups#92

fix: FlexiblePathfinding migration follow-ups#92
skaldarnar merged 77 commits intodevelopfrom
fix/behavior-follow-ups

skaldarnar commented Jan 22, 2022 •

edited

Loading

Uh oh!

jdrueckert commented Jan 23, 2022

Uh oh!

jdrueckert commented Jan 30, 2022

Uh oh!

skaldarnar commented Feb 12, 2022

Uh oh!

jdrueckert Mar 6, 2022

Uh oh!

skaldarnar Mar 13, 2022

Uh oh!

skaldarnar Mar 13, 2022

Uh oh!

Uh oh!

skaldarnar left a comment

Uh oh!

skaldarnar Mar 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

skaldarnar commented Jan 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdrueckert commented Jan 23, 2022

Uh oh!

jdrueckert commented Jan 30, 2022

Uh oh!

skaldarnar commented Feb 12, 2022

Uh oh!

jdrueckert Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

skaldarnar Mar 13, 2022

Choose a reason for hiding this comment

Uh oh!

skaldarnar Mar 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skaldarnar left a comment

Choose a reason for hiding this comment

Uh oh!

skaldarnar Mar 13, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

skaldarnar commented Jan 22, 2022 •

edited

Loading