Fix Composites and Sub-Trees not ending their Children when exited#113
Fix Composites and Sub-Trees not ending their Children when exited#113achynes wants to merge 3 commits into
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.
|
Thanks @ashblue! Please go ahead and change whatever you need to, I'm finished up with this for the time being anyway. Interesting, I must have missed the auto-end stack when I was debugging. I don't think I'm doing anything particularly weird with my trees. Unfortunately I can't share my examples from the project and am a bit snowed under at the moment to make and test a modified example. In any case, I'll leave this with you now and again thanks so much for the plugin! All the best! |
|
I will say, the cases where I had issues were Tasks that would run actions on a seperate system for async work in the game, e.g. 'Go over there and attack that character'. I found that when the sub-tree holding that Task (and therefore Actions) exited, it didn't End the Task, so the Actions didn't end either, leading to characters continuing to move/animate/etc. while other new behaviours were also kicking off. The ending of the Actions was called from the OnExit of the Task, and that seemed to work fine once I added the code in this PR. |
|
@achynes thanks for the follow up response. I've broken this off into a separate ticket to research and figure out what might be going on here as the edge case will need a patch. I'm assuming you noticed this behavior with an async task maybe? Or was it specifically just any sub-tree. Just curious as I'll have to tinker to figure out what loses the end event. Sub-trees shouldn't cause issues as they're copied into the main tree normally at runtime. I'm wondering if you found some sorta async task bug. |
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.