fix: take child result into account in ConditionAction::modify

[jdrueckert]

The modify method checks the condition, but doesn't make use of the input parameter BehaviorState result which it gets passed to from DecoratorNode based on previous execution of the node's child.

I believe that this is one of the issues leading to Terasology/WildAnimals#98.

TODO:

• assess whether the original logic was intentional and is actually correct (@casals might know?)
• consider overriding modify in GuardAction as well (or instead?)
• as noted in bug: Lure behavior for sheep not working Terasology/WildAnimals#98 (comment), with this fix, the AI still doesn't seem to behave as expected - this might be a side-effect of this fix or something unrelated...

[keturn]

The "assess whether original logic was intentional" is the kind of thing that makes me look for a unit test.

I see that this change to the code didn't change the result of any the existing engine.logic.behavior tests. 🤔

I'm also not sure how to interpret

Terasology/engine-tests/src/test/java/org/terasology/engine/logic/behavior/SelectorTest.java

Lines 11 to 22 in be0f9de

	@Test
	public void testAllSuccess() {
	assertBT("{ selector:[success, success, success]}",
	Arrays.asList(BehaviorState.SUCCESS, BehaviorState.SUCCESS, BehaviorState.SUCCESS), Arrays.asList(4, 1, 4, 1, 4, 1));
	}
	@Test
	public void testAllFail() {
	assertBT("{ selector:[failure, failure, failure]}",
	Arrays.asList(BehaviorState.FAILURE, BehaviorState.FAILURE, BehaviorState.FAILURE), Arrays.asList(4, 1, 2, 3, 4, 4));
	}

How does that mean 4, 1, 2, 3, 4, 4 ?

[jdrueckert]

I'm also not sure how to interpret

Terasology/engine-tests/src/test/java/org/terasology/engine/logic/behavior/SelectorTest.java

Lines 11 to 22 in be0f9de

	@Test
	public void testAllSuccess() {
	assertBT("{ selector:[success, success, success]}",
	Arrays.asList(BehaviorState.SUCCESS, BehaviorState.SUCCESS, BehaviorState.SUCCESS), Arrays.asList(4, 1, 4, 1, 4, 1));
	}
	@Test
	public void testAllFail() {
	assertBT("{ selector:[failure, failure, failure]}",
	Arrays.asList(BehaviorState.FAILURE, BehaviorState.FAILURE, BehaviorState.FAILURE), Arrays.asList(4, 1, 2, 3, 4, 4));
	}

How does that mean 4, 1, 2, 3, 4, 4 ?

No idea either... I would hope that @casals can shed some light here.

The "assess whether original logic was intentional" is the kind of thing that makes me look for a unit test.

I see that this change to the code didn't change the result of any the existing engine.logic.behavior tests. thinking

The fact that none of the existing tests fails with this change tells me that if the original logic was intentional, we don't seem to have the associated assumptions covered in any of these tests. Would be interesting to understand what the existing tests are supposed to cover.

[casals]

This test checks if the returned states are as expected, according to a predefined enum: https://github.com/MovingBlocks/Terasology/blob/be0f9ded060c09c4aa29cec8bc811ded35dc14c1/engine/src/main/java/org/terasology/engine/logic/behavior/core/BehaviorState.java