GDevelop icon indicating copy to clipboard operation
GDevelop copied to clipboard

Variable expressions

Open arthuro555 opened this issue 2 years ago • 4 comments

Overview

An implementation of variable expressions, inspired by the following proposition: https://forum.gdevelop-app.com/t/reworking-variables-to-be-returnable-by-expressions/34593 - This discussion G´goes further than the current changes, and goes the explicit rather than the implicit route.

ToDo

  • [X] Update parser with new grammar
  • [X] Update Code Generation for processing the new grammar
  • [X] Add basic expressions (get scene variable, get global variable...)
  • [ ] Convert existing variable instructions/expressions to use the new variable field
  • [ ] IDE AutoCompletion of variable returning expressions in all parameter fields
  • [ ] IDE AutoCompletions of variable parameters
  • [ ] Proper tests

The changes

Those changes adds expression that returns variables. There are 4 added by the PR currently, SceneVar to get a scene variable, GlobalVar for global variables, Object.Var for object variables and FromJSON to get a variable from a JSON string. Those expressions can be used inside of number or string expressions, and the variable will be implicitly casted to the correct type (E.g. "Hello I am " + SceneVar(name) + " and I am " + ToString(SceneVar(age) + 1) == "Hello I am " + VariableString(name) + " and I am " + ToString(Variable(age) + 1)).

Those can also be used in a new variable parameter field that accepts only those expressions. There, the variable reference will be passed directly. This allows for anything that accepts a variable reference as parameter to accept one from any scope, unlike the previous variable parameters.

Implementation uncertainties

ImplicitVariableCastNode

Currently, the implementation adds the implicit casts as expression nodes around function calls that return a variable, which contains the expected type for it to be casted to. I am unsure if this is the way to go, since nodes usually contain a part of the written expression instead of being a "modifier" for their child nodes. This is the approach that made the most sense to me (especially in regards to the code generator) though.

Convertion of instructions using the old format

This change makes little sense if the instructions and expressions that accept variable references as parameter are not changed to use this new parameter type, but that would also be a breaking change. I have a few options in mind

  • Do not convert old instructions to the new format, write new ones in the new format
    • ✔️ Doesn't break older games
    • ❌ Makes little sense to me as it would make both the new and old format lack some instructions
    • ❌ Would cause tangles messes of using both methods, causing a lot of confusion
  • Make copies of all existing instructions in the new format with different names, make new instructions only in the new format
    • ✔️ Doesn't break older games
    • ❌ Forces people to memorize a lot of new expressions
      • Makes people don't want to adpot the new format
    • ❌ Creates a mess of duplicated instructions that do the same thing just a different syntax
  • Change all current instructions to use the new format
    • ✔️ No more dealing with this issue after everything and everyone transitionned at once
    • BREAKING CHANGE
      • Especially bad since we just got out of beta 😬
    • ❌ Doesn't allow for a smooth transition to the new format

arthuro555 avatar Oct 13 '21 12:10 arthuro555

I've not dig into this a lot, but a general remark before we go further: returning variables seems fine (and auto-converting them to the required data type too) but should we go further and remove entirely the Var()/SceneVar() expressions?

I'm asking because this is a lot of work, and for the user the difference between these two things: "Hello I am " + VariableString(name) + " and I am " + ToString(Variable(age) + 1) "Hello I am " + SceneVar(name) + " and I am " + ToString(SceneVar(age) + 1)

can be almost negligible. It's maybe a welcome improvement... but still it's fairly hard to read. What if we finally do the same as other "languages" by allowing variables to be used directly?

"Hello I am " + name + " and I am " + ToString(age + 1) (and for an object: Object.myVariable.myChild)

There is a risk of collision with objects (in which case we would favor objects, for backward compatibility) and functions (same I think). We would probably require variables to be declared on objects to support them (otherwise, you must go back to using a function to access them). We keep existing Variable/VariableString functions for compatibility (which are just getter/casts).

What do you think?

4ian avatar Oct 13 '21 18:10 4ian

🤔 While I do see the advantage, I think that this might cause problems with both

  1. The scope of variables - this syntax is just a name, so it does not carry the scope of variables. For global variables, we could maybe make a special global variable name that holds all variables globally (kind of like gklobalThis in js), though I am not a fan of this and I think it might be confusing to users (something that we need to be careful about since many users already find the concept of variables hard to grasp when starting with GDevelop). It gets worse for objects though, as suddenly any object name is also the name of a structure scoped to the currently selected object, which would be I think pretty confusing to many.

  2. The naming of expressions in GDevelop - since variable names can clash with functions names with that syntax we will have to get very careful not to use any commonly used names as expression names, as any new expression would be a breaking change for users that have got a variable named like that new expression.

So I would personally prefer variable expressions as they carry scope semantically (Object.Var contains a variable of Object, SceneVar contains a variable from the scene, FromJSON doesn't store the variable and just gives out the variable from the json, etc) and separates the scope of variable names and functions/objects, preventing any breaking changes due to name clashes between new functions and variables.

But since we are talking about such a huge design decision for GDevelop, I think it would be more appropriate to ask the community what they prefer through some kind of poll?

arthuro555 avatar Oct 13 '21 18:10 arthuro555

So basically the options are:

1. Variables are scoped to either scene, global, or an object instance, and there is one function for each scope

Examples:

Set boolean of scene variable a to true Set boolean of global variable b to true Set x position to Variable(x) Set text to GlobalVariableString(text) Set text to Object1.VariableString(my.text["is cool"])

Pros:

  • It is what we currently have, and not changing it means no need to change any game to use another system

Cons:

  • Limited to a few variable scopes
  • Need to be casted explicitly to be used in expressions (Variable, VariableString...)
  • Causes a lot of duplication, which makes it harder for devs to add features, more work for translators, and more instructions bloating the instruction selector

2. Opposite possibility: fully implicit variables. You just write variable names, and they will automatically be casted from the correct type. Special scene variables contain the other scopes

Examples:

Set boolean of variable a to true Set boolean of variable global.b to true Set x position to x Set text to global.text Set text to Object1.my.text["is cool"]

Pros:

  • Minimal amount of typing for better productivity
  • Many programming languages function like that, making it more intuitive to programmers or people that messed with programming languages before GDevelop
  • Removes the need for any explicit cast
  • Eliminates duplicated function

Cons:

  • It breaks extremely easily. If GDevelop adds a new expression, and it is named the same as your variable, the variable will be treated as a function and your game will cease to function. If you add a new object and name it as one of your scene structures, it will suddenly become a variable if that object, breaking many things in your game, etc. And those kinds of issues would be very hard to debug.
  • It can only have variables in a few specific scopes
  • "special variables" like the global structure or structures named the same as an object can be very confusing to many

And finally my in-between solution: variable expressions. Variable scope is defined by the expression it comes from. In expressions, they are implicitly casted.

Examples:

Set boolean of variable SceneVar(a) to true Set boolean of variable GlobalVar(b) to true Set x position to SceneVar(x) Set text to GlobalVar(text) Set text to Object1.Var(my).text["is cool"]

(The snytax for structures can be either SceneVar(MyStructure.child) or SceneVar(MyStructure).child)

Pros:

  • Eliminates duplicate functions by moving the declaration of the scope to the expression instead of the instruction
  • Allows variables to come from outside any scope (would allow for example a FromJSON("{\"my\": \"JSON\"}") expression that returns a variable, just like SceneVar, or to return multiple values from one expression by returning a )
  • Allows writing less for getting the value of a variable in expressions
  • Allows keeping the names from variables in a separate lexical scope than the functions and object names

Cons:

  • Not a big difference expression lengthwise when writing expressions
  • Makes more to write for non-expression variable fields than before

Personally, I'd go with the last one 👀

arthuro555 avatar Nov 02 '21 22:11 arthuro555

Ok so I've been thinking about this and I want to be sure about the plan before continuing:

  1. Finish this, to allow having a generic variable field that accepts a variable expression. We probably want to keep stuff like SceneVar for undeclared variables, to allow using undeclared variables like before.

  2. Follow-up PR, allow using a variable identifier instead of a variable expression for declared variables. Scene variables > Global variables > Free functions, Object variables > Object functions. If a variable declaration overshadows a function, show it in diagnostic reports.

Is that right? If so, I'll try to work on this again after I'm done with async actions.


Other follow-up changes I'd like to propose:

  • Adding a concept of declaration of a variable for a scope, kind of let in javascript

    • This allows for not having temporary variables be implicitly "global" to the whole scene event sheet, which is cleaner code
    • This would make usage of variables in extensions more convenient, not always needing a prefix
    • This would allow for making the for each event not override a scene variable, but use a scoped one, allowing for not leaking the variable outside of the scope it is used and for more intuitive behavior when using asynchronous actions (e.g. JavaScript's for(var ... vs for(let ...)
  • Adding a structure & array literal

    • I am a real fan of the JavaScript syntax for those, I'd try to do something similar
  • Adding "Copy variable to another" action

    • It is already implemented as gdjs.Variable.copy
    • It wasn't implemented because supporting all scopes would have duplicated it too much
    • It is a quite common thing to do, and right now users are relying on passing the variable through ToJSON expression & ParseJSON action to deep-copy it 😱

arthuro555 avatar Feb 02 '22 21:02 arthuro555