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

Add multiple concurrent conditions support #30

Open
adamscott opened this issue Nov 23, 2021 · 8 comments
Open

Add multiple concurrent conditions support #30

adamscott opened this issue Nov 23, 2021 · 8 comments

Comments

@adamscott
Copy link
Contributor

Transitions should support concurrent conditions. Being able to transit on condition groups instead of simple conditions.

Each condition in the condition group must be true for the condition group to be true. But if a condition group of many is true, the transition is done.

I built a working prototype, but this will need some polish to clean up things.
https://github.com/adamscott/gd-YAFSM/tree/condition-groups

Capture d’écran 2021-11-22 220518

Capture d’écran 2021-11-22 220544

@imjp94
Copy link
Owner

imjp94 commented Nov 23, 2021

Cool! I always wanted to support complex condition.
Your solution is really interesting, I suppose contents of the group act like AND while groups itself act like OR.

However, there's something that concerns me:

  • It isn't obvious to the user of the behaviour of the groups and theirs contents(AND/OR)
  • Just my assumption, users might still ask for even more complex condition, and then we would have to implement nested condition. Which I think is over-engineered and it would be hard to make decent UI for such complications.

My point of view is that such system should be as simple as possible and readability is the key.

For me myself, I still haven't figured out the perfect way to tackle this feature. The best I can think of is an ExpressionCondition

@adamscott
Copy link
Contributor Author

Talking of ExpressionCondition, I just implemented one in my branch. It uses params, but the expression itself is in the condition.
I'll create a new branch for a quick PR, maybe. Further testing is needed, but it seems to work as-is.

The code for it is dead simple. I'm just inputting the params and their values as Expression input variables. Expression is one heck of a class.

Capture d’écran 2021-11-23 124222
Capture d’écran 2021-11-23 124237
Capture d’écran 2021-11-23 124303

@adamscott
Copy link
Contributor Author

Cool! I always wanted to support complex condition. Your solution is really interesting, I suppose contents of the group act like AND while groups itself act like OR.

However, there's something that concerns me:

* It isn't obvious to the user of the behaviour of the groups and theirs contents(AND/OR)

* Just my assumption, users might still ask for even more complex condition, and then we would have to implement nested condition. Which I think is over-engineered and it would be hard to make decent UI for such complications.

My point of view is that such system should be as simple as possible and readability is the key.

For me myself, I still haven't figured out the perfect way to tackle this feature. The best I can think of is an ExpressionCondition

It was essential to me to have these condition groups, because now, I can further dissociate behavior from the code.

Imagine a button that triggers "skip" to the state machine. And you have a state Cutscene that can transition in the state Game if "skip" is triggered or if "cutscene_end" is triggered.

The skip button does not have to implement "cutscene_end" or trigger a function that, depending on the context, will trigger cutscene_end if your're in the state Cutscene.

We can now more easily use triggers like "timeout" made by Timers, as an example. Then, using the FSM, behavior will follow if implemented. Less need to implement logic that will trigger specific triggers if conditions are reunited.

@imjp94
Copy link
Owner

imjp94 commented Nov 23, 2021

Just did a quick test on the PR and it is impressive!

Cool! I always wanted to support complex condition. Your solution is really interesting, I suppose contents of the group act like AND while groups itself act like OR.
However, there's something that concerns me:

* It isn't obvious to the user of the behaviour of the groups and theirs contents(AND/OR)

* Just my assumption, users might still ask for even more complex condition, and then we would have to implement nested condition. Which I think is over-engineered and it would be hard to make decent UI for such complications.

My point of view is that such system should be as simple as possible and readability is the key.
For me myself, I still haven't figured out the perfect way to tackle this feature. The best I can think of is an ExpressionCondition

It was essential to me to have these condition groups, because now, I can further dissociate behavior from the code.

Imagine a button that triggers "skip" to the state machine. And you have a state Cutscene that can transition in the state Game if "skip" is triggered or if "cutscene_end" is triggered.

The skip button does not have to implement "cutscene_end" or trigger a function that, depending on the context, will trigger cutscene_end if your're in the state Cutscene.

We can now more easily use triggers like "timeout" made by Timers, as an example. Then, using the FSM, behavior will follow if implemented. Less need to implement logic that will trigger specific triggers if conditions are reunited.

But wouldn't it be easier to use expression in this case?
Screenshot 2021-11-24 063045

@adamscott
Copy link
Contributor Author

Mmm. I'll think about it! It could simplify stuff, sure. Like, you're right, everything done with condition groups are done with expression conditions.

But at the cost of readability. As every condition are "AND" binded, I kinda suspect that expressions will be bloated when the complexity of a transition is anything more than simple. Imagine something like (trigger["skip"] and wait_time > 5.0 and player_state == "idle") or trigger["timeout"] Or anything more complex. It begins to be quite large in the editor, no?

With condition groups, expressions have more chances to be kept small. And expressions are less needed. Like in the last example, no need for expressions.

@imjp94
Copy link
Owner

imjp94 commented Nov 24, 2021

Expression
Screenshot 2021-11-24 072847
Screenshot 2021-11-24 072935
Condition group
Screenshot 2021-11-24 073354
Screenshot 2021-11-24 073418

From perspective of UI, both expression and condition group has their own strength & weakness. Expression looks minimal in inspector while condition group looks more organized in graph.

By comparison, yes, condition group is definitely easier to read in the graph, but the long expression is still very self explaining, while it can still be beautified by wrapping text or even syntax highlighting or break expression into boxes like condition group.

But what really makes me leans toward expression is that it is more flexible, users are free to write expression from any levels of complexity. For example, expression like trigger["timeout"] and (is_a or is_b) is impossible to achieve in condition group unless it is nestable.

So, the workflow with ExpressionCondition would somehow look like this:

  1. Simple condition with primitive Condition class(FloatCondition, StringCondition etc.)
  2. Use ExpressCondition to take advantages of AND/OR, like trigger["skip"] or trigger["timeout"]
  3. Use ExpressCondition to write really complex condition, like (trigger["skip"] and wait_time > 5.0 and player_state == "idle") or (trigger["timeout"] and (is_a or is_b))

@adamscott
Copy link
Contributor Author

Well, I'm not talking about nuking the ExpressionCondition, haha.
Capture d’écran 2021-11-23 203542

But I understand your point. For now, it appears to be a good solution to only merge ExpressionCondition, not condition groups. But if the need rise, we know that's possible.

@imjp94
Copy link
Owner

imjp94 commented Nov 24, 2021

Well, I'm not talking about nuking the ExpressionCondition, haha.
Capture d’écran 2021-11-23 203542

That's pretty cool!

But I understand your point. For now, it appears to be a good solution to only merge ExpressionCondition, not condition groups. But if the need rise, we know that's possible.

That's a good idea. Who knows, maybe I would prove myself wrong afterwards

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

No branches or pull requests

2 participants