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 incorrect registration of behavior trees containing faulty XML #438

Merged

Conversation

schornakj
Copy link
Contributor

@schornakj schornakj commented Sep 28, 2022

This fixes a problem where a behavior tree containing an invalid structure (for example, a BehaviorTree with less than one child or more than two children) could still be instantiated by the factory, even though the XML parser threw and exception when trying to load it. This was possible because adding a BehaviorTree's name to the list of available behavior trees occurred before the check for the validity of the BehaviorTree's XML.

  • Modify XML parsing to add a BehaviorTree node to the list of available trees only if that BehaviorTree is valid.
  • Delete unused tree_names and tree_count variables.
  • Extend the unit tests to reflect this error case. These tests fail without the fix in this PR.
  • Move XML parsing helper functions into an anonymous namespace so they can be used in more places.

@schornakj schornakj changed the title Fix behavior trees with faulty XML being registered and instantiated when they should not Fix behavior trees with faulty XML being registered when they should not be Sep 28, 2022
@schornakj schornakj force-pushed the pr-fix-registering-invalid-tree branch from 8d3f76d to 696331f Compare September 30, 2022 23:08
@schornakj schornakj changed the title Fix behavior trees with faulty XML being registered when they should not be Fix incorrect registration of behavior trees containing faulty XML Sep 30, 2022
@schornakj schornakj marked this pull request as ready for review September 30, 2022 23:09
@facontidavide
Copy link
Collaborator

Thanks for the help.
But I think that you should not remove the function from having public access, since Groot 1.0 relies on that.

Also, seems like you moved some code without much modification, and this makes it very hard for me to understand if you changed something or not.

include/behaviortree_cpp_v3/xml_parsing.h Outdated Show resolved Hide resolved
src/xml_parsing.cpp Outdated Show resolved Hide resolved
@schornakj schornakj force-pushed the pr-fix-registering-invalid-tree branch from 696331f to 88ab089 Compare October 3, 2022 22:34
@schornakj schornakj force-pushed the pr-fix-registering-invalid-tree branch from 88ab089 to 6b113a8 Compare October 3, 2022 22:41
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

2 participants