ThePat02 / BehaviourToolkit

A collection of tools for AI Behaviour in the Godot 4 Game Engine!
MIT License
303 stars 12 forks source link

add: Dynamic changes to Behaviour Patterns at runtime #47

Closed SirPigeonz closed 6 months ago

SirPigeonz commented 7 months ago

This PR makes both FSM and BTree systems work properly when their nodes are moved, removed, added at runtime.

On top of that many nodes are now fine when they don't have important for them nodes as a child or property references. This allows for preparing "empty" slots for intended behaviours or branches and modeling main behaviours with them, that is "slots", in mind. Upcomming warning system can be used instead for important configuration warnings when behaviours are created by hand in the Editor.

Changes:

Changes after review 1

Changes after review 2

If you wish to add, remove, move FSMState nodes at run-time first add new FSMStates stop the FSM with method FiniteStateMachine.exit_active_state_and_stop and re-start it with method method FiniteStateMachine.start providing one of the new states either as start method property or change member FiniteStateMachine.initial_state before running start(). After this procedure you can delete unused states.

ThePat02 commented 7 months ago

Keep in mind that some things might change how everything is handled, since #17 requires every script to be a @tool!

SirPigeonz commented 7 months ago

Keep in mind that some things might change how everything is handled, since #17 requires every script to be a @tool!

Yup :) I already added few guards with if Engine.is_editor_hint().

SirPigeonz commented 7 months ago

@ThePat02 Ready for review :)

This time for sure xD

ThePat02 commented 7 months ago

Good work! I'll look into it when I finished implementing #17 to avoid any breaking conflicts.

SirPigeonz commented 7 months ago

Good work!

Senkyu! :3

I'll look into it when I finished implementing #17 to avoid any breaking conflicts.

Do as you prefer :) Although, I would personally do opposite, because I changed some behaviours of the addon. Some configurations that were illegal and crash in the past now are legal just do nothing and would need different type of warning and asserts. (I'm just worried that some of your work will be not needed).

That said I tried to test it diligently as I could. Each node was tested alone and with others and main example also runs as used to. I also plan to move to this version (2.0 + this PR) with my prototype so I will probably also catch regressions.

What I want the most is it for you to look into the design and tell me what you think and if I missed something, or should change something :)

SirPigeonz commented 7 months ago

One other thing I noticed: When changing states of a FSM, what happens if you change the active state? Did the current state exit?

Good question! Hmm, I should add active_state guard for processing too.

When you change state with change_state() it runs states exit same with transitions. Or do you mean is it running exit when deleted with Node API? Then no it doesn't run exit. Should it?

We could add method to FiniteStateMachine for deleting states that calls exit if state is current active_state, for situations it's needed.

What do you think?

SirPigeonz commented 7 months ago

@ThePat02 Adressed few points in new commit:

ThePat02 commented 7 months ago

One other thing I noticed: When changing states of a FSM, what happens if you change the active state? Did the current state exit?

Good question! Hmm, I should add active_state guard for processing too.

When you change state with change_state() it runs states exit same with transitions. Or do you mean is it running exit when deleted with Node API? Then no it doesn't run exit. Should it?

We could add method to FiniteStateMachine for deleting states that calls exit if state is current active_state, for situations it's needed.

What do you think?

Well what is the workflow with dynamic changing of nodes? There needs to be some kind of failsafe when changin states manually and via code.

SirPigeonz commented 7 months ago

Well what is the workflow with dynamic changing of nodes? There needs to be some kind of failsafe when changin states manually and via code.

Ahhh now I understand, sorry 😁 Ok yeah, there should be some safe API to model states the way user needs for their use case. I will think more about it and write my API ideas.

ThePat02 commented 6 months ago

I think it is best to push this feature to a later version of the plugin (possibliy 2.1) as there are some other pitfalls and problems that come with this. I think it is very important to release the new syntax changes, as more and more people are interested in using it. I'd like to avoid problems like people having to rewrite their code with the new syntax too much.

SirPigeonz commented 6 months ago

I think it is best to push this feature to a later version of the plugin (possibliy 2.1) as there are some other pitfalls and problems that come with this. I think it is very important to release the new syntax changes, as more and more people are interested in using it. I'd like to avoid problems like people having to rewrite their code with the new syntax too much.

I'm fine if it will be in 2.1 if you want to release 2.0 early due to changes in API. I use latest code in my game either way.

Furthermore, I just finished a bigger commission for a client, so I will have more free time now to work on this and my game. I will probably finish a new version of this PR today or tomorrow.

ThePat02 commented 6 months ago

Sounds great. If you need anything, make sure to add me. I'll unsub for now so I don't get notis for every commit!

SirPigeonz commented 6 months ago

@ThePat02 Ready for review again :)

ThePat02 commented 6 months ago

I'll take a look at it after finishing up the other changes. BTW I added a formatting action you can steal for your own projects.