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

Added a few new features for FSMs #75

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dfego
Copy link
Contributor

@dfego dfego commented Jan 8, 2024

I was working some with the FSM capabilities and wanted to add some additional functionality. The three core things added here:

  • Split Transition: Transitions that always transition based on a boolean check, and have two next states. I did what I could to work within the existing framework here, so there still exists a bare "next state" on this, but it warns the user if it's used.
  • Always Transition: A simple transition that always goes to the next state, which is useful for things that occur and always want to move on.
  • Print on Enter State: The equivalent to the LeafPrint for BT, just prints some text on enter.

Of course feel free to reject this based on not wanting these particular features in here, or let me know if there's anything else you'd like to see changed so it fits better. I just figured if I was doing some of this work for myself, might as well pay it forward!

I think I covered all the bases here as well.

  • Added the element classes
  • Added script templates where appropriate
  • Added an example to show how they work
  • Added to the toolbox in the UI
  • Added unique icons for two of them (and reused the print one)
  • Added to the documentation

dfego added 3 commits January 8, 2024 16:48
I had a use case where I had booleans, always causing a transition from
a given state, but to different paths. Before, I had to create two
separate transitions, but now I can create a transition where I assign
two next states, one on true and one on false, based on a function
written in an extended script.
@ThePat02
Copy link
Owner

ThePat02 commented Jan 8, 2024

Hello! First of all: Thank you for contributing to this plugin. I will review your changes in the next few days, when I got a little more time on my hand! Just some quick notes:

  • I think a "PrintState" is not a very useful addition. I get that it can be pretty handy for debugging or creating your conditions, but ultimately it is not of much use. I think a toggle on the StateMachine could be a better solution in the future, when we < finally > add debug features. You can't really use this Node in any meaningful way, since extending it would override _on_enter (Well, I guess you could call super, but I think you get my point)
  • Same thing with the "Always". I think a bool on a transition would be achieve the same, without adding anymore nodes.
  • The "Split" is a pretty good idea. I'm not quite sure how to correctly handle it tho, but this is a nice addition!

One thing to keep in mind. We will soon merge #73 by @SirPigeonz that will allow users to swap around nodes freely inside Behaviour Patterns freely. We will need to see if there are any breaking changes!

@ThePat02 ThePat02 self-requested a review January 8, 2024 23:36
@ThePat02 ThePat02 self-assigned this Jan 8, 2024
@dfego
Copy link
Contributor Author

dfego commented Jan 9, 2024

Sounds good! Once you review it, if you'd like to move the print and always to flags, I can see about making those changes! This was just the path that was most obvious to me at the time.

I will say, as I look at adding a flag for "always transition" to FSMTransition, it gets awkward. While the way I implemented it was single-purpose, it made clear it wasn't meant to be extended. I'm not sure how to property achieve "always transition" being a flag while making it not overridable by an extended script without making changes to fsm.gd itself. But perhaps that's the way to go. Part of me wants to go poke at this, but I'm going to wait because this is your project and I'd like to follow your design!

Thanks about Split! I chose this path because it required no external changes outside of my class. I didn't have to modify the state machine or anything else, it just works as-is (if you use it correctly). That said, it not being a subclass of FSMTransition could also work, but then it would require more special-case code.

I'll hang tight on this one until after #73 comes in and then I'll work with you on figuring things out!

Also thank you so much for this plugin. It's great functionality, reasonably well documented (which is why I chose it over alternatives), and feels like it has a lot of polish.

@dfego
Copy link
Contributor Author

dfego commented Jan 9, 2024

Alright, I've investigated how to conditionally modify Inspector variables (via _get_property_list()) and have an idea of how to deal with things like dependent variables (such as "Always Transition" disabling "Event" and "Use Event"). I'd just need some guidance on how you'd like that to behave. It's possible to gray them out (make them readonly), which leaves them visible but also won't clear their values. I believe it's also possible to remove them from the editor without clearing their values. Clearing their values and removing them would also be possible, but that might be annoying if the box gets checked accidentally.

@ThePat02
Copy link
Owner

ThePat02 commented Jan 9, 2024

I am not quite sure. One thing I thought of tho: Having the Split note being some sort of composite node instead of a transition.

Finite State Machine

State

Transition
Split

Transition
Transition

But I'm not at my PC right now, so...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed before, having an always_transition property bool on FSMTransition would be a better idea. This should be checked in the FiniteStateMachine class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sketched this out locally, and ended up implementing a new property (with some dependency juggling with event and use_event) and just replaced is_valid():

## Evaluates true, if the transition conditions are met.
func is_valid(_actor: Node, _blackboard: Blackboard) -> bool:
	if always_transition:
		return true

	return false

This would keep everything else in place, because FiniteStateMachine fundamentally doesn't need to change -- it's just the transition itself that acts differently. Do you seen an advantage to exposing this logic to the FiniteStateMachine class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this to show what I mean. If you'd like the implementation to change a bit it'll be easier now that it's in Transition.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A own print state node is overkill - however this feature should totally be part of a debugging toolset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it into examples and removed class_name.

@SirPigeonz
Copy link
Contributor

I am not quite sure. One thing I thought of tho: Having the Split note being some sort of composite node instead of a transition.

Finite State Machine

State

Transition
Split

Transition
Transition

But I'm not at my PC right now, so...

That might be a bit of an overkill :D and harder to use.

@dfego
Copy link
Contributor Author

dfego commented Jan 9, 2024

I am not quite sure. One thing I thought of tho: Having the Split note being some sort of composite node instead of a transition.

Finite State Machine

State

Transition
Split

Transition
Transition

But I'm not at my PC right now, so...

I'm interested in what you see as the advantage to this direction, so I can try and understand it. One of my goals was to reduce the number of transitions I'd need to write for dead-simple boolean checks that lead in two different directions. This wouldn't exactly achieve that goal.

@SirPigeonz
Copy link
Contributor

SirPigeonz commented Jan 9, 2024

* **Split Transition:** Transitions that always transition based on a boolean check, and have two next states. I did what I could to work within the existing framework here, so there still exists a bare "next state" on this, but it warns the user if it's used.

Love the idea! :D

* **Always Transition:** A simple transition that always goes to the next state, which is useful for things that occur and always want to move on.

I like it. Although, Pat doesn't like many nodes. I have a node like this already in my project tho, xD I will steal your icon! xD (I was too lazy to make one yet)

* **Print on Enter State:** The equivalent to the `LeafPrint` for BT, just prints some text on enter.

I agree this should be part of debug tools, not a separate state. I planned to add signals for basic nodes events like tick() and on_enter(), on_update(), but didn't have time lately. Those could be used for debug as well. For example, you could just make a node that you add as a child of the state node and it will handle the printing ba connecting to on_enter signal.

Cool addition thx for the PR! :)

@ThePat02
Copy link
Owner

ThePat02 commented Jan 9, 2024

I like it. Although, Pat doesn't like many nodes. I have a node like this already in my project tho, xD I will steal your icon! xD (I was too lazy to make one yet)

Well, I think stuff like "always" can be simply included in the base node.

@SirPigeonz
Copy link
Contributor

I like it. Although, Pat doesn't like many nodes. I have a node like this already in my project tho, xD I will steal your icon! xD (I was too lazy to make one yet)

Well, I think stuff like "always" can be simply included in the base node.

I agree, but I have a "but". If you add too many features in the base class when you DO want to extend the class, you have to handle all those features in the child classes even if they don't need some features. For example, SplitTransition will not have much use of the always bool. On top of that, it's nice to have icons for different types of nodes, it helps when working with behaviors. :D

@dfego
Copy link
Contributor Author

dfego commented Jan 10, 2024

I like it. Although, Pat doesn't like many nodes. I have a node like this already in my project tho, xD I will steal your icon! xD (I was too lazy to make one yet)

Well, I think stuff like "always" can be simply included in the base node.

I agree, but I have a "but". If you add too many features in the base class when you DO want to extend the class, you have to handle all those features in the child classes even if they don't need some features. For example, SplitTransition will not have much use of the always bool. On top of that, it's nice to have icons for different types of nodes, it helps when working with behaviors. :D

Agreed. I'm already experiencing it with the "next" transition member when I have a split transition, and having incompatible modes (like "always" in "split" when that simply can't work) can lead to confusing code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants