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

Bug fix/set blackboard #955

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

devis12
Copy link

@devis12 devis12 commented Apr 1, 2025

SetBlackboard action bug fix

Previous SetBlackboard action node did not update the sequence id, the timestamp when called on a pre-existing entry.
Moreover, it allowed to bypass type checking.

This occurred only when the input was a blackboard pointer.

By using the BT::Blackboard::set method I believe also is thread safe which before was not, but I provided test that were failing previously and now are passing exclusively for the two cases above

devis12 and others added 3 commits March 31, 2025 12:37
Previous SetBlackboard action node did not update the sequence id, the timestamp when called on a pre-existing entry.
Moreover, it allowed to bypass type checking.
@devis12
Copy link
Author

devis12 commented Apr 7, 2025

Btw... By investigating on this bug, I realized that how the Blackboard::set itself is handled is pretty weird wrt port remappings. Take this tree for instance....

<?xml version="1.0"?>
<root BTCPP_format="4" main_tree_to_execute="BehaviorTree">
    <!-- ////////// -->
    <BehaviorTree ID="BehaviorTree">
        <Sequence>
            <SetBlackboard output_key="pos" value="0.0;0.0" />
            <KeepRunningUntilFailure>
                <ForceSuccess>
                    <Sequence>
                        <UpdatePosition pos_in="{pos}" x="0.1" y="0.2" pos_out="{pos}"/>
                        <SubTree ID="A" _autoremap="true" new_pos="2.2;2.4" />
                        <Sleep msec="500"/>
                    </Sequence>
                </ForceSuccess>
            </KeepRunningUntilFailure>
        </Sequence>
    </BehaviorTree>
    <BehaviorTree ID="A">
        <Sequence>
            <SetBlackboard output_key="pos" value="3.0;5.0" />
        </Sequence>
    </BehaviorTree>
    <!-- ////////// -->
    <TreeNodesModel>
        <SubTree ID="BehaviorTree" description=""/>
        <SubTree ID="A" description="">
            <input_port default="1.0;1.0" name="new_pos">Updated position</input_port>
        </SubTree>
    </TreeNodesModel>
    <!-- ////////// -->
</root>

It's completely useless... I grant you that, but it should work... Some key points to understand it:

  • The KeepRunningUntilFailure and ForceSuccess are just a combo trick to repeat the sequence... You can repeat it two times to have the same result
  • pos in the root blackboard is strongly typed (i.e. custom type "Position2D") because of UpdatePosition node
  • It is auto-remapped in the subtree A where I use an on the fly instantiated entry to push into pos essentially an Any of type string (the type checking to resolve the port remappings are not triggered because of the fact that the string value is not strongly typed)

The first iteration runs without any issue, but on the second I would have a Any::copyInto failure due to the fact that the UpdatePosition is trying to push there through the SetOutput call an object of type Position2D, but encounters a string....

Please note that this error is not due whatsover to my modifications proposed in the PR and it's not resolved by them either... Maybe we shall rethink a little bit how the current port remapping and type checking work (for instance... the type check when there is an autoremap to resolve is not as flexible as when there is not: in the latter case string conversion or math acceptable casts are accepted, while in the former not....)

I hope the explanation is sufficiently clear.. @facontidavide what do you think about it ?

root added 2 commits April 7, 2025 14:33
Made sure to handle possible type inconsistencies which might arise when using port remapping with subtrees.
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.

None yet

1 participant