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

Makes BTComposite nodes aware of changes in their children #44

Closed

Conversation

SirPigeonz
Copy link
Contributor

@SirPigeonz SirPigeonz commented Nov 29, 2023

This makes possible to change children of a composite nodes at runtime, allowing for many cool stuff. 😄

I use it for injecting custom behaviors in a skill system in my game.

For AI it could be used to build mob generator, that takes scenes with pre-defined elements and creates many possible combinations, without need to create each scene manually by instancing sub-scenes.

…n be edited at runtime.

This makes possible to change contents of a composite nodes at runtime, allowing for many cool stuff.

I use it for injecting custom behaviors in a skill system in my game.

For AI it could be used to build mob generator, that takes scenes with pre-defined elements and creates many possible combinations, without need to create each scene manually by instancing sub-scenes.
@ThePat02
Copy link
Owner

Nice feature! Here are my thoughts:

  • I think to merge this into main, we need to make sure that every node supports hot-swapping on runtime. I think things are very easily broken if your remove and add nodes to either a BT or a FSM while its running.
  • I think needing to call super() on every extended class is very tedious and can be confusing for new users. Maybe connecting the signal using @onready would solve this. Seems pretty hacky tho.
  • The function needs to check if the node is currently in the editor, because I will soon make all scripts a @tool in order to use configuration warnings.

@SirPigeonz
Copy link
Contributor Author

SirPigeonz commented Nov 29, 2023

Good points! 😃

I think to merge this into main, we need to make sure that every node supports hot-swapping on runtime. I think things are very easily broken if your remove and add nodes to either a BT or a FSM while its running.

Thats true, at first I wanted to inject whole BTrees in my system starting with BTRoot but noticed that FSMStateIntegratedBT can't handle properly changing it's children at runtime. So I decided to do that in Composites because it looked like simple, clean and fitting place to do that, but making it available everywhere makes even more sense.

I can work on it if you want :) (First I will submit PR for 2.0 with delta change)

This will be also quite big change... should we make it 1.x feature or 2.x?

I think needing to call super() on every extended class is very tedious and can be confusing for new users. Maybe connecting the signal using @onready would solve this. Seems pretty hacky tho.

Is it even possible to use @onready for that? I wanted to makse setter that deos that but then I remembered that variables don't call setters on initialization. :( Unfortunately, and at the same time fortunately, manual super() in ready() will stay in GDS 2, technically is consistent and makes sense... but UX wise is meh, especially for Add-ons like this where you can and often should extend addon scripts... In 3.x it was called automatically in _ready() but many users complained that it's not consistent... now other half complainst that it's tedious... xD

I think we can minimize tediusness and make it less error prone and confusing if we would provide it in GDS templates? (I totally forgot to do that my bad! xD)

The function needs to check if the node is currently in the editor, because I will soon make all scripts a @tool in order to use configuration warnings.

Roger will do that! And 👍 🎉 🍾 for conf warning! :D

@ThePat02
Copy link
Owner

Is it even possible to use @onready for that?

I was thinking of something like

@onready var blabla = wrapper()


func wrapper():
# Connect here
```

@SirPigeonz
Copy link
Contributor Author

SirPigeonz commented Nov 29, 2023

I was thinking of something like

@onready var blabla = wrapper()


func wrapper():
# Connect here

What?! I didn't know you can do that! D: This is cool.

That's actually a nice solution! I don't feel it's hacky at all and much better in our use case than super() that user has to remembder or not delete by accident...

@ThePat02
Copy link
Owner

That's actually a nice solution! I don't feel it's hacky at all and much better in our use case than super() that user has to remembder or not delete by accident...

Since connect() does return Error you don't even need the wrapper.

@ThePat02 ThePat02 added enhancement New feature or request needs-work PR needs more work labels Nov 30, 2023
@ThePat02 ThePat02 marked this pull request as draft November 30, 2023 22:39
@ThePat02
Copy link
Owner

ThePat02 commented Dec 3, 2023

Is this PR redundant now that there is #47?

@SirPigeonz
Copy link
Contributor Author

Is this PR redundant now that there is #47?

Ah I forgot. Yes, I left it just in case it might be needed but can be archived I think.

@ThePat02 ThePat02 closed this Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-work PR needs more work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants