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

Precondition Node should run children to completion before checking it's if statement again #904

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsobek
Copy link

@dsobek dsobek commented Dec 27, 2024

I ran across an issue where a Precondition's "if" statement was evaluating from true to false while it's children were still executing. This resulted in the children only executing partway before the Precondition node short circuited the tree by returning the value of the "else" port unexpectedly.

For a more concrete example, suppose I want to implement a "RunOnce" behavior as a subtree, the xml would look like the following:

<Sequence>
    <Script code="first_run := true" />
    <Precondition if="first_run == true" else="FAILURE">
        <Sequence>
            <Script code="first_run := false" />
            <SomeLongRunningAction />
        </Control>
     </Decorator>
 </Control>

Over multiple ticks of SomeLongRunningAction this will shortcircuit return FAILURE from the "else" of Precondition. This PR would allow for SomeLongRunningAction (and any other following nodes that are children of Precondition) to run to Completion.

@facontidavide
Copy link
Collaborator

This is a change in the behavior. Some people may rely on this, even if I agree that it was not my intention.

I am not sure if I should change this or not forthat reason

@facontidavide facontidavide self-assigned this Jan 10, 2025
@dsobek
Copy link
Author

dsobek commented Jan 10, 2025

If people are relying on the current behavior of this node, and are implementing a situation similar to the one I mention here, they are likely experiencing another bug:

If the Precondition node returns early because its "if" condition now evaluates to false after returning a RUNNING state from it's children, the children are left in a running state because resetChild() is not called, which I imagine could have negative effects if there is some required cleanup in a running child.

I'll also note that this node behaves as expected as long as the children return something other than a RUNNING status so I imagine is a rare edge case.

My guess is that this change doesn't affect anybody or maybe very few people since there is a chance they would've encountered this other bug. But if you are concerned about this change, I'd recommend fixing this other bug and adding some logic to cleanup the children if the condition is no longer met.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants