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 delayed subscription missing first message and change topic name #52

Merged
merged 7 commits into from
Apr 12, 2024

Conversation

tony-p
Copy link
Contributor

@tony-p tony-p commented Apr 2, 2024

Fixes issue #51

*
* @return true will clear the message after ticking/processing.
*/
virtual bool clearProcessedMessage() { return true; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why by default this should return true. Can you elaborate? When should a user override this?

Copy link
Contributor Author

@tony-p tony-p Apr 5, 2024

Choose a reason for hiding this comment

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

That retains current behaviour of the node. Anything else and current default implementation behaviour will change

@facontidavide
Copy link
Collaborator

Thanks, but I would not customize the retention policy through inheritance.

Also, it is not very clear what the intent is. What about adding a parameter in the constructor, such as bool latching or change the vistual function to override to bool isLatching() const?

style: update from review

Co-authored-by: Davide Faconti <davide.faconti@gmail.com>
@tony-p
Copy link
Contributor Author

tony-p commented Apr 5, 2024

Thanks, but I would not customize the retention policy through inheritance.

Also, it is not very clear what the intent is. What about adding a parameter in the constructor, such as bool latching or change the virtual function to override to bool isLatching() const?

It is more flexible (some more discussion in #42). For example a colleague wanted to clear it based on a port value, this can easily be added in the overridden definition, although for those particular use cases I think it was better to just define 2 actions

Maybe there are other uses cases basing it on the content of the message itself ... but haven't hit that myself yet

Definitely open to better naming

@facontidavide facontidavide merged commit 57fbeee into BehaviorTree:humble Apr 12, 2024
1 check 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