Return None if ticking BT after it finishes.
#45
Conversation
|
Some more things to point out: I used Result instead of Option since a None value isn't every descriptive. We also can't add a |
|
I would honestly stick with |
|
Yes, agree with @kaphula. Return an 'Option' instead fits nicer, also because ticking tree after finish is not an error per se. I'd prefer us not introducing any new dep if not strictly needed. Thanks for picking up this task @andriyDev, remember to bump version 👍 |
|
First I don't agree this isn't an error - you clearly shouldn't be ticking a finished tree, and we are exiting early because we're in an "invalid" state. Second, I don't think having no other error cases is a good reason to not have an expressive error case. Adding additional error cases in the future would be much less cumbersome, and debug printing the error even with this can make it clear what's going on (compare "None" to "hey don't tick a finished behavior tree"). Third, while I understand the desire to limit the number of dependencies (a worthy goal in general), All that said, I've still made the commit to switch to |
|
@Sollimann also to follow up, we don't need to bump the version since that already happened in #38. We are currently at 0.9.0. |
|
Rebased now that #42 is merged. |
BT after it finishes.None if ticking BT after it finishes.
There was a problem hiding this comment.
Approved!
TL;DR
I understand where you're coming from, and I appreciate the effort. Returning an Option feels like the most appropriate and lightweight solution for now. It avoids overcomplicating the api and keeps the door open for refactoring later if the need arises.
To your points:
1.) I think we disagree on the semantics here, which is fine. From my point of view ticking a finished tree is not inherently erroneous — it's an unsupported action, but not a failure of execution or logic. The only two possible states here are "tree produces a status" (some value) or "tree is finished" (none). If we ever encounter additional cases where we need more meaningful differentiation down the line, we can always refactor to introduce a more expressive type at that point. However, at the moment, Option is the simplest and most idiomatic fit for the problem we're solving (from my pov).
2.) I agree with this
3.) I also agree that thiserror are one of the more common and stable libs. I want to avoid dependency bloat which is common for so many libs out there. As the code evolves, if we encounter scenarios that truly require advanced error handling, we can reevaluate and introduce a proper error-handling strategy (including dependencies) then
Today, ticking a behavior tree after it has finished has confusing results. Looking at the code, it seems the intent was that once a
Statefinishes, it is not expected to tick again. For example, if you tick a Sequence/Select after it has finished, it always returnsRunning. Here's an example test that shows this strange behavior:This is clearly wrong. We could fix this particular issue (make Sequence/Select return a finished status if you tick it again), but it's not clear that ticking a finished behavior makes sense at all - so perhaps we should stop that.
This PR solves this by just keeping track of the BT returns a Success or Failure status and then prevents ticking in those cases (returning an Option)