Add ExpressionCondition and ExpressionConditionEditor

[adamscott]

This PR adds a new condition based on the Godot class Expression for making complex conditions.

The user can enter an expression using params as variables. The condition will pass if the expression returns true, or will fail if there is an error or if the expression returns false.

For triggers, it's a little more complicated. As triggers are null params values, and Godot expects named input variables, it's not possible to directly use triggers in the condition. The same could be said standard params, but usually, params are continually there, but updated. Triggers are currently deleted from params after use.

So, to be able to test true or false on a trigger, "trigger" in the expression can accept parameters and will return true or false if they exist. So, expressions like this are possible: trigger["board"] and height > 50.0

This intends to fix in part #21

Here's some screenshots of my PR:

[imjp94]

Just figure out a way to support trigger in expression without syntax like trigger["trigger_name"] so cleaner expression like skip or timeout is possible, see commit b7ad09c, maybe you can pull it to your branch

It works by extracting parameter names from expression with the help of regex.

[adamscott]

It works by extracting parameter names from expression with the help of regex.

I'm not in favor of this commit. This could bring bugs and this reduce the potential of the ExpressionCondition. The intent is nice, but for documentation purposes, it's rather easy to say that the expression parsing is compatible with the Expression class, and that there is a trigger variable.

Expression accepts valid GDScript syntax and some functions (like pow()).

An expression can be made of any arithmetic operation, built-in math function call, method call of a passed instance, or built-in type construction call.

An example expression text using the built-in math functions could be sqrt(pow(3,2) + pow(4,2)).

I don't think regex is the right way to parse scripts.

I rather use a shorter trigger variable name, like T. And yes, we can access the trigger variable using the dot syntax instead of the array syntax. So, T["timeout"] and T.timeout are the same.

We could use a custom function too that makes sure no params are overridden. We could use a base instance with the method trigger() to be used like this: trigger('timeout'), which could be easily read and used.

The image condition could be read as loaded and (trigger("timeout") or trigger("skip")), which reads quite nicely.

[adamscott]

I modified my ExpressionCondition to set the variable instance as the base instance of the expression, with trigger(trigger_name) and t(trigger_name) functions. t() is just a shorthand for trigger(). And it works.

[adamscott]

I modified the visual of the ExpressionCondition to indicate the script nature of the condition without the need of an icon.

The expression wraps too in the TextEdit.

[imjp94]

Well, the main reason that I hope to simplify the syntax of trigger in expression was not just for the sake of visual, but I don't want trigger to be presented to user as anything else but a parameter

Trigger or any other parameters are nothing but a string to look up from Dictionary.

I think here are the challenges that we are facing right now:

• There's no easy way to pass default value when parameters are missing, I think that's why you implemented TriggerInstance class while I choose regex
• Even if we can resolve the first problem, we would still have to pass the right type of default values, otherwise, error like this execute: Invalid operands to operator >, bool and int. would still be raised

So, I think the key to final solution is that, it must be able to pass default values to Expression.execute

And, here are the potential solutions that we have right now:

• Use ParameterInstance(similar to TriggerInstance) but it wraps all kind of parameters, so the syntax would be like param("button_pressed") or param("speed") > 10 and param("speed") < 20
• Regex that can precisely extract all parameters, which can be easily achieved with a special prefix character, let's said $ and the the syntax would be like $button_pressed or $speed > 10 and $speed < 20

Still, I can't think of a nice way to pass the right type of default value

[adamscott]

Trigger or any other parameters are nothing but a string to look up from Dictionary.

Yes and no. Trigger are the culprit here. They are specifically distinct as the fact that they only exist for a cycle with a null value, which is special. Null should be replaced someday with a special Trigger type, to make sure that it does not conflict with a potential valid null param (which I see potential uses, like setting a reference as a param).

So, the way you implemented gd-YAFSM, yes it's the case. But semantically and logically, they are two different beasts.

I think here are the challenges that we are facing right now:

• There's no easy way to pass default value when parameters are missing, I think that's why you implemented TriggerInstance class while I choose regex
• Even if we can resolve the first problem, we would still have to pass the right type of default values, otherwise, error like this execute: Invalid operands to operator >, bool and int. would still be raised

I agree with the first challenge.

I disagree with the second. I think that the motto With great power comes great responsibility applies here. We let the user to setup their own conditions in a script language, so in my book, they have to make sure that their use of the params is not prone to bugs.

Ironically, you forgot a potential solution in your list. One simple way to make sure the right type of default value is... to reimplement ConditionGroups. Let me explain: condition groups remove 95% of the need for expression conditions. You want either triggers to work? Simple, just create 2 condition groups. Want height > 1.5 and weight > 100, or age > 13 for your park ride state? Simple, create 2 condition groups. No need for ExpressionCondition.

Sure, ExpressionCondition could stay, but as an advanced feature. A seldom used advanced feature.

The reasoning behind not using Condition Groups in the project was the fact that it was easy to use trigger.cutscene_end or trigger.skip. I still think that triggers are a special beast that needs a special way to test.

We cannot have the cake and eat it too. I know you want to make gd-YAFSM dead simple to use, but expressions are an advanced feature. Either we reorganise the way conditions work to reduce the need to use the advanced feature, or we embrace the fact that expressions are an advanced feature.

[imjp94]

Yes and no. Trigger are the culprit here. They are specifically distinct as the fact that they only exist for a cycle with a null value, which is special. Null should be replaced someday with a special Trigger type, to make sure that it does not conflict with a potential valid null param (which I see potential uses, like setting a reference as a param).

Internally, yes, they works differently, but I really don't see the point of presenting it as anything else, as from user's perspective, a trigger is much like a boolean that stays for 1 frame.

However, I do agree that triggers should be stored in its own dictionary separating from parameters, which should be easy enough that it wouldn't even cause any breaking changes.

I disagree with the second. I think that the motto With great power comes great responsibility applies here. We let the user to setup their own conditions in a script language, so in my book, they have to make sure that their use of the params is not prone to bugs.

But currently, there's no way to declare default values at all, the only workaround would be setting parameters in _ready which is really not desirable as it would makes debugging code way harder.

I disagree taking Expression class as a fully working scripting language like gdscript, the purpose of the API being exposed is to act as a tool to interpret math expression like what the inspector do. Otherwise, this feature would be called ScriptCondition that makes use of Script.

Ironically, you forgot a potential solution in your list. One simple way to make sure the right type of default value is... to reimplement ConditionGroups. Let me explain: condition groups remove 95% of the need for expression conditions. You want either triggers to work? Simple, just create 2 condition groups. Want height > 1.5 and weight > 100, or age > 13 for your park ride state? Simple, create 2 condition groups. No need for ExpressionCondition.

Well, this is how I see those solutions:

• Basic: Primitive conditions
• Slightly Advance: ConditionGroup
• Advance: ExpressionCondition

The thing with those advance condition(ConditionGroup or ExpressionCondition) is that, they are not necessary. Even for now, we can already make complex condition the only drawback is that writing more code, for example, speed > 10 and speed < 20 can be achieved with a speed_range boolean.

That's why I would rather spend more time to figure out a more flexible solution like ExpressionCondition.
For a repository that is maintained by one man, deduction is always better than addition, it would be much easier to maintain 1 feature than 2 features with similar functionalities.
Plus, ConditionGroup always requires ExpressionCondition to achieve more complex condition while ExpressionCondition can just work on its own.

Btw, I think we should keep the discussion related to how we gonna make ExpressionCondition works.