Skip to content

Conversation

EzraBrooks
Copy link
Member

@EzraBrooks EzraBrooks commented Aug 25, 2025

Closes PickNikRobotics/moveit_pro#14087.

  • Reduces duplication in VerifyXML by handling the ID check for built-in
    node types up front so they can then be definitively looked up in the
    registered nodes.

  • Enhances error messaging in VerifyXML by using either the node name
    or the ID, depending on which is appropriate, instead of leaving
    users guessing "which Decorator is wrong"

  • Fixes custom Action and Condition nodes using shorthand syntax not
    being properly verified

  • Fixes <Control ID="ReactiveSequence"/> not being verified with the
    same logic as <ReactiveSequence/>

  • Fixes <Action ID="MyAction"/> not triggering a behavior lookup when
    <MyAction/> would.

@EzraBrooks EzraBrooks marked this pull request as ready for review August 25, 2025 19:23
@EzraBrooks EzraBrooks requested a review from dsobek August 25, 2025 19:24
Copy link

@dsobek dsobek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but this could also be refactored to use a switch statement, or to dedup code that is common between validating nodes that have node types for names vs nodes that have their ID as their tag.

@EzraBrooks EzraBrooks force-pushed the add-id-to-validation branch 2 times, most recently from f021331 to c977e6c Compare August 26, 2025 20:25
@EzraBrooks EzraBrooks changed the title Include node ID in the VerifyXML failure message Clean up and consolidate VerifyXML logic Aug 26, 2025
@EzraBrooks EzraBrooks force-pushed the add-id-to-validation branch 7 times, most recently from f02e8a6 to daa3d99 Compare August 26, 2025 20:57
@EzraBrooks EzraBrooks changed the title Clean up and consolidate VerifyXML logic Refactor VerifyXML to clarify logic Aug 26, 2025
@EzraBrooks EzraBrooks marked this pull request as draft August 26, 2025 21:05
@EzraBrooks EzraBrooks force-pushed the add-id-to-validation branch from daa3d99 to 97e5b11 Compare August 26, 2025 21:08
- Reduces duplication in VerifyXML by handling the ID check for built-in
node types up front so they can then be definitively looked up in the
registered nodes.

- Enhances error messaging in VerifyXML by using *either* the node name
  *or* the ID, depending on which is appropriate, instead of leaving
users guessing "which Decorator is wrong"

- Fixes custom Action and Condition nodes using shorthand syntax not
  being properly verified

- Fixes `<Control ID="ReactiveSequence"/>` not being verified with the
  same logic as `<ReactiveSequence/>`

- Fixes `<Action ID="MyAction"/>` not triggering a behavior lookup when
  `<MyAction/>` would.
@EzraBrooks EzraBrooks force-pushed the add-id-to-validation branch from 97e5b11 to cc77236 Compare August 26, 2025 21:21
@EzraBrooks
Copy link
Member Author

Uh oh, there's a test that never should've been passing..

@EzraBrooks EzraBrooks force-pushed the add-id-to-validation branch from 10dba30 to 85e3fe3 Compare August 27, 2025 15:45
@EzraBrooks EzraBrooks force-pushed the add-id-to-validation branch from 85e3fe3 to ceadf1a Compare August 27, 2025 15:50
@EzraBrooks EzraBrooks marked this pull request as ready for review August 27, 2025 16:01
@EzraBrooks
Copy link
Member Author

upstream PR here BehaviorTree#1000, but moving on for now.

@EzraBrooks EzraBrooks merged commit c852132 into main Aug 28, 2025
8 checks passed
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