From ac85e444fd797fb1d3e29a04b37473739209e84a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davy=20H=C3=A9lard?= Date: Tue, 25 Jun 2024 13:14:42 +0200 Subject: [PATCH 1/4] Add a test. --- ...ceneAsyncCodeGenerationIntegrationTests.js | 82 ++++++++++++++++++- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/GDevelop.js/__tests__/GDJSSceneAsyncCodeGenerationIntegrationTests.js b/GDevelop.js/__tests__/GDJSSceneAsyncCodeGenerationIntegrationTests.js index aad6191bdd5b..26dc3cd54882 100644 --- a/GDevelop.js/__tests__/GDJSSceneAsyncCodeGenerationIntegrationTests.js +++ b/GDevelop.js/__tests__/GDJSSceneAsyncCodeGenerationIntegrationTests.js @@ -23,6 +23,15 @@ describe('libGD.js - GDJS Async Code Generation integration tests', function () }); const generateAndRunEventsForLayout = (events, logCode = false) => { + const { runtimeScene, runCompiledEvents } = generateEventsForLayout( + events, + (logCode = false) + ); + runCompiledEvents(); + return runtimeScene; + }; + + const generateEventsForLayout = (events, logCode = false) => { const serializedProjectElement = new gd.SerializerElement(); project.serializeTo(serializedProjectElement); @@ -46,8 +55,10 @@ describe('libGD.js - GDJS Async Code Generation integration tests', function () serializedProjectElement.delete(); serializedSceneElement.delete(); - runCompiledEvents(gdjs, runtimeScene, []); - return runtimeScene; + return { + runtimeScene, + runCompiledEvents: () => runCompiledEvents(gdjs, runtimeScene, []), + }; }; describe('Basics', () => { @@ -364,6 +375,73 @@ describe('libGD.js - GDJS Async Code Generation integration tests', function () ).toBe(2 + 5); }); + it('can execute async events without side effect on local variables of the scene', function () { + // Reproduce a bug where the async events were not clearing + // the local variable stack. + // Local variables declarations were added over a not empty stack + // whereas actions, conditions and expressions were still using + // the expected stack index. + + // The following comments apply to the second run of events. + scene.getVariables().insertNew('SuccessVariable', 0).setValue(0); + const { runtimeScene, runCompiledEvents } = generateEventsForLayout([ + { + type: 'BuiltinCommonInstructions::Standard', + // Expected: Define local variables at stack index 0. + // Actual: Define local variables at stack index 1. + variables: [{ name: 'MyLocalVariable', type: 'number', value: 0 }], + conditions: [], + actions: [ + // Modify local variables at stack index 0. + { + type: { value: 'SetNumberVariable' }, + parameters: ['MyLocalVariable', '=', '456'], + }, + ], + }, + // Expected: Pop local variables at stack index 0. + // Actual: Pop local variables at stack index 1. + { + type: 'BuiltinCommonInstructions::Standard', + // Expected: Define local variables at stack index 0. + // Actual: Define local variables at stack index 1. + variables: [{ name: 'MyLocalVariable', type: 'number', value: 123 }], + conditions: [], + actions: [ + // Get local variables at stack index 0. + // Expected : The declaration value + // Actual : The value set by the previous event: 456 + { + type: { value: 'SetNumberVariable' }, + parameters: ['SuccessVariable', '=', 'MyLocalVariable'], + }, + // The only purpose of the wait is to trigger context switches. + { + type: { value: 'Wait' }, + parameters: ['1'], + }, + ], + }, + ]); + + // Run scene events a first time. + runCompiledEvents(); + expect( + runtimeScene.getVariables().get('SuccessVariable').getAsNumber() + ).toBe(123); + + // Process the tasks (after faking it's finished). + // The context switching happens here. + runtimeScene.getAsyncTasksManager().markAllFakeAsyncTasksAsFinished(); + runtimeScene.getAsyncTasksManager().processTasks(runtimeScene); + + // Run scene events a second time. + runCompiledEvents(); + expect( + runtimeScene.getVariables().get('SuccessVariable').getAsNumber() + ).toBe(123); + }); + it('generates an async fork that shares a scene variable a non-async sub-event', function () { scene.getVariables().insertNew('SuccessVariable', 0).setValue(0); scene.getVariables().insertNew('MySceneVariable', 0).setValue(1); From 5892a3abe4a4ab7480446b8ac292093334b7ea5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davy=20H=C3=A9lard?= Date: Tue, 25 Jun 2024 12:09:18 +0200 Subject: [PATCH 2/4] Fix local variables default values when the wait action is used --- .../Events/CodeGeneration/EventsCodeGenerator.cpp | 13 ++++++++----- GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp | 6 +++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/Core/GDCore/Events/CodeGeneration/EventsCodeGenerator.cpp b/Core/GDCore/Events/CodeGeneration/EventsCodeGenerator.cpp index c099b00fbeda..761f3a7947bc 100644 --- a/Core/GDCore/Events/CodeGeneration/EventsCodeGenerator.cpp +++ b/Core/GDCore/Events/CodeGeneration/EventsCodeGenerator.cpp @@ -709,11 +709,14 @@ EventsCodeGenerator::GenerateCallback( const gd::String actionsDeclarationsCode = GenerateObjectsDeclarationCode(callbackContext); - const gd::String callbackCode = - callbackFunctionName + " = function (" + - GenerateEventsParameters(callbackContext) + ") {\n" + - restoreLocalVariablesCode + - actionsDeclarationsCode + actionsCode + "}\n"; + const gd::String clearLocalVariablesCode = + GenerateLocalVariablesStackAccessor() + ".length = 0;\n"; + + const gd::String callbackCode = callbackFunctionName + " = function (" + + GenerateEventsParameters(callbackContext) + + ") {\n" + restoreLocalVariablesCode + + actionsDeclarationsCode + actionsCode + + clearLocalVariablesCode + "}\n"; AddCustomCodeOutsideMain(callbackCode); diff --git a/GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp b/GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp index 2306f7e74819..444fba332b9e 100644 --- a/GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp +++ b/GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp @@ -16,8 +16,7 @@ AsyncExtension::AsyncExtension() { gd::BuiltinExtensionsImplementer::ImplementsAsyncExtension(*this); GetAllEvents()["BuiltinAsync::Async"].SetCodeGenerator( - [](gd::BaseEvent &event_, - gd::EventsCodeGenerator &codeGenerator, + [](gd::BaseEvent &event_, gd::EventsCodeGenerator &codeGenerator, gd::EventsCodeGenerationContext &parentContext) { gd::AsyncEvent &event = dynamic_cast(event_); @@ -71,7 +70,8 @@ AsyncExtension::AsyncExtension() { } return "{\n" + parentAsyncObjectsListGetter + "{\n" + - asyncContextBuilder + asyncActionCode + "}\n" + "}\n"; + asyncContextBuilder + asyncActionCode + + "}\n" + "}\n"; }); GetAllActions()["BuiltinAsync::ResolveAsyncEventsFunction"].SetFunctionName( From 578c37f71db0d7a97518748eab08bc480a7f7dfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davy=20H=C3=A9lard?= Date: Tue, 25 Jun 2024 15:58:02 +0200 Subject: [PATCH 3/4] Review changes --- .../GDJSSceneAsyncCodeGenerationIntegrationTests.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/GDevelop.js/__tests__/GDJSSceneAsyncCodeGenerationIntegrationTests.js b/GDevelop.js/__tests__/GDJSSceneAsyncCodeGenerationIntegrationTests.js index 26dc3cd54882..28212aa131e5 100644 --- a/GDevelop.js/__tests__/GDJSSceneAsyncCodeGenerationIntegrationTests.js +++ b/GDevelop.js/__tests__/GDJSSceneAsyncCodeGenerationIntegrationTests.js @@ -25,7 +25,7 @@ describe('libGD.js - GDJS Async Code Generation integration tests', function () const generateAndRunEventsForLayout = (events, logCode = false) => { const { runtimeScene, runCompiledEvents } = generateEventsForLayout( events, - (logCode = false) + logCode ); runCompiledEvents(); return runtimeScene; @@ -376,7 +376,7 @@ describe('libGD.js - GDJS Async Code Generation integration tests', function () }); it('can execute async events without side effect on local variables of the scene', function () { - // Reproduce a bug where the async events were not clearing + // Try to reproduce a bug where the async events were not clearing // the local variable stack. // Local variables declarations were added over a not empty stack // whereas actions, conditions and expressions were still using @@ -435,6 +435,10 @@ describe('libGD.js - GDJS Async Code Generation integration tests', function () runtimeScene.getAsyncTasksManager().markAllFakeAsyncTasksAsFinished(); runtimeScene.getAsyncTasksManager().processTasks(runtimeScene); + // This test can't actually reproduce the issue because + // `runCompiledEvents()` instantiate `gdjs.SceneCode.localVariables` + // at every call. + // Run scene events a second time. runCompiledEvents(); expect( From 8cb17c48ba205e0ed814bfc170b55bd83dfbcb31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davy=20H=C3=A9lard?= Date: Tue, 25 Jun 2024 16:04:14 +0200 Subject: [PATCH 4/4] Revert format --- GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp b/GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp index 444fba332b9e..2306f7e74819 100644 --- a/GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp +++ b/GDJS/GDJS/Extensions/Builtin/AsyncExtension.cpp @@ -16,7 +16,8 @@ AsyncExtension::AsyncExtension() { gd::BuiltinExtensionsImplementer::ImplementsAsyncExtension(*this); GetAllEvents()["BuiltinAsync::Async"].SetCodeGenerator( - [](gd::BaseEvent &event_, gd::EventsCodeGenerator &codeGenerator, + [](gd::BaseEvent &event_, + gd::EventsCodeGenerator &codeGenerator, gd::EventsCodeGenerationContext &parentContext) { gd::AsyncEvent &event = dynamic_cast(event_); @@ -70,8 +71,7 @@ AsyncExtension::AsyncExtension() { } return "{\n" + parentAsyncObjectsListGetter + "{\n" + - asyncContextBuilder + asyncActionCode + - "}\n" + "}\n"; + asyncContextBuilder + asyncActionCode + "}\n" + "}\n"; }); GetAllActions()["BuiltinAsync::ResolveAsyncEventsFunction"].SetFunctionName(