Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jdrueckert
Copy link
Member

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:

@jdrueckert jdrueckert added Type: Bug Issues reporting and PRs fixing problems Status: Needs Investigation Requires to be debugged or checked for feasibility, etc. Category: Gameplay Content Requests, Issues and Changes targeting gameplay mechanics and content Topic: AI Requests, Issues and Changes related to pathfinding, behaviors, etc. Status: Needs Testing Requires to be tested in-game for reproducibility Type: Question Issue intended to help understanding something that is unclear labels Jun 6, 2022
@keturn
Copy link
Member

keturn commented Jun 6, 2022

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

@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
Copy link
Member Author

jdrueckert commented Jun 6, 2022

I'm also not sure how to interpret

@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
Copy link
Contributor

casals commented Jun 7, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Gameplay Content Requests, Issues and Changes targeting gameplay mechanics and content Status: Needs Investigation Requires to be debugged or checked for feasibility, etc. Status: Needs Testing Requires to be tested in-game for reproducibility Topic: AI Requests, Issues and Changes related to pathfinding, behaviors, etc. Type: Bug Issues reporting and PRs fixing problems Type: Question Issue intended to help understanding something that is unclear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants