Add asynchronous events & Wait action

[arthuro555]

Closes #2975

Aims at adding asynchronous event support to GDevelop.

TODO (polish):

• Write tests for code generation
• Write tests for the async task manager
• Add an instruction that uses PromiseTask to ensure it works Unit tests show that it works
• Add documentation on making an asynchronous action
• Decide about: the design of LongLivedObjectsList (is it the best data structure to use/can we do better?)
• Decide about: could avoid creating and splitting events into AsyncEvent?
• Fix the test with object groups
• Avoid replacing a parameter when assigning asyncObjectsLists
• Add test case about TaskGroup code generation (object async action)
• Decide about: how things should work when an async action is used on an object
• Check the code generation works with the hot reloading (stable unique identifiers, and the hot reloader should probably replace the callbacks of the async tasks that are waiting :))
  • Will be done in another PR since it is a bit complex Nevermind, it was extremely easy after all 😅
• Revert unwanted changes done by the autoformatter
• FINAL REVIEW 🎉

DONE:

• Add wait action
• Add a way to define instruction as asynchronous
• Add basic code generation
• Make the generated code functional
  • Backup/restore object lists
  • Ensure preprocessing works on all events
  • Test all edge cases
• Ensure support of events functions
  • Find out why it randomly turns asynv on its own
• Pass runtimeScene to the callback without leaking runtimeScene
• Write wiki page
• Add visual indicator to asynchronous actions

This PR uses a kind-of workaround to make this work: While preprocessing the events, iterate over actions, and if an asynchronous one is found, move them all with the subevents to an "asynchronous" subevent. This is because otherwise, it is difficult to generate the asynchronous callback function. The part that would do the generation of that function would be gd::EventsCodeGenerator::GenerateActionsListCode. That function doesn't have access to subevents though, it is the responsibility of an event's code generator to mix actions, conditions, and subevents code generation as it likes. Splitting that code generation between both is hard and error prone. By moving affected actions and subevents together to an internal subevent, we can do a custom mix of actions and subevents, allowing us to have one function responsible for all code generation of those subevents and actions again, allowing us to have a less error-prone and concise code generation.

Of course, this isn't without issues:
First, while it works like a charm right now, it uses the assumption that moving subevents and actions to a single subevent will not change the behavior of the event. That assumption might not child true for potential future events, for example, if we are to add an "if/else" event, moving actions from the if branch to the subevents of the else branch may break the event completely.
The preprocessing, although I have not benchmarked it, must be adding quite an overhead on code generation.
This can be confusing to contributors since they might not understand why this event is not behaving or being used the same as the other ones or may be confused as to how half of the code works if they are unaware of the other half (preprocessing and code-generation).

Overall though, I think this should be mostly fine, let's get this PR done before assuring it will fully work though! 😁

[4ian]

Nice start, thanks for giving this a try :)

This PR uses a kind-of workaround to make this work

I think that's fine for now. We might want to maybe avoid this to allow things like reporting errors in the future with a link to the "original event" (i.e: mark events having errors in a Diagnostic Manager, and allow the events to display these errors visually in the events sheet, which means the original event pointer/address must be stored).

But to begin we can do with your workaround and introduce something later in the code generation if we don't want this anymore :)

My main concern for now is that the wait (or any asynchronous action) is thened directly. This means that the "lifecycle" of the promise resolution is directly handled by the JavaScript engine. Notably, it means it can run at "any time", which can cause issue because 1) we might want to run all the "resolved" promises before the main events and 2) worse, it's not cancelable/pausable and could run outside of the scene (like if the scene is destroyed, it could still run) (or if multiple scenes are paused, their promises will run).

This is why I think I discussed in another topic the fact that these promises should be stored somewhere inside the game engine (probably the runtime scene?).
This part would have for responsibility to store these promise, and at each frame before the events, run the continuation of any promise. This means surely a slightly different approach, with an API like this (warning, pseudo code):

// In the code generated for the action:
const pickedObjectsMap = ...; // Construct a map of all picked objects (to do at code generation I guess)
runtimeScene.waitForPromise(wait(3000), pickedObjectsMap, eventsListXxx); // eventsListXxx is the function to run after.

// The function
waitForPromise(promise, pickedObjectsMap, continuation) {
  const waitedPromise = {
    // TODO: handle errors better I guess.
    promise: promise.then(() => waitedPromies.settled = true, () => waitedPromies.settled = true), 
    continuation, 
    settled: false, 
    pickedObjectsMap,
  }; 
  this._waitedPromises.push(waitedPromise);
}

// In the runtime scene, before events:
for(const waitedPromise of this._waitedPromises) {
  if (waitedPromise.settled) continuation(this, waitedPromise.pickedObjectsMap); // TODO: not sure how to "restore" objects
}

By doing this, it means that the runtime scene can choose when to run the "continuation" events. So it should flow naturally and never run outside of the scene. It's predictable :)

I'm not sure exactly how to handle objects, but pretty sure that when deleting objects, we'll need to go through the pickedObjectsMap of all this._waitedPromises to remove the object just deleted :)

[arthuro555]

Yep, that sounds like a good idea. Only a little objection: I do not think it should be part of the main events loop. One of the main advantages of javascript promises is that it doesn't run anytime, it runs when there is no other code running (so it won't run in the middle of rendering, events code, or something, but always between two game frames), and it runs ASAP, where our game event loop, while called often, isn't called nearly as often as necessary for example for making a game server (you wouldn't believe the performance difference between a P2P "server" I made in GDevelop then in JavaScript. I didn't actually benchmark it, but one was fluid while the other was unplayable). If we execute asynchronous callbacks only before scene events, it doesn't really change anything for the user since both happen at the same time in the events loop, except that waiting for the beginning of the next frame makes the processing slightly more delayed for one option.

[arthuro555]

I'm not sure exactly how to handle objects, but pretty sure that when deleting objects, we'll need to go through the pickedObjectsMap of all this._waitedPromises to remove the object just deleted :)

Also, I think I've found an IMO better solution, see LongLivedObjectList.

It seems to work well, I've found no issues after playing for a while with a few configurations of objects and groups.

[4ian]

Also, I think I've found an IMO better solution, see LongLivedObjectList.

Very interesting idea! At least thinking about it I think it should work in theory.

Remember that objects like sprites can and will be recycled when there number is high enough. You should ensure there are no callbacks surviving a recycling.

and it runs ASAP, where our game event loop, while called often, isn't called nearly as often as necessary for example for making a game server (you wouldn't believe the performance difference between a P2P "server" I made in GDevelop then in JavaScript. I didn't actually benchmark it, but one was fluid while the other was unplayable).

I think the difference is in what the game loop pattern vs a game server is:

• A game loop is run "only" 60 times per second, with the goal of going as quickly as possible. Predictability might be an important factor, for example to allow to record/replay sessions. But indeed if a promise is solved at the beginning of the first frame, it might have to wait for the next frame, so ~16ms before starting to be even considered. Note that this is usually a good thing, to keep a stable framerate/avoid stuttering in gameplay.
• A game server is all about latency, and in this case every millisecond is important, so you want your server to execute asap the promises (and as quickly as possible, and in a multithreaded way if possible).

But because these are two different use cases, I'm not sure I would discard the pattern of "storing promises then execute the continuation at the next game loop". Our game engine is single threaded for the moment, so it should actually not make a difference for players?

For game servers (well, this is not officially supported), in this case I would recommend that we enable a different "mode" ("game server" mode) that would switch to another implementation, following this time your pattern "execute promise asap without storing them for the next frame" (because every milliseconds count).
That's maybe a larger solution but I think it's not the only thing that would need adaptations to run as an efficient game server.

[4ian]

Note that you can feel free to continue experimenting without the "stored waiting promises" for now if you don't believe in it, but I still think we'll need this abstraction for the usual "mode" of running things in the game loop.

It's not actually far from the reactor pattern/event loop integrated into Node.js/a browser :) It's just that it's tied to the game loop so you're paying extra latency of each frame (especially if the main logic is almost inexistant and so everything is running in short lived promises).
So while it's not a good choice for a game server, it should be fine for rendering a game (and if one day we somehow manage to make a multithreaded game engine, this will be the place where we orchestrate which job is going on which "thread").

[arthuro555]

Self-review time 😎
I think generally we should recheck all variables and functions name as I think most are misleading either objectively or to everyone that isn't me.
It's going at a good pace so far, I am pretty confident that this will end up working fine :)

[arthuro555]

Nice, it now works across all events types! Except... The for each variable.
That one has kind of a problem since the engine has no concept of block-scoped/declared variables, so I can't really add "closures" for the good value to be kept, so the asynchronous callback will just get the current value of the variable and not the one of the current iterations of the for each event...

Personally, I think that to be really fixed we need to add block-scoped variables/ declaration of variables for a block, which are kind of out of scope. Would it be acceptable to leave this broken for now, add big warnings on the wiki, and fix it in a later PR, since it would complexify this one too much to do it now?

[arthuro555]

That's... Really weird. The wait function seems to work fine in events function, but the events function itself somehow gets (inconsistently, but most of the time) generated as if it was an asynchronous function??? (Which crashes the game since it doesn't return anything to .then)

[arthuro555]

I'm marking this ready for review since it is mostly working now, but the work is not finished

[4ian]

Went through the code. Overall I think it make sense! I'll still be thinking about alternate or slightly different approaches to see if we can simplify some things.
Questions about:

• As discussed I'd like to get the then removed from code generation instead call a runtimeScene.waitForPromise or similar method. In some implementation it could be a then, but in the classic game engine we'll probably have the waited promise callbacks run before events.
• How is this working in extension and, probably more hairy to think about, custom behavior methods? :)

[arthuro555]

For game servers (well, this is not officially supported), in this case I would recommend that we enable a different "mode"

The problem with that is that the only integrated (not counting user extensions) realtime networking option is P2P, and what most users mess with when trying out multiplayer (as far as I see from my interactions with the community at least). When connecting the games together like this directly though, they are all like a "game server", and that is also what i meant by that. If all games take many milliseconds before beginning to process the messages of each other, the game simply becomes unplayable, and people will criticise GDevelops networking for being way too slow.

[4ian]

I found a bug thanks to a test: LongLivedObjectsList is indexing objects using their names, but their names are just their "original names" while in a function, they are referred to by another name (the parameter name).
The LongLivedObjectsList are constructed using the wrong names for the objects (they are constructed with their original names, instead of the names used to refer to them) and can't be found when the callback is called.

See the changes made here (the first test changed, the one which is the simplest, is also failing): 446e94e

Note that deletion of objects was working well (before I hit this bug).

[4ian]

I think I fixed the issue by passing the "object name", as known during code generation.
I'll add another test with groups too.

@arthuro555 Is it possible for you to commit some test games you've been using, in GDJS/tests/games? Will be useful to have one or two games in addition to the automated tests.

[4ian]

@arthuro555 This seems dangerous, you assign the parameter asyncObjectsList to a new version made from itself. What happens if this is called multiple times?
You will create an asyncObjectsList from asyncObjectsList, itself from asyncObjectsList.

[arthuro555]

Async events guarantee that, after one, the current function ends, since the code of any actions/subevents afterward is put in a callback function after the asynchronous task has finished executing. So this piece of code cannot be twice in the same function. Agreed, ideally, we'd use a separate variable (const newAsyncObjectsLists) but that would add more complexity for generating the arguments of callback functions since they would not match up between the calling end and the invoked end. The definition of the variable is as an argument of the function, setting it only changes the value for the current function, which has reached its end anyway, making it fully harmless. Therefore I judged this as not important enough to be worth increasing the complexity of the arguments code generation.

[4ian]

I was concerned about being in an async callback with two subevents having both two async actions.

I think this works because the two subevents will be generated in two different functions, but that's a detail that may not hold in the future.
I think it's safer to do const parentAsyncObjectsLists = asyncObjectsLists; const asyncObjectsLists = gdjs.LongLivedObjectsList.from(parentAsyncObjectsList)

[4ian]

Actually the trick of declaring a parentXxx variable does not work well because the shadowing of variable does not work as expected ( Cannot access 'asyncObjectsLists' before initialization) :/
Might still be worth avoiding modifying the parameter so that we can in the future not be afraid of putting sub events in functions or not.

[arthuro555]

That'd be a syntax error, one cannot declare a variable using the name of an argument:

The event code generation with one function per event is currently not just a detail in my opinion, but a pretty important implementation choice for GDJS. If we were to change it, though I don't see why we would do it in the foreseeable future for GDJS, I'm pretty sure other changes would be needed anyways. We could do such a change then, which would be trivial with scoped blocks and no arguments preventing the "overshadowing".

[4ian]

I have another test that fails because somehow it generates in the final callback, when refering to a group ("MyGroup") that contains "MyParamObject1" and "MyParamObject2":

    gdjs.copyArray(asyncObjectsList.getObjects("MyParamObject1"), functionNamespace.GDMyParamObject1Objects3);
    gdjs.copyArray(eventsFunctionContext.getObjects("MyParamObject2"), functionNamespace.GDMyParamObject2Objects3);

instead of

    gdjs.copyArray(asyncObjectsList.getObjects("MyParamObject1"), functionNamespace.GDMyParamObject1Objects3);
    gdjs.copyArray(asyncObjectsList.getObjects("MyParamObject2"), functionNamespace.GDMyParamObject2Objects3);

As if the context forgot that MyParamObject2 was already referred to before. I'll take a look but this looks suspicious.

[4ian]

@arthuro555 this line seems weird to me. The nearest parent of an async callback is... itself?

Shouldn't this be either the parent, if async, or the nearest async parent of the parent (which can be null)?

Note that if we change this, we probably need to change the InheritsFrom too and the way we iterate on the for loops over EventsCodeGenerationContext (to start at this, rather than at the parent... I guess).

I'm asking because I think this is creating an issue where:

• we want to check if an object is declared in the parent.
• but because we check the current context nearest async parent, which is itself, the object is set to be declared.

[arthuro555]

I think this is normal, since it is the simplest way to have a parent tell all its children that it is the new nearest context as that pointer is inherited from a parent. This is used in two places: ObjectsListNeeded, in which we do want this context to be taken into account, as otherwise, the current async action would not know to pass down objects lists to the actions directly following it.

In ShouldUseAsyncObjectsLists though, it looks like that is an issue, yes. If an object has been used in the previous actions of the current context, it doesn't mean that we should suddenly start using async lists.

What I propose is that instead of changing this line, which is correct and the most simple (as in performant) for most uses, I propose that instead, we change the for loop in ShouldUseAsyncObjectsLists to start at nearestAsyncParent->parent->nearestAsyncParent instead of nearestAsyncParent.

[arthuro555]

Just made a commit for that, please tell me if I missed something I have not touched the code for about a month and might remember the full context around it incorrectly.

[4ian]

Note for me: check if should be also checking what is being declared by the context (and refactor the contexts so that things are not set as declared anymore, this mutation is making things more complex to reason about)

[arthuro555]

As if the context forgot that MyParamObject2 was already referred to before. I'll take a look but this looks suspicious.

I found the issue: it seems that the documentation of ObjectAlreadyDeclared is lying:

  /**
   * Return true if an object list has already been declared (or is going to be
   * declared).
   */
  bool ObjectAlreadyDeclared(const gd::String& objectName) const

It seems that last part is not true, because if I replace ObjectAlreadyDeclared with IsToBeDeclared in ShouldUseAsyncObjectsLists, the test now passes 🤔

The description is true only after calling the code generation function for the context, which I guess might not have been yet is some cases.

[arthuro555]

I modified the code to also check for IsToBeDeclared if ObjectAlreadyDeclared was false, and the failing tests now pass 👍

[4ian]

I got to the same conclusion! I started to rename this to ObjectAlreadyDeclaredInParents.
Thinking more, I'm also thinking if we should rework how async contexts are handled:

• For now, all objects declared in the parents are not transfered to the async context. It's "fine" because when we need a new object list, we get it thanks to GenerateAllInstancesGetterCode that calls ShouldUseAsyncObjectsLists. This is a bit "false" though, because the object might be already declared in the parent, so calling GenerateAllInstancesGetterCode is "false": we're not getting all instances.
• Instead, we could not do any change in GenerateAllInstancesGetterCode, but do the check in declareObjectList for ShouldUseAsyncObjectsLists. The async context should use the LongLivedObjectsLists whenever it must get something already declared from an non async parent.

  • This implies to rework the async contexts so that they inherits the list of alreadyDeclaredObjectsLists from the parent.

  • This implies to rework ShouldUseAsyncObjectsLists so that it does a different job, maybe remove it entirely and replace it by a function that tells if if the object list can be gotten from the parent as usual (i.e, it will run:

    gd::String copiedListName =
        GetObjectListName(object, *context.GetParentContext());
    return "gdjs.copyArray(" + copiedListName + ", " + objectListName + ");\n";
    

    ) or if it must be gotten from the async objects lists.

This would align the reasoning I think between async/non async and make the "declared objects in the parents" consistent?

[4ian]

FYI, I've added some more tests. Some may fail, I just tested on another branch with a slightly different approach - don't lose time fixing these :)

[4ian]

I've pushed another round of tests. I have another branch based on this where I've done a few changes on all object lists are declared. I'll make a PR soon and we'll see if there are things to merge/how to proceed :)

[4ian]

@arthuro555 Any idea how we could create a test case that would have failed on the asyncContext - ObjectWillBeDeclared(objectName)? This would be useful for my branch too

[arthuro555]

I think that should be used when you have such an event:

Condition:
  <none>
Action:
   Wait 0.1s
Subevent:
   Condition: 
      Variable a of object A is 1
   Actions:
      Wait 0.1s
      Change variable a of object A to 2

With a scene that has two object A, one with the variable a being 1 and the other 0.

With the previous typo, that last action would have been generated getting the objects from the normal object source (the scene or function context) instead of the saved object list, so we'd need to make the test ensure that after that code, one object has still the value 0, and the other has now the value 2.

Which is odd, since we should already be testing for that 🤔

[4ian]

Closing as merged in #3852! Wow! 🥳