Fix Composites and Sub-Trees not ending their Children when exited #113
Conversation
…se cases with optional sub-trees
|
First off thanks for using my open source library and cool game. I'll take a look and review this as soon as I can. Also I'm dealing with jury duty atm and some tight end of year deadlines with work. So it may take me a while to get these changes in as every change requires aggressive testing and manual regression testing (a fork of the library will probably be easier for you than waiting on me to implement everything sadly). |
There was a problem hiding this comment.
The graphical bug display fix is clever here.
I'm not sure on the End calls. Something seems off there as these should be firing normally automatically. Wondering if something is circumventing the End calls in your implementation by accident maybe? As an action game I worked on earlier this year was using End calls that seemed to run just fine. If they didn't trigger I definitely would have noticed animations crashing. But then again maybe I'm wrong and there is an End call edge case I've missed.
If you could walk me through the edge cases you've run into with the End calls it might help me figure out what's going on here. There may be an easier fix here if there is an edge case to make sure nodes are exiting properly. Also I'd need to add tests to these to prove out the edge cases before this could go into the code base. A lot of live games are using this library so gotta make sure all bases are covered before putting changes in.
| AddBox(container); | ||
|
|
||
| if (task.Children != null) { | ||
| if (task.Children != null && task.Children.Count > 0) { |
There was a problem hiding this comment.
Ah nice. Yeah I can see this was creating a graphical display bug
|
|
||
| public BehaviorTreeBuilder Splice (BehaviorTree tree) { | ||
| _tree.Splice(PointerCurrent, tree); | ||
| if (tree != null && tree.Root != null) { |
There was a problem hiding this comment.
Hmmm, I feel like this should technically fail if you're passing in an empty null tree as most people would probably want to fix the error in that case. Maybe throwing an intentional error here might be more appropriate? I might modify the PR to remove this but feel free to debate me on this one (seems like the best path forward to me). What specific edge case were you hitting with this?
| base.Reset(); | ||
| } | ||
|
|
||
| protected void NotifyChildEnd(int childEnding) { |
There was a problem hiding this comment.
Is the last child not getting an End call on it when the BT tree completes? If so I'll need to get a test in to verify this and that it fixes the created bug. And also do manual regression testing.
It looks like there is code in here to call End on tasks automatically instead of manually calling like this. So I'm wondering why this would be needed?
Curious what the logic here is to forcibly call end on all the composites. If it's just not being triggered at all that would make sense. But I'm pretty sure End is firing fine in my code base last I checked. Please share more on your specific use case.
2 participants
(Plus a couple of null deref fixes.)
I was having issues when I first set up this plugin on my work project; various stateful things were hanging around unexpectedly. I realised this was missing, and once added, everything has worked well with it for the past 8 months (the game is in Early Access: https://store.steampowered.com/app/1724030/Eyes_of_Hellfire/).
I couldn't get those random changed 'using' directives to not show as modified, whatever I tried to do with line endings in VS and git, so hopefully not an issue!
And thanks for the super helpful plugin! It was just the thing I needed to stand up a bunch of AI behaviours with minimal code.