From 98cb0d60bcdbd9f57bd46390a22141593422bd49 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 6 May 2026 18:15:29 +0000 Subject: [PATCH 01/12] Fix Or object picking and add distributive Or variant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing "Or" condition unconditionally overwrote the parent event's picked object lists with an empty "final" list whenever no true branch contributed picks for an object. This erased upstream picks (the AllowedArea pair pattern) and produced surprising 0-picked results. Two changes: - Or (existing): track per-object whether at least one true branch actually referenced the object. If none did, leave the parent's picked list untouched at the end of the Or instead of overwriting it. This preserves Door/Coin semantics (the action acts only on the specific object whose state was tested) and fixes the AllowedArea pair pattern where outside-Or picks were being wiped. - OrDistributive (new): a sibling condition with "independent object picking" semantics. A branch that does not reference an object behaves as if it picked the parent's full list for that object. Suited to patterns like "text input submitted OR submit button clicked → read the text input", where the action acts on objects that are unrelated to the branch that fired. Each operator has a different correct use case; both are kept because neither subsumes the other (Door/Coin breaks under distributive semantics; Input/Button breaks under preserve-picks semantics). Adds a regression test file covering the canonical patterns for both operators (Door/Coin, AllowedArea preservation, all-false preservation, Input + SubmitButton, score-on-trigger, and a sanity test documenting that distributive Or mis-fits Door/Coin). --- .../Builtin/CommonInstructionsExtension.cpp | 29 +- .../Builtin/CommonInstructionsExtension.cpp | 192 ++++++++- ...ctPickingCodeGenerationIntegrationTests.js | 383 ++++++++++++++++++ 3 files changed, 585 insertions(+), 19 deletions(-) create mode 100644 GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js diff --git a/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp b/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp index 300e20b8b09f..ae60f231375d 100644 --- a/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp +++ b/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp @@ -67,7 +67,10 @@ BuiltinExtensionsImplementer::ImplementsCommonInstructionsExtension( _("Checks if at least one sub-condition is true. If no " "sub-condition is specified, it will always be false. " "This is rarely used — multiple events and sub-events are " - "usually a better approach."), + "usually a better approach.\n\n" + "Picked objects: each branch contributes the objects it " + "actually picked. Objects that no true branch referenced are " + "left as they were before the Or."), _("If one of these conditions is true:"), "", "res/conditions/or24_black.png", @@ -75,6 +78,30 @@ BuiltinExtensionsImplementer::ImplementsCommonInstructionsExtension( .SetCanHaveSubInstructions() .MarkAsAdvanced(); + extension + .AddCondition( + "OrDistributive", + _("Or (independent object picking)"), + _("Checks if at least one sub-condition is true, while keeping " + "object picking independent across branches. A branch that " + "does not constrain a given object behaves as if all instances " + "of that object were picked for that branch (instead of " + "contributing nothing).\n\n" + "Use this when the action acts on objects that are unrelated " + "to the branch that fired (for example: \"text input was " + "submitted OR submit button was clicked\" → read the text " + "input). Do not use this when the action should act only on " + "the specific object whose state was tested in the branch " + "that fired (for example: \"door collided OR coin collided\" " + "→ hide the touched object) — use the regular Or for that."), + _("If one of these conditions is true (independent object " + "picking):"), + "", + "res/conditions/or24_black.png", + "res/conditions/or_black.png") + .SetCanHaveSubInstructions() + .MarkAsAdvanced(); + extension .AddCondition( "And", diff --git a/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp b/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp index 2a657380bed5..889166ce4e26 100644 --- a/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp +++ b/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp @@ -253,6 +253,24 @@ CommonInstructionsExtension::CommonInstructionsExtension() { gd::String conditionsCode; gd::InstructionsList &conditions = instruction.GetSubInstructions(); + gd::String codeNamespace = codeGenerator.GetCodeNamespaceAccessor(); + auto finalListName = [&](const gd::String &objectName) { + return codeNamespace + ManObjListName(objectName) + + gd::String::From(parentContext.GetContextDepth()) + "_" + + gd::String::From(parentContext.GetCurrentConditionDepth()) + + "final"; + }; + // Per-object runtime flag: was there at least one branch that was + // true AND that referenced (declared) this object? If not, we leave + // the parent's picked list untouched at the end of the Or, instead + // of overwriting it with an empty "final" list. + auto hasContribName = [&](const gd::String &objectName) { + return codeNamespace + ManObjListName(objectName) + + gd::String::From(parentContext.GetContextDepth()) + "_" + + gd::String::From(parentContext.GetCurrentConditionDepth()) + + "hasContrib"; + }; + // The Or "return" true by setting the upper boolean to true. // So, it needs to be initialized to false. conditionsCode += codeGenerator.GenerateUpperScopeBooleanFullName( @@ -298,11 +316,10 @@ CommonInstructionsExtension::CommonInstructionsExtension() { it != objectsListsToBeDeclared.end(); ++it) { emptyListsNeeded.insert(*it); gd::String objList = codeGenerator.GetObjectListName(*it, context); - gd::String finalObjList = - codeGenerator.GetCodeNamespaceAccessor() + ManObjListName(*it) + - gd::String::From(parentContext.GetContextDepth()) + "_" + - gd::String::From(parentContext.GetCurrentConditionDepth()) + - "final"; + gd::String finalObjList = finalListName(*it); + // Mark that this object got a contribution from a true branch + // that referenced it. + conditionsCode += " " + hasContribName(*it) + " = true;\n"; conditionsCode += " for (let j = 0, jLen = " + objList + ".length; j < jLen ; ++j) {\n"; conditionsCode += " if ( " + finalObjList + ".indexOf(" + @@ -319,7 +336,6 @@ CommonInstructionsExtension::CommonInstructionsExtension() { gd::String declarationsCode; // Declarations code - gd::String codeNamespace = codeGenerator.GetCodeNamespaceAccessor(); for (set::iterator it = emptyListsNeeded.begin(); it != emptyListsNeeded.end(); ++it) { //"OR" condition must declare objects list, but without getting @@ -330,13 +346,12 @@ CommonInstructionsExtension::CommonInstructionsExtension() { // be filled with objects by conditions, but they will have no // incidence on further conditions, as conditions use "normal" // ones. - gd::String finalObjList = - codeNamespace + ManObjListName(*it) + - gd::String::From(parentContext.GetContextDepth()) + "_" + - gd::String::From(parentContext.GetCurrentConditionDepth()) + - "final"; + gd::String finalObjList = finalListName(*it); codeGenerator.AddGlobalDeclaration(finalObjList + " = [];\n"); declarationsCode += finalObjList + ".length = 0;\n"; + // Per-object contribution flag, reset at the start of every Or. + codeGenerator.AddGlobalDeclaration(hasContribName(*it) + " = false;\n"); + declarationsCode += hasContribName(*it) + " = false;\n"; } declarationsCode += "let " + codeGenerator.GenerateBooleanFullName( @@ -349,16 +364,16 @@ CommonInstructionsExtension::CommonInstructionsExtension() { code += conditionsCode; // When condition is finished, "final" objects lists become the - // "normal" ones. + // "normal" ones — but only for objects that received at least one + // contribution (a true branch that actually referenced the object). + // If no true branch contributed, leave the parent's picked list + // untouched so picks established before the Or are preserved. code += "{\n"; for (set::iterator it = emptyListsNeeded.begin(); it != emptyListsNeeded.end(); ++it) { - gd::String finalObjList = - codeNamespace + ManObjListName(*it) + - gd::String::From(parentContext.GetContextDepth()) + "_" + - gd::String::From(parentContext.GetCurrentConditionDepth()) + - "final"; - code += "gdjs.copyArray(" + finalObjList + ", " + + gd::String finalObjList = finalListName(*it); + code += "if (" + hasContribName(*it) + ") gdjs.copyArray(" + + finalObjList + ", " + codeGenerator.GetObjectListName(*it, parentContext) + ");\n"; } code += "}\n"; @@ -366,6 +381,147 @@ CommonInstructionsExtension::CommonInstructionsExtension() { return code; }); + GetAllConditions()["BuiltinCommonInstructions::OrDistributive"] + .SetCustomCodeGenerator( + [](gd::Instruction &instruction, + gd::EventsCodeGenerator &codeGenerator, + gd::EventsCodeGenerationContext &parentContext) { + gd::InstructionsList &conditions = + instruction.GetSubInstructions(); + gd::String codeNamespace = + codeGenerator.GetCodeNamespaceAccessor(); + + auto finalListName = [&](const gd::String &objectName) { + return codeNamespace + ManObjListName(objectName) + + gd::String::From(parentContext.GetContextDepth()) + "_" + + gd::String::From( + parentContext.GetCurrentConditionDepth()) + + "distFinal"; + }; + + // PASS 1: Discover every object referenced across all branches + // by generating each branch's condition code into a discovery + // context. The output is discarded; we only keep the union of + // referenced objects, which is needed before we can emit the + // real per-branch code with the correct pre-registration. + std::set allReferencedObjects; + for (unsigned int cId = 0; cId < conditions.size(); ++cId) { + gd::EventsCodeGenerationContext discoveryContext; + discoveryContext.InheritsFrom(parentContext); + discoveryContext.ForbidReuse(); + codeGenerator.GenerateConditionCode( + conditions[cId], "isConditionTrue", discoveryContext); + std::set branchObjects = + discoveryContext.GetAllObjectsToBeDeclared(); + allReferencedObjects.insert(branchObjects.begin(), + branchObjects.end()); + } + + // Distributive Or: every referenced object must be available in + // the parent's picked list, filled from the scene if not already + // picked by a previous condition. This is what allows a branch + // that doesn't reference an object to behave as "unconstrained" + // (i.e. contribute the parent's picked list rather than empty). + for (auto &objectName : allReferencedObjects) { + parentContext.ObjectsListNeeded(objectName); + } + + // PASS 2: Emit per-branch code. Each branch's child context is + // pre-registered with the entire union of referenced objects, + // so the branch's child holds a copy of the parent's picked + // list for every union object — even those the branch itself + // doesn't constrain. + gd::String conditionsCode; + conditionsCode += codeGenerator.GenerateUpperScopeBooleanFullName( + "isConditionTrue", parentContext) + + " = false;\n"; + + for (unsigned int cId = 0; cId < conditions.size(); ++cId) { + gd::EventsCodeGenerationContext context; + context.InheritsFrom(parentContext); + context.ForbidReuse(); + + // Pre-register every union object on this branch's child so + // GenerateObjectsDeclarationCode emits a copy from the parent + // even for objects the branch doesn't itself touch. + for (auto &objectName : allReferencedObjects) { + context.ObjectsListNeeded(objectName); + } + + gd::String conditionCode = codeGenerator.GenerateConditionCode( + conditions[cId], "isConditionTrue", context); + + conditionsCode += "{\n"; + conditionsCode += + codeGenerator.GenerateObjectsDeclarationCode(context); + if (!conditions[cId].GetType().empty()) + conditionsCode += conditionCode; + + conditionsCode += "if(" + + codeGenerator.GenerateBooleanFullName( + "isConditionTrue", context) + + ") {\n"; + conditionsCode += + " " + + codeGenerator.GenerateUpperScopeBooleanFullName( + "isConditionTrue", context) + + " = true;\n"; + for (auto &objectName : allReferencedObjects) { + gd::String objList = + codeGenerator.GetObjectListName(objectName, context); + gd::String finalObjList = finalListName(objectName); + conditionsCode += " for (let j = 0, jLen = " + objList + + ".length; j < jLen ; ++j) {\n"; + conditionsCode += " if ( " + finalObjList + + ".indexOf(" + objList + "[j]) === -1 )\n"; + conditionsCode += " " + finalObjList + ".push(" + + objList + "[j]);\n"; + conditionsCode += " }\n"; + } + conditionsCode += "}\n"; + conditionsCode += "}\n"; + } + + // Emit "final" lists declarations. + gd::String declarationsCode; + for (auto &objectName : allReferencedObjects) { + gd::String finalObjList = finalListName(objectName); + codeGenerator.AddGlobalDeclaration(finalObjList + " = [];\n"); + declarationsCode += finalObjList + ".length = 0;\n"; + } + declarationsCode += "let " + + codeGenerator.GenerateBooleanFullName( + "isConditionTrue", parentContext) + + " = false;\n"; + + gd::String code; + code += declarationsCode; + code += conditionsCode; + + // Final copy: distributive Or overwrites the parent's picked + // list with the union built up across branches when at least + // one branch was true. Each true branch contributes (either + // its own picks if it referenced the object, or the parent's + // list as it was at the start of the Or otherwise). When no + // branch was true the Or returns false; in that case we leave + // the parent's picks untouched so behavior is consistent with + // the regular Or in the all-false case. + code += "if (" + + codeGenerator.GenerateUpperScopeBooleanFullName( + "isConditionTrue", parentContext) + + ") {\n"; + for (auto &objectName : allReferencedObjects) { + gd::String finalObjList = finalListName(objectName); + code += "gdjs.copyArray(" + finalObjList + ", " + + codeGenerator.GetObjectListName(objectName, + parentContext) + + ");\n"; + } + code += "}\n"; + + return code; + }); + GetAllConditions()["BuiltinCommonInstructions::And"].SetCustomCodeGenerator( [](gd::Instruction &instruction, gd::EventsCodeGenerator &codeGenerator, gd::EventsCodeGenerationContext &parentContext) { diff --git a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js new file mode 100644 index 000000000000..fabfdca1c08c --- /dev/null +++ b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js @@ -0,0 +1,383 @@ +/** + * Regression tests for the object-picking semantics of the two "Or" sub-event + * conditions: + * + * - "BuiltinCommonInstructions::Or" (the existing Or, with a fix that stops + * overwriting the parent's picked list when no true branch contributed for a + * given object). Mental model: each branch contributes only the instances + * it actually picked; objects that no true branch referenced are left as + * they were before the Or. Use this when the action should act on the + * specific object whose state was tested in the branch that fired + * (Door/Coin pattern). + * + * - "BuiltinCommonInstructions::OrDistributive" (new condition). Mental + * model: a branch that does not constrain a given object behaves as if it + * contributed the parent's full picked list for that object. The picked + * list at the action is the union over all true branches. Use this when + * the action acts on objects unrelated to the branch that fired + * (TextInput + SubmitButton pattern). + * + * The canonical patterns covered below: + * 1. Door/Coin — preserve-picks Or only. + * 2. AllowedArea pair — preserve-picks Or only (bug-fix path). + * 3. Both branches false — both Or variants must leave parent picks alone. + * 4. Input + Submit — distributive Or only. + * 5. "Score on trigger" — distributive Or only. + * 6. Sanity — distributive Or breaks Door/Coin (documents the + * difference between the two operators). + */ + +const initializeGDevelopJs = require('../../Binaries/embuild/GDevelop.js/libGD.js'); +const { makeMinimalGDJSMock } = require('../TestUtils/GDJSMocks'); +const { + generateCompiledEventsFromSerializedEvents, +} = require('../TestUtils/CodeGenerationHelpers.js'); + +describe('libGD.js - GDJS Or object picking semantics integration tests', () => { + let gd = null; + beforeAll(async () => { + gd = await initializeGDevelopJs(); + }); + + /** + * Build a small project with two object parameters, run the supplied events, + * and return the resulting (mutated) parameter object lists plus the + * runtime scene so tests can assert on picked lists and scene variables. + * + * The events function is given two object parameters: ObjectA and ObjectB. + * Each test seeds these with a single instance whose 'MyVariable' is set + * to a value the test controls, so that picking conditions of the form + * `VarObjet(ObjectX, "MyVariable", "=", N)` deterministically pick or skip + * the instance. + */ + const runEventsWithTwoObjects = (events, { aValue, bValue }) => { + const serializerElement = gd.Serializer.fromJSObject(events); + const runCompiledEvents = generateCompiledEventsFromSerializedEvents( + gd, + serializerElement, + { + parameterTypes: { + ObjectA: 'object', + ObjectB: 'object', + }, + logCode: false, + } + ); + + const { gdjs, runtimeScene } = makeMinimalGDJSMock(); + + const a1 = runtimeScene.createObject('ObjectA'); + a1.getVariables().get('MyVariable').setNumber(aValue); + const objectAList = new gdjs.Hashtable(); + objectAList.put('ObjectA', [a1]); + + const b1 = runtimeScene.createObject('ObjectB'); + b1.getVariables().get('MyVariable').setNumber(bValue); + const objectBList = new gdjs.Hashtable(); + objectBList.put('ObjectB', [b1]); + + runCompiledEvents(gdjs, runtimeScene, [objectAList, objectBList]); + + return { runtimeScene, objectAList, objectBList, a1, b1 }; + }; + + // Picks ObjectA where MyVariable === expectedAValue. + const pickAByVar = (expectedAValue) => ({ + type: { value: 'VarObjet' }, + parameters: ['ObjectA', 'MyVariable', '=', String(expectedAValue)], + }); + // Picks ObjectB where MyVariable === expectedBValue. + const pickBByVar = (expectedBValue) => ({ + type: { value: 'VarObjet' }, + parameters: ['ObjectB', 'MyVariable', '=', String(expectedBValue)], + }); + // Free condition with constant truth value (does not reference any object). + const freeCondition = (alwaysTrue) => ({ + type: { value: 'Egal' }, + parameters: ['1', '=', alwaysTrue ? '1' : '0'], + }); + + // Action: increment ObjectA's "Touched" variable. Runs once per picked + // ObjectA instance, so the post-condition variable reveals how many were + // picked when the action ran. + const touchA = { + type: { value: 'ModVarObjet' }, + parameters: ['ObjectA', 'Touched', '+', '1'], + }; + const touchB = { + type: { value: 'ModVarObjet' }, + parameters: ['ObjectB', 'Touched', '+', '1'], + }; + // Action: set scene variable, runs once regardless of picks. Used to + // confirm the Or's truth value (whether the action block ran at all). + const setScene = (name, value) => ({ + type: { value: 'ModVarScene' }, + parameters: [name, '=', String(value)], + }); + + /* ------------------------------------------------------------------ */ + /* 1. Door/Coin — the preserve-picks Or must scope picks to the */ + /* branch that fired. */ + /* ------------------------------------------------------------------ */ + describe('Or — Door/Coin (act on the touched object)', () => { + const buildEvents = (orType) => [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + { + type: { value: orType }, + parameters: [], + subInstructions: [pickAByVar(5), pickBByVar(7)], + }, + ], + actions: [touchA, touchB, setScene('OrFired', 1)], + events: [], + }, + ]; + + it('Or picks A only when A matches', () => { + const { runtimeScene, a1, b1 } = runEventsWithTwoObjects( + buildEvents('BuiltinCommonInstructions::Or'), + { aValue: 5, bValue: 999 } + ); + expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(b1.getVariables().get('Touched').getAsNumber()).toBe(0); + }); + + it('Or picks B only when B matches', () => { + const { runtimeScene, a1, b1 } = runEventsWithTwoObjects( + buildEvents('BuiltinCommonInstructions::Or'), + { aValue: 999, bValue: 7 } + ); + expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(0); + expect(b1.getVariables().get('Touched').getAsNumber()).toBe(1); + }); + + it('Or picks both when both match', () => { + const { runtimeScene, a1, b1 } = runEventsWithTwoObjects( + buildEvents('BuiltinCommonInstructions::Or'), + { aValue: 5, bValue: 7 } + ); + expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(b1.getVariables().get('Touched').getAsNumber()).toBe(1); + }); + }); + + /* ------------------------------------------------------------------ */ + /* 2. AllowedArea pair — outside-Or picking that the Or must not */ + /* erase (the bug fix). */ + /* ------------------------------------------------------------------ */ + describe('Or — preserves outside picks when no true branch contributed', () => { + // Outside the Or, ObjectA is filtered to the instance with var=5. Then + // the Or runs: + // branch B: VarObjet(ObjectA, "MyVariable", "=", 999) — references A, + // but is false. + // branch C: Egal(1, 1) — true, free. + // No true branch contributed for A, so the action should still run on + // the previously picked A. Before the fix, the Or overwrote A with the + // empty "final" list and the action saw nothing. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + pickAByVar(5), + { + type: { value: 'BuiltinCommonInstructions::Or' }, + parameters: [], + subInstructions: [pickAByVar(999), freeCondition(true)], + }, + ], + actions: [touchA, setScene('OrFired', 1)], + events: [], + }, + ]; + + it('keeps the outside-Or pick of ObjectA when only the free branch is true', () => { + const { runtimeScene, a1 } = runEventsWithTwoObjects(events, { + aValue: 5, + bValue: 0, + }); + expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + }); + }); + + /* ------------------------------------------------------------------ */ + /* 3. Both branches false — both Or variants leave parent picks */ + /* untouched (the action does not run, but sub-events that read */ + /* parent picks should not see them wiped). */ + /* ------------------------------------------------------------------ */ + describe('Or — all-false leaves parent picks alone', () => { + const buildEvents = (orType) => [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + pickAByVar(5), + { + type: { value: orType }, + parameters: [], + subInstructions: [pickAByVar(999), pickBByVar(999)], + }, + ], + // The Or is false so the action does not run, but a sub-event runs + // unconditionally on whatever ObjectA list survives. If the Or + // (incorrectly) wiped the list, ObjectA.Touched would be 0. + actions: [], + events: [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [], + actions: [touchA], + events: [], + }, + ], + }, + ]; + + it('Or: picks survive when the Or is false', () => { + const { a1 } = runEventsWithTwoObjects( + buildEvents('BuiltinCommonInstructions::Or'), + { aValue: 5, bValue: 0 } + ); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + }); + + it('OrDistributive: picks survive when the Or is false', () => { + const { a1 } = runEventsWithTwoObjects( + buildEvents('BuiltinCommonInstructions::OrDistributive'), + { aValue: 5, bValue: 0 } + ); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + }); + }); + + /* ------------------------------------------------------------------ */ + /* 4. Input + SubmitButton — the canonical case for OrDistributive. */ + /* A branch that doesn't reference ObjectA must keep ObjectA */ + /* available to the action. */ + /* ------------------------------------------------------------------ */ + describe('OrDistributive — Input/Button (act on ObjectA regardless)', () => { + // branch A: pickAByVar(5) — references A, may be true or false. + // branch B: free, always true — does NOT reference A. + // Action: touch A. The action should run on ObjectA whenever the Or + // returns true, even if branch A failed to pick A. + const buildEvents = (orType) => [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + { + type: { value: orType }, + parameters: [], + subInstructions: [pickAByVar(5), freeCondition(true)], + }, + ], + actions: [touchA, setScene('OrFired', 1)], + events: [], + }, + ]; + + it('OrDistributive picks A when only the free branch is true', () => { + const { runtimeScene, a1 } = runEventsWithTwoObjects( + buildEvents('BuiltinCommonInstructions::OrDistributive'), + { aValue: 999, bValue: 0 } // branch A is false (var != 5) + ); + expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + }); + + it('OrDistributive picks A when branch A also matches', () => { + const { runtimeScene, a1 } = runEventsWithTwoObjects( + buildEvents('BuiltinCommonInstructions::OrDistributive'), + { aValue: 5, bValue: 0 } + ); + expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + }); + + it('preserve-picks Or fails on this pattern (expected; documents why a separate condition exists)', () => { + // With the regular Or, branch A is false and branch B does not + // reference A. No true branch contributed for A, so the bug-fixed Or + // leaves the parent's A list at whatever it was at the start of the + // Or. Because A is registered by the Or as "empty if just declared", + // its list starts empty here — and so the action runs on zero + // instances. This is the case where users need OrDistributive. + const { a1 } = runEventsWithTwoObjects( + buildEvents('BuiltinCommonInstructions::Or'), + { aValue: 999, bValue: 0 } + ); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(0); + }); + }); + + /* ------------------------------------------------------------------ */ + /* 5. "Score on trigger" — OrDistributive lets unrelated objects be */ + /* available regardless of which branch fired. */ + /* ------------------------------------------------------------------ */ + describe('OrDistributive — score-on-trigger (unrelated object stays available)', () => { + // Same pattern as Input/Button but the action targets ObjectB while the + // branches test ObjectA / a free condition. With OrDistributive, + // ObjectB stays unconstrained and the action runs on it. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + { + type: { value: 'BuiltinCommonInstructions::OrDistributive' }, + parameters: [], + subInstructions: [pickAByVar(5), freeCondition(true)], + }, + ], + actions: [touchB, setScene('OrFired', 1)], + events: [], + }, + ]; + + it('action acts on ObjectB even though no branch references it', () => { + const { runtimeScene, b1 } = runEventsWithTwoObjects(events, { + aValue: 999, + bValue: 42, + }); + expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); + expect(b1.getVariables().get('Touched').getAsNumber()).toBe(1); + }); + }); + + /* ------------------------------------------------------------------ */ + /* 6. Sanity check: OrDistributive is NOT a drop-in replacement for */ + /* Or in the Door/Coin pattern. This test documents that distributive */ + /* Or, when used on Door/Coin, "leaks" the unrelated-branch object */ + /* into the action — which is exactly why we keep both operators. */ + /* ------------------------------------------------------------------ */ + describe('OrDistributive — Door/Coin shows why both operators are needed', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + { + type: { value: 'BuiltinCommonInstructions::OrDistributive' }, + parameters: [], + subInstructions: [pickAByVar(5), pickBByVar(7)], + }, + ], + actions: [touchA, touchB], + events: [], + }, + ]; + + it('only A matches: OrDistributive still touches B (use regular Or for this case)', () => { + // Branch A picks A=5. Branch B is false. Distributive Or keeps B + // unconstrained for branch A's contribution, so B ends up picked too. + // This is the documented mis-fit between distributive Or and the + // Door/Coin pattern. + const { a1, b1 } = runEventsWithTwoObjects(events, { + aValue: 5, + bValue: 999, + }); + expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(b1.getVariables().get('Touched').getAsNumber()).toBe(1); + }); + }); +}); From ceb26c26f1e934f20d1fbba3a265150e8da078e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 6 May 2026 21:21:02 +0000 Subject: [PATCH 02/12] Fix all-false Or test assertions The previous "picks survive when the Or is false" tests asserted on a sub-event action firing, but the parent event's conditions short-circuit when the Or is false, so the sub-event never runs and the assertion was unsound. Replace with a sanity check that the action does not run when both Or branches are false (which is what's actually observable). The "preserve parent picks when no true branch contributed for an object" path is fully covered by the AllowedArea case above (where the Or itself is true). --- ...ctPickingCodeGenerationIntegrationTests.js | 38 ++++++++----------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js index fabfdca1c08c..be71893e142e 100644 --- a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js +++ b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js @@ -206,51 +206,43 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => }); /* ------------------------------------------------------------------ */ - /* 3. Both branches false — both Or variants leave parent picks */ - /* untouched (the action does not run, but sub-events that read */ - /* parent picks should not see them wiped). */ + /* 3. All-false Or returns false (sanity check that a both-false */ + /* Or properly fails the parent event so the action does not run). */ + /* The "preserve parent picks when no branch contributed" path is */ + /* fully exercised by case 2 above; once the Or is false, the */ + /* parent event short-circuits before any action can observe the */ + /* picked-list state, so there is nothing more to assert here. */ /* ------------------------------------------------------------------ */ - describe('Or — all-false leaves parent picks alone', () => { + describe('Or — all-false returns false', () => { const buildEvents = (orType) => [ { type: 'BuiltinCommonInstructions::Standard', conditions: [ - pickAByVar(5), { type: { value: orType }, parameters: [], subInstructions: [pickAByVar(999), pickBByVar(999)], }, ], - // The Or is false so the action does not run, but a sub-event runs - // unconditionally on whatever ObjectA list survives. If the Or - // (incorrectly) wiped the list, ObjectA.Touched would be 0. - actions: [], - events: [ - { - type: 'BuiltinCommonInstructions::Standard', - conditions: [], - actions: [touchA], - events: [], - }, - ], + actions: [setScene('OrFired', 1)], + events: [], }, ]; - it('Or: picks survive when the Or is false', () => { - const { a1 } = runEventsWithTwoObjects( + it('Or: action does not run when both branches are false', () => { + const { runtimeScene } = runEventsWithTwoObjects( buildEvents('BuiltinCommonInstructions::Or'), { aValue: 5, bValue: 0 } ); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(runtimeScene.getVariables().has('OrFired')).toBe(false); }); - it('OrDistributive: picks survive when the Or is false', () => { - const { a1 } = runEventsWithTwoObjects( + it('OrDistributive: action does not run when both branches are false', () => { + const { runtimeScene } = runEventsWithTwoObjects( buildEvents('BuiltinCommonInstructions::OrDistributive'), { aValue: 5, bValue: 0 } ); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(runtimeScene.getVariables().has('OrFired')).toBe(false); }); }); From b5e04b35d188b528908ef3a06f9458a9fb120004 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 6 May 2026 21:46:55 +0000 Subject: [PATCH 03/12] Reset Or branch boolean to false; broaden Or picking tests - Or / OrDistributive codegen: each branch's isConditionTrue boolean is now reset to false at the start of the branch. Without this, a branch could inherit the previous branch's truth value, causing a branch that picks zero instances to be wrongly treated as a contribution. The bug was latent in the original Or but masked by the structure of typical Door/Coin events; OrDistributive exposes it because every branch references every union object. - Tests now use three instances per object with deterministic MyVariable values (1, 2, 3) so each test asserts which specific instances get touched (per-instance Touched array), rather than just totals. - Adds a sibling ObjectC and tests confirming that picks for objects the Or does not reference are not disturbed. - Adds branches that nest BuiltinCommonInstructions::And inside the Or, so a single branch picks several objects together. Includes asymmetric And/picking cases that highlight the difference between preserve-picks Or and OrDistributive. - Adds a regression test for the boolean-reset fix: AllowedArea pattern with the free branch BEFORE the false A-branch (would have failed without the reset). --- .../Builtin/CommonInstructionsExtension.cpp | 21 +- ...ctPickingCodeGenerationIntegrationTests.js | 660 ++++++++++++------ 2 files changed, 467 insertions(+), 214 deletions(-) diff --git a/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp b/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp index 889166ce4e26..29db2b6bfe61 100644 --- a/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp +++ b/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp @@ -297,8 +297,17 @@ CommonInstructionsExtension::CommonInstructionsExtension() { // Create new objects lists and generate condition conditionsCode += codeGenerator.GenerateObjectsDeclarationCode(context); - if (!conditions[cId].GetType().empty()) + if (!conditions[cId].GetType().empty()) { + // Reset the branch's boolean to false: condition codegen does + // not zero its return boolean, so without this each branch + // would inherit the previous branch's truth value, causing a + // false branch that picks 0 to be treated as a contribution. + conditionsCode += + codeGenerator.GenerateBooleanFullName("isConditionTrue", + context) + + " = false;\n"; conditionsCode += conditionCode; + } // If the condition is true : merge all objects picked in the // final object lists. @@ -454,8 +463,16 @@ CommonInstructionsExtension::CommonInstructionsExtension() { conditionsCode += "{\n"; conditionsCode += codeGenerator.GenerateObjectsDeclarationCode(context); - if (!conditions[cId].GetType().empty()) + if (!conditions[cId].GetType().empty()) { + // See the matching reset comment in the Or generator above: + // each branch must start with isConditionTrue = false so a + // previous true branch does not leak into this one. + conditionsCode += + codeGenerator.GenerateBooleanFullName("isConditionTrue", + context) + + " = false;\n"; conditionsCode += conditionCode; + } conditionsCode += "if(" + codeGenerator.GenerateBooleanFullName( diff --git a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js index be71893e142e..15e81dfc7f67 100644 --- a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js +++ b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js @@ -17,14 +17,28 @@ * the action acts on objects unrelated to the branch that fired * (TextInput + SubmitButton pattern). * + * Each test seeds three instances per object (MyVariable=1, 2 and 3) so a + * picking condition such as `VarObjet(ObjectX, "MyVariable", "=", N)` + * deterministically picks the Nth instance. After running, an + * `ModVarObjet(..., "Touched", "+", "1")` action is used per relevant object + * to reveal exactly which instances were picked at action time — the test + * asserts on the `Touched` array, not just on totals, so each instance's + * fate is checked individually. An unrelated `ObjectC` is included (and + * picked by a sibling condition outside the Or) to verify that the Or + * never disturbs picks for objects it does not reference. + * * The canonical patterns covered below: - * 1. Door/Coin — preserve-picks Or only. - * 2. AllowedArea pair — preserve-picks Or only (bug-fix path). - * 3. Both branches false — both Or variants must leave parent picks alone. - * 4. Input + Submit — distributive Or only. - * 5. "Score on trigger" — distributive Or only. - * 6. Sanity — distributive Or breaks Door/Coin (documents the - * difference between the two operators). + * 1. Door/Coin — preserve-picks Or only. + * 2. AllowedArea pair — preserve-picks Or only (bug-fix path). + * 3. All-false — both Or variants must return false + * (no action runs). + * 4. Input + Submit — distributive Or only. + * 5. Score-on-trigger — distributive Or only. + * 6. Door/Coin sanity (negative test for OrDistributive). + * 7. Branches with `And` — multi-condition branches that pick + * several objects at once. + * 8. Unrelated objects — picks for objects the Or doesn't + * reference are never disturbed. */ const initializeGDevelopJs = require('../../Binaries/embuild/GDevelop.js/libGD.js'); @@ -40,17 +54,13 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => }); /** - * Build a small project with two object parameters, run the supplied events, - * and return the resulting (mutated) parameter object lists plus the - * runtime scene so tests can assert on picked lists and scene variables. - * - * The events function is given two object parameters: ObjectA and ObjectB. - * Each test seeds these with a single instance whose 'MyVariable' is set - * to a value the test controls, so that picking conditions of the form - * `VarObjet(ObjectX, "MyVariable", "=", N)` deterministically pick or skip - * the instance. + * Build a small project with three object parameters (ObjectA, ObjectB, + * ObjectC), seed each with three instances whose `MyVariable` is 1, 2 and + * 3 respectively, run the supplied events, and return both the runtime + * scene and the per-object instance arrays so tests can assert on the + * `Touched` flag of each individual instance. */ - const runEventsWithTwoObjects = (events, { aValue, bValue }) => { + const runEventsWithThreeObjectsThreeInstances = (events, logCode) => { const serializerElement = gd.Serializer.fromJSObject(events); const runCompiledEvents = generateCompiledEventsFromSerializedEvents( gd, @@ -59,317 +69,543 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => parameterTypes: { ObjectA: 'object', ObjectB: 'object', + ObjectC: 'object', }, - logCode: false, + logCode: !!logCode, } ); const { gdjs, runtimeScene } = makeMinimalGDJSMock(); - const a1 = runtimeScene.createObject('ObjectA'); - a1.getVariables().get('MyVariable').setNumber(aValue); - const objectAList = new gdjs.Hashtable(); - objectAList.put('ObjectA', [a1]); + const makeInstances = (objectName) => { + const insts = [1, 2, 3].map((value) => { + const obj = runtimeScene.createObject(objectName); + obj.getVariables().get('MyVariable').setNumber(value); + return obj; + }); + const lists = new gdjs.Hashtable(); + // Pre-pick every instance so the picked list starts populated. The + // generated events function uses `eventsFunctionContext.getObjects` for + // the parameter as the source for picking, so a pre-populated list is + // what conditions filter against. + lists.put(objectName, insts.slice()); + return { insts, lists }; + }; - const b1 = runtimeScene.createObject('ObjectB'); - b1.getVariables().get('MyVariable').setNumber(bValue); - const objectBList = new gdjs.Hashtable(); - objectBList.put('ObjectB', [b1]); + const a = makeInstances('ObjectA'); + const b = makeInstances('ObjectB'); + const c = makeInstances('ObjectC'); - runCompiledEvents(gdjs, runtimeScene, [objectAList, objectBList]); + runCompiledEvents(gdjs, runtimeScene, [a.lists, b.lists, c.lists]); - return { runtimeScene, objectAList, objectBList, a1, b1 }; + return { + runtimeScene, + aInsts: a.insts, + bInsts: b.insts, + cInsts: c.insts, + aLists: a.lists, + bLists: b.lists, + cLists: c.lists, + }; }; - // Picks ObjectA where MyVariable === expectedAValue. - const pickAByVar = (expectedAValue) => ({ - type: { value: 'VarObjet' }, - parameters: ['ObjectA', 'MyVariable', '=', String(expectedAValue)], - }); - // Picks ObjectB where MyVariable === expectedBValue. - const pickBByVar = (expectedBValue) => ({ + // Returns [touched1, touched2, touched3] for the given instance array, + // where each entry is the value of the "Touched" object variable. A 0 in + // the array means "the action did not run on this instance". + const touchedFlags = (insts) => + insts.map((o) => o.getVariables().get('Touched').getAsNumber()); + + const pickByVar = (objectName, expectedValue) => ({ type: { value: 'VarObjet' }, - parameters: ['ObjectB', 'MyVariable', '=', String(expectedBValue)], + parameters: [ + objectName, + 'MyVariable', + '=', + String(expectedValue), + ], }); + const pickAByVar = (v) => pickByVar('ObjectA', v); + const pickBByVar = (v) => pickByVar('ObjectB', v); + const pickCByVar = (v) => pickByVar('ObjectC', v); + // Free condition with constant truth value (does not reference any object). const freeCondition = (alwaysTrue) => ({ type: { value: 'Egal' }, parameters: ['1', '=', alwaysTrue ? '1' : '0'], }); - // Action: increment ObjectA's "Touched" variable. Runs once per picked - // ObjectA instance, so the post-condition variable reveals how many were - // picked when the action ran. - const touchA = { - type: { value: 'ModVarObjet' }, - parameters: ['ObjectA', 'Touched', '+', '1'], - }; - const touchB = { + // Action that increments the per-instance "Touched" variable. Runs once + // per picked instance. + const touch = (objectName) => ({ type: { value: 'ModVarObjet' }, - parameters: ['ObjectB', 'Touched', '+', '1'], - }; - // Action: set scene variable, runs once regardless of picks. Used to - // confirm the Or's truth value (whether the action block ran at all). + parameters: [objectName, 'Touched', '+', '1'], + }); + const touchA = touch('ObjectA'); + const touchB = touch('ObjectB'); + const touchC = touch('ObjectC'); + + // Action that sets a scene variable, runs once regardless of picks. Used + // to confirm the Or's truth value (whether the action block ran at all). const setScene = (name, value) => ({ type: { value: 'ModVarScene' }, parameters: [name, '=', String(value)], }); - /* ------------------------------------------------------------------ */ - /* 1. Door/Coin — the preserve-picks Or must scope picks to the */ - /* branch that fired. */ - /* ------------------------------------------------------------------ */ - describe('Or — Door/Coin (act on the touched object)', () => { - const buildEvents = (orType) => [ + const andOf = (...subInstructions) => ({ + type: { value: 'BuiltinCommonInstructions::And' }, + parameters: [], + subInstructions, + }); + const orOf = (...subInstructions) => ({ + type: { value: 'BuiltinCommonInstructions::Or' }, + parameters: [], + subInstructions, + }); + const orDistributiveOf = (...subInstructions) => ({ + type: { value: 'BuiltinCommonInstructions::OrDistributive' }, + parameters: [], + subInstructions, + }); + + /* ================================================================== */ + /* 1. Door/Coin — the preserve-picks Or scopes picks to the branch */ + /* that fired. Only the matching instance(s) get touched. */ + /* ================================================================== */ + describe('Or — Door/Coin (act on the touched object), three instances each', () => { + const buildEvents = () => [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [ - { - type: { value: orType }, - parameters: [], - subInstructions: [pickAByVar(5), pickBByVar(7)], - }, - ], + conditions: [orOf(pickAByVar(2), pickBByVar(2))], actions: [touchA, touchB, setScene('OrFired', 1)], events: [], }, ]; - it('Or picks A only when A matches', () => { - const { runtimeScene, a1, b1 } = runEventsWithTwoObjects( - buildEvents('BuiltinCommonInstructions::Or'), - { aValue: 5, bValue: 999 } - ); + it('only A=2 matches: only A2 is touched, no B is touched', () => { + // Branch A picks A=2 (matches A2 only). Branch B picks B=2 (B has + // value 2 too — but with multi-instance we need to control which B + // values exist). Here B values are [1, 2, 3] so branch B is also + // true and picks B2. To exercise "only A matches", make branch B + // false: pick B=999. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickAByVar(2), pickBByVar(999))], + actions: [touchA, touchB, setScene('OrFired', 1)], + events: [], + }, + ]; + const { runtimeScene, aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); - expect(b1.getVariables().get('Touched').getAsNumber()).toBe(0); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); }); - it('Or picks B only when B matches', () => { - const { runtimeScene, a1, b1 } = runEventsWithTwoObjects( - buildEvents('BuiltinCommonInstructions::Or'), - { aValue: 999, bValue: 7 } - ); + it('only B=3 matches: only B3 is touched, no A is touched', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickAByVar(999), pickBByVar(3))], + actions: [touchA, touchB, setScene('OrFired', 1)], + events: [], + }, + ]; + const { runtimeScene, aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(0); - expect(b1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 1]); }); - it('Or picks both when both match', () => { - const { runtimeScene, a1, b1 } = runEventsWithTwoObjects( - buildEvents('BuiltinCommonInstructions::Or'), - { aValue: 5, bValue: 7 } - ); + it('both branches match: only the picked instances get touched, others stay untouched', () => { + const { runtimeScene, aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(buildEvents()); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); - expect(b1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 1, 0]); }); }); - /* ------------------------------------------------------------------ */ + /* ================================================================== */ /* 2. AllowedArea pair — outside-Or picking that the Or must not */ - /* erase (the bug fix). */ - /* ------------------------------------------------------------------ */ + /* erase (the bug fix). Only the originally picked A2 should be */ + /* touched. */ + /* ================================================================== */ describe('Or — preserves outside picks when no true branch contributed', () => { - // Outside the Or, ObjectA is filtered to the instance with var=5. Then - // the Or runs: - // branch B: VarObjet(ObjectA, "MyVariable", "=", 999) — references A, - // but is false. - // branch C: Egal(1, 1) — true, free. - // No true branch contributed for A, so the action should still run on - // the previously picked A. Before the fix, the Or overwrote A with the - // empty "final" list and the action saw nothing. + // Outside the Or, ObjectA is filtered to A2. Inside the Or: + // branch B: pickA=999 — references A but is false. + // branch C: free, true — does not reference A. + // No true branch contributed for A, so the bug-fixed Or leaves the + // outside pick of A2 alone. Only A2 should be touched. const events = [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [ - pickAByVar(5), - { - type: { value: 'BuiltinCommonInstructions::Or' }, - parameters: [], - subInstructions: [pickAByVar(999), freeCondition(true)], - }, - ], + conditions: [pickAByVar(2), orOf(pickAByVar(999), freeCondition(true))], actions: [touchA, setScene('OrFired', 1)], events: [], }, ]; - it('keeps the outside-Or pick of ObjectA when only the free branch is true', () => { - const { runtimeScene, a1 } = runEventsWithTwoObjects(events, { - aValue: 5, - bValue: 0, - }); + it('keeps the outside-Or pick (A2) when only the free branch is true', () => { + const { runtimeScene, aInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + }); + + it('keeps the outside-Or pick (A2) when the free branch comes BEFORE the false A-branch', () => { + // Branch order matters: this test exercises the boolean-reset fix. + // Without it, the true free branch would leak its truth value into + // the next branch's "if(isConditionTrue)" check, and the false + // pickA(999) branch would be wrongly treated as a contribution + // — collapsing parent.A to the empty filtered list. + const eventsReversed = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + pickAByVar(2), + orOf(freeCondition(true), pickAByVar(999)), + ], + actions: [touchA, setScene('OrFired', 1)], + events: [], + }, + ]; + const { runtimeScene, aInsts } = + runEventsWithThreeObjectsThreeInstances(eventsReversed); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); }); }); - /* ------------------------------------------------------------------ */ - /* 3. All-false Or returns false (sanity check that a both-false */ - /* Or properly fails the parent event so the action does not run). */ - /* The "preserve parent picks when no branch contributed" path is */ - /* fully exercised by case 2 above; once the Or is false, the */ - /* parent event short-circuits before any action can observe the */ - /* picked-list state, so there is nothing more to assert here. */ - /* ------------------------------------------------------------------ */ + /* ================================================================== */ + /* 3. All-false Or returns false (sanity check that the Or correctly */ + /* fails the parent event so the action does not run). */ + /* ================================================================== */ describe('Or — all-false returns false', () => { - const buildEvents = (orType) => [ + const buildEvents = (orWrapper) => [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [ - { - type: { value: orType }, - parameters: [], - subInstructions: [pickAByVar(999), pickBByVar(999)], - }, - ], - actions: [setScene('OrFired', 1)], + conditions: [orWrapper(pickAByVar(999), pickBByVar(999))], + actions: [touchA, touchB, setScene('OrFired', 1)], events: [], }, ]; it('Or: action does not run when both branches are false', () => { - const { runtimeScene } = runEventsWithTwoObjects( - buildEvents('BuiltinCommonInstructions::Or'), - { aValue: 5, bValue: 0 } - ); + const { runtimeScene, aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(buildEvents(orOf)); expect(runtimeScene.getVariables().has('OrFired')).toBe(false); + expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); }); it('OrDistributive: action does not run when both branches are false', () => { - const { runtimeScene } = runEventsWithTwoObjects( - buildEvents('BuiltinCommonInstructions::OrDistributive'), - { aValue: 5, bValue: 0 } - ); + const { runtimeScene, aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(buildEvents(orDistributiveOf)); expect(runtimeScene.getVariables().has('OrFired')).toBe(false); + expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); }); }); - /* ------------------------------------------------------------------ */ - /* 4. Input + SubmitButton — the canonical case for OrDistributive. */ + /* ================================================================== */ + /* 4. Input + SubmitButton — canonical case for OrDistributive. */ /* A branch that doesn't reference ObjectA must keep ObjectA */ /* available to the action. */ - /* ------------------------------------------------------------------ */ + /* ================================================================== */ describe('OrDistributive — Input/Button (act on ObjectA regardless)', () => { - // branch A: pickAByVar(5) — references A, may be true or false. - // branch B: free, always true — does NOT reference A. - // Action: touch A. The action should run on ObjectA whenever the Or - // returns true, even if branch A failed to pick A. - const buildEvents = (orType) => [ + // branch 1: pickA=999 — references A, false (no instance has var=999). + // branch 2: free, true — does not reference A. + // With OrDistributive, all three A instances stay available. + const events = [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [ - { - type: { value: orType }, - parameters: [], - subInstructions: [pickAByVar(5), freeCondition(true)], - }, - ], + conditions: [orDistributiveOf(pickAByVar(999), freeCondition(true))], actions: [touchA, setScene('OrFired', 1)], events: [], }, ]; - it('OrDistributive picks A when only the free branch is true', () => { - const { runtimeScene, a1 } = runEventsWithTwoObjects( - buildEvents('BuiltinCommonInstructions::OrDistributive'), - { aValue: 999, bValue: 0 } // branch A is false (var != 5) - ); + it('OrDistributive: all A instances stay picked when only the free branch is true', () => { + const { runtimeScene, aInsts } = + runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(touchedFlags(aInsts)).toEqual([1, 1, 1]); }); - it('OrDistributive picks A when branch A also matches', () => { - const { runtimeScene, a1 } = runEventsWithTwoObjects( - buildEvents('BuiltinCommonInstructions::OrDistributive'), - { aValue: 5, bValue: 0 } - ); - expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); + it('OrDistributive: when both branches true, every A is still picked (union over all branches)', () => { + const events2 = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orDistributiveOf(pickAByVar(2), freeCondition(true)), + ], + actions: [touchA, setScene('OrFired', 1)], + events: [], + }, + ]; + const { aInsts } = runEventsWithThreeObjectsThreeInstances(events2); + // Branch 1 contributes only A2. Branch 2 is unconstrained so it + // contributes the parent's full A list (A1, A2, A3). Union = all + // three. + expect(touchedFlags(aInsts)).toEqual([1, 1, 1]); }); it('preserve-picks Or fails on this pattern (expected; documents why a separate condition exists)', () => { - // With the regular Or, branch A is false and branch B does not - // reference A. No true branch contributed for A, so the bug-fixed Or - // leaves the parent's A list at whatever it was at the start of the - // Or. Because A is registered by the Or as "empty if just declared", - // its list starts empty here — and so the action runs on zero - // instances. This is the case where users need OrDistributive. - const { a1 } = runEventsWithTwoObjects( - buildEvents('BuiltinCommonInstructions::Or'), - { aValue: 999, bValue: 0 } - ); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(0); + // With the regular Or, branch 1 is false and branch 2 does not + // reference A. No true branch contributed for A, so the bug-fixed + // Or leaves A at whatever it was at the start of the Or — and + // because A is registered as "empty if just declared", that's an + // empty list. The action runs zero times. + const events3 = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickAByVar(999), freeCondition(true))], + actions: [touchA, setScene('OrFired', 1)], + events: [], + }, + ]; + const { aInsts } = runEventsWithThreeObjectsThreeInstances(events3); + expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); }); }); - /* ------------------------------------------------------------------ */ + /* ================================================================== */ /* 5. "Score on trigger" — OrDistributive lets unrelated objects be */ /* available regardless of which branch fired. */ - /* ------------------------------------------------------------------ */ + /* ================================================================== */ describe('OrDistributive — score-on-trigger (unrelated object stays available)', () => { - // Same pattern as Input/Button but the action targets ObjectB while the - // branches test ObjectA / a free condition. With OrDistributive, - // ObjectB stays unconstrained and the action runs on it. const events = [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [ - { - type: { value: 'BuiltinCommonInstructions::OrDistributive' }, - parameters: [], - subInstructions: [pickAByVar(5), freeCondition(true)], - }, - ], + conditions: [orDistributiveOf(pickAByVar(2), freeCondition(true))], + // Action acts on B although neither branch references B. actions: [touchB, setScene('OrFired', 1)], events: [], }, ]; - it('action acts on ObjectB even though no branch references it', () => { - const { runtimeScene, b1 } = runEventsWithTwoObjects(events, { - aValue: 999, - bValue: 42, - }); + it('action acts on every B instance even though no branch references B', () => { + const { runtimeScene, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); - expect(b1.getVariables().get('Touched').getAsNumber()).toBe(1); + expect(touchedFlags(bInsts)).toEqual([1, 1, 1]); }); }); - /* ------------------------------------------------------------------ */ + /* ================================================================== */ /* 6. Sanity check: OrDistributive is NOT a drop-in replacement for */ - /* Or in the Door/Coin pattern. This test documents that distributive */ - /* Or, when used on Door/Coin, "leaks" the unrelated-branch object */ - /* into the action — which is exactly why we keep both operators. */ - /* ------------------------------------------------------------------ */ + /* Or in the Door/Coin pattern. This test documents the leak. */ + /* ================================================================== */ describe('OrDistributive — Door/Coin shows why both operators are needed', () => { + // Branch 1 picks A=2. Branch 2 picks B=999 (false). With distributive + // semantics, branch 1 leaves B unconstrained and contributes B's + // entire parent list, so the action ends up touching every B. const events = [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [ - { - type: { value: 'BuiltinCommonInstructions::OrDistributive' }, - parameters: [], - subInstructions: [pickAByVar(5), pickBByVar(7)], - }, - ], + conditions: [orDistributiveOf(pickAByVar(2), pickBByVar(999))], actions: [touchA, touchB], events: [], }, ]; - it('only A matches: OrDistributive still touches B (use regular Or for this case)', () => { - // Branch A picks A=5. Branch B is false. Distributive Or keeps B - // unconstrained for branch A's contribution, so B ends up picked too. - // This is the documented mis-fit between distributive Or and the - // Door/Coin pattern. - const { a1, b1 } = runEventsWithTwoObjects(events, { - aValue: 5, - bValue: 999, - }); - expect(a1.getVariables().get('Touched').getAsNumber()).toBe(1); - expect(b1.getVariables().get('Touched').getAsNumber()).toBe(1); + it('only A=2 matches but distributive Or also touches every B (use regular Or for this case)', () => { + const { aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(bInsts)).toEqual([1, 1, 1]); + }); + }); + + /* ================================================================== */ + /* 7. Branches with `And` — a single branch can constrain several */ + /* objects at once. The branch is true only when every */ + /* sub-condition is true; when true, every sub-condition's picks */ + /* contribute to the corresponding object's picked list. */ + /* ================================================================== */ + describe('Or — branches with And { picks A and B together }', () => { + it('Or: each true And-branch contributes its own A and B picks; others untouched', () => { + // Branch 1: And{A=1, B=1} — both match → branch 1 true → contribute + // A1, B1. + // Branch 2: And{A=3, B=3} — both match → branch 2 true → contribute + // A3, B3. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orOf( + andOf(pickAByVar(1), pickBByVar(1)), + andOf(pickAByVar(3), pickBByVar(3)) + ), + ], + actions: [touchA, touchB], + events: [], + }, + ]; + const { aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 0, 1]); + expect(touchedFlags(bInsts)).toEqual([1, 0, 1]); + }); + + it('Or: And-branch is false if any sub-condition fails → no contribution from that branch', () => { + // Branch 1: And{A=1, B=1} — both match → contribute A1, B1. + // Branch 2: And{A=2, B=999} — A=2 picks A2 but B=999 picks nothing, + // so And is false → no contribution. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orOf( + andOf(pickAByVar(1), pickBByVar(1)), + andOf(pickAByVar(2), pickBByVar(999)) + ), + ], + actions: [touchA, touchB], + events: [], + }, + ]; + const { aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 0, 0]); + expect(touchedFlags(bInsts)).toEqual([1, 0, 0]); + }); + }); + + describe('OrDistributive — branches with And { picks A and B together }', () => { + it('OrDistributive: each true And-branch is symmetric to Or because every branch references both A and B', () => { + // When every branch references every object in the union, the + // unconstrained-fill of OrDistributive does not engage and the + // result is identical to the regular Or above. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orDistributiveOf( + andOf(pickAByVar(1), pickBByVar(1)), + andOf(pickAByVar(3), pickBByVar(3)) + ), + ], + actions: [touchA, touchB], + events: [], + }, + ]; + const { aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 0, 1]); + expect(touchedFlags(bInsts)).toEqual([1, 0, 1]); + }); + + it('OrDistributive: asymmetric branches — branch with And{A=1} leaves B unconstrained, the lone B-branch leaves A unconstrained', () => { + // Branch 1: And{A=1} — references only A; B is unconstrained for + // this branch (contributes the parent's full B list when branch + // is true). + // Branch 2: pickB=2 — references only B; A is unconstrained for + // this branch (contributes the parent's full A list when branch + // is true). + // Both branches are true → every A and every B end up in the union. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orDistributiveOf(andOf(pickAByVar(1)), pickBByVar(2)), + ], + actions: [touchA, touchB], + events: [], + }, + ]; + const { aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 1, 1]); + expect(touchedFlags(bInsts)).toEqual([1, 1, 1]); + }); + + it('OrDistributive: same asymmetric branches with the regular Or only touch the picked instances', () => { + // Same shape as above but with the preserve-picks Or — branches + // contribute only what they actually picked, so only A1 and B2 + // are touched. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(andOf(pickAByVar(1)), pickBByVar(2))], + actions: [touchA, touchB], + events: [], + }, + ]; + const { aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 0, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 1, 0]); + }); + }); + + /* ================================================================== */ + /* 8. Unrelated objects — picks for objects the Or doesn't reference */ + /* are never disturbed. ObjectC is picked by a sibling top-level */ + /* event whose conditions reference only C, and that should yield */ + /* the same C picks regardless of how the Or above behaved. */ + /* ================================================================== */ + describe('Or / OrDistributive — unrelated ObjectC is picked independently', () => { + const buildTwoEvents = (orWrapper) => [ + // Event 1 — exercises Or behavior on A and B. ObjectC is never + // mentioned in this event. + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orWrapper(pickAByVar(2), pickBByVar(2))], + actions: [touchA, touchB], + events: [], + }, + // Event 2 — sibling event picking ObjectC by its variable. This + // event has no relation to the Or above; its picks must be + // unaffected by what happened in event 1. + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [pickCByVar(3)], + actions: [touchC], + events: [], + }, + ]; + + it('Or: ObjectC=3 is picked independently of the Or on A and B', () => { + const { aInsts, bInsts, cInsts } = + runEventsWithThreeObjectsThreeInstances(buildTwoEvents(orOf)); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(cInsts)).toEqual([0, 0, 1]); + }); + + it('OrDistributive: ObjectC=3 is picked independently of the OrDistributive on A and B', () => { + const { cInsts } = runEventsWithThreeObjectsThreeInstances( + buildTwoEvents(orDistributiveOf) + ); + expect(touchedFlags(cInsts)).toEqual([0, 0, 1]); + }); + + it('Or with Or false: ObjectC is still picked correctly by the sibling event', () => { + // The Or in event 1 is false, so event 1's actions do not run. The + // sibling event 2 picks ObjectC normally because event contexts + // are independent. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickAByVar(999), pickBByVar(999))], + actions: [touchA, touchB], + events: [], + }, + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [pickCByVar(1)], + actions: [touchC], + events: [], + }, + ]; + const { aInsts, bInsts, cInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(cInsts)).toEqual([1, 0, 0]); }); }); }); From 85178204ea1dedc8f122b978b757350fa53909b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 6 May 2026 22:39:42 +0000 Subject: [PATCH 04/12] Address review on Or picking tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Inline every test's events object so each test is self-contained. No more describe-level shared `events` or `buildEvents()` helpers that were only used by a single test. - Fix the misleading comment on the Door/Coin "only A matches" test (was rambling about a hypothetical B=2 setup that didn't match the actual events). - Drop the misnamed "score-on-trigger" test: with classic Or, a branch that nobody references doesn't constrain that object at all, so the test would pass identically with regular Or and was not actually demonstrating OrDistributive's distinguishing behavior. Replace with a focused "action on B, exactly one branch references B and is false" pair that shows OrDistributive succeeds (every B touched) where regular Or fails (zero B touched). - Drop the unnecessary `andOf(pickAByVar(1))` single-condition And wrappers — they were equivalent to plain `pickAByVar(1)`. Replace with multi-condition Ands (`andOf(pickAByVar(1), pickBByVar(1))`) paired with a non-And branch to make a genuinely asymmetric And-inside-Or test that shows the distributive vs preserve-picks difference. - Rewrite section 8 (unrelated ObjectC). Previously it ran two independent top-level events whose contexts are independent by design, so it wasn't testing anything interesting. Now C is exercised in the SAME event as the Or, with three orderings: no condition on C (action acts on all C), C picked before the Or (only the picked C is touched, the Or does not erase it), C picked after the Or (only the picked C is touched, the Or does not pre-empt it). Each ordering is run for both Or variants. 24 tests, all passing. --- ...ctPickingCodeGenerationIntegrationTests.js | 473 ++++++++++-------- 1 file changed, 264 insertions(+), 209 deletions(-) diff --git a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js index 15e81dfc7f67..9d7851384ccc 100644 --- a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js +++ b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js @@ -14,31 +14,16 @@ * model: a branch that does not constrain a given object behaves as if it * contributed the parent's full picked list for that object. The picked * list at the action is the union over all true branches. Use this when - * the action acts on objects unrelated to the branch that fired - * (TextInput + SubmitButton pattern). + * the action acts on objects that some other branch did pick but the + * firing branch doesn't reference (TextInput + SubmitButton pattern). * * Each test seeds three instances per object (MyVariable=1, 2 and 3) so a * picking condition such as `VarObjet(ObjectX, "MyVariable", "=", N)` * deterministically picks the Nth instance. After running, an * `ModVarObjet(..., "Touched", "+", "1")` action is used per relevant object * to reveal exactly which instances were picked at action time — the test - * asserts on the `Touched` array, not just on totals, so each instance's - * fate is checked individually. An unrelated `ObjectC` is included (and - * picked by a sibling condition outside the Or) to verify that the Or - * never disturbs picks for objects it does not reference. - * - * The canonical patterns covered below: - * 1. Door/Coin — preserve-picks Or only. - * 2. AllowedArea pair — preserve-picks Or only (bug-fix path). - * 3. All-false — both Or variants must return false - * (no action runs). - * 4. Input + Submit — distributive Or only. - * 5. Score-on-trigger — distributive Or only. - * 6. Door/Coin sanity (negative test for OrDistributive). - * 7. Branches with `And` — multi-condition branches that pick - * several objects at once. - * 8. Unrelated objects — picks for objects the Or doesn't - * reference are never disturbed. + * asserts on the per-instance `Touched` array, not just on totals, so + * each instance's fate is checked individually. */ const initializeGDevelopJs = require('../../Binaries/embuild/GDevelop.js/libGD.js'); @@ -103,9 +88,6 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => aInsts: a.insts, bInsts: b.insts, cInsts: c.insts, - aLists: a.lists, - bLists: b.lists, - cLists: c.lists, }; }; @@ -117,12 +99,7 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => const pickByVar = (objectName, expectedValue) => ({ type: { value: 'VarObjet' }, - parameters: [ - objectName, - 'MyVariable', - '=', - String(expectedValue), - ], + parameters: [objectName, 'MyVariable', '=', String(expectedValue)], }); const pickAByVar = (v) => pickByVar('ObjectA', v); const pickBByVar = (v) => pickByVar('ObjectB', v); @@ -171,22 +148,11 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => /* 1. Door/Coin — the preserve-picks Or scopes picks to the branch */ /* that fired. Only the matching instance(s) get touched. */ /* ================================================================== */ - describe('Or — Door/Coin (act on the touched object), three instances each', () => { - const buildEvents = () => [ - { - type: 'BuiltinCommonInstructions::Standard', - conditions: [orOf(pickAByVar(2), pickBByVar(2))], - actions: [touchA, touchB, setScene('OrFired', 1)], - events: [], - }, - ]; - - it('only A=2 matches: only A2 is touched, no B is touched', () => { - // Branch A picks A=2 (matches A2 only). Branch B picks B=2 (B has - // value 2 too — but with multi-instance we need to control which B - // values exist). Here B values are [1, 2, 3] so branch B is also - // true and picks B2. To exercise "only A matches", make branch B - // false: pick B=999. + describe('Or — Door/Coin (act on the touched object)', () => { + it('only branch A matches: only A2 is touched, no B is touched', () => { + // Branch A picks A=2 (matches A2). Branch B picks B=999 (no + // instance has var=999, so the branch is false). The action + // touches whatever survives in each picked list. const events = [ { type: 'BuiltinCommonInstructions::Standard', @@ -202,7 +168,7 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); }); - it('only B=3 matches: only B3 is touched, no A is touched', () => { + it('only branch B matches: only B3 is touched, no A is touched', () => { const events = [ { type: 'BuiltinCommonInstructions::Standard', @@ -219,8 +185,16 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => }); it('both branches match: only the picked instances get touched, others stay untouched', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickAByVar(2), pickBByVar(2))], + actions: [touchA, touchB, setScene('OrFired', 1)], + events: [], + }, + ]; const { runtimeScene, aInsts, bInsts } = - runEventsWithThreeObjectsThreeInstances(buildEvents()); + runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); expect(touchedFlags(bInsts)).toEqual([0, 1, 0]); @@ -233,21 +207,23 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => /* touched. */ /* ================================================================== */ describe('Or — preserves outside picks when no true branch contributed', () => { - // Outside the Or, ObjectA is filtered to A2. Inside the Or: - // branch B: pickA=999 — references A but is false. - // branch C: free, true — does not reference A. - // No true branch contributed for A, so the bug-fixed Or leaves the - // outside pick of A2 alone. Only A2 should be touched. - const events = [ - { - type: 'BuiltinCommonInstructions::Standard', - conditions: [pickAByVar(2), orOf(pickAByVar(999), freeCondition(true))], - actions: [touchA, setScene('OrFired', 1)], - events: [], - }, - ]; - it('keeps the outside-Or pick (A2) when only the free branch is true', () => { + // Outside the Or, ObjectA is filtered to A2. Inside the Or: + // branch 1: pickA=999 — references A but is false. + // branch 2: free, true — does not reference A. + // No true branch contributed for A, so the bug-fixed Or leaves + // the outside pick of A2 alone. Only A2 should be touched. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + pickAByVar(2), + orOf(pickAByVar(999), freeCondition(true)), + ], + actions: [touchA, setScene('OrFired', 1)], + events: [], + }, + ]; const { runtimeScene, aInsts } = runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); @@ -255,12 +231,12 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => }); it('keeps the outside-Or pick (A2) when the free branch comes BEFORE the false A-branch', () => { - // Branch order matters: this test exercises the boolean-reset fix. - // Without it, the true free branch would leak its truth value into - // the next branch's "if(isConditionTrue)" check, and the false - // pickA(999) branch would be wrongly treated as a contribution - // — collapsing parent.A to the empty filtered list. - const eventsReversed = [ + // Branch order matters: this test exercises the boolean-reset + // fix. Without it, the true free branch would leak its truth + // value into the next branch's "if(isConditionTrue)" check, and + // the false pickA(999) branch would be wrongly treated as a + // contribution — collapsing parent.A to the empty filtered list. + const events = [ { type: 'BuiltinCommonInstructions::Standard', conditions: [ @@ -272,7 +248,7 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => }, ]; const { runtimeScene, aInsts } = - runEventsWithThreeObjectsThreeInstances(eventsReversed); + runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); }); @@ -283,26 +259,33 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => /* fails the parent event so the action does not run). */ /* ================================================================== */ describe('Or — all-false returns false', () => { - const buildEvents = (orWrapper) => [ - { - type: 'BuiltinCommonInstructions::Standard', - conditions: [orWrapper(pickAByVar(999), pickBByVar(999))], - actions: [touchA, touchB, setScene('OrFired', 1)], - events: [], - }, - ]; - it('Or: action does not run when both branches are false', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickAByVar(999), pickBByVar(999))], + actions: [touchA, touchB, setScene('OrFired', 1)], + events: [], + }, + ]; const { runtimeScene, aInsts, bInsts } = - runEventsWithThreeObjectsThreeInstances(buildEvents(orOf)); + runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().has('OrFired')).toBe(false); expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); }); it('OrDistributive: action does not run when both branches are false', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orDistributiveOf(pickAByVar(999), pickBByVar(999))], + actions: [touchA, touchB, setScene('OrFired', 1)], + events: [], + }, + ]; const { runtimeScene, aInsts, bInsts } = - runEventsWithThreeObjectsThreeInstances(buildEvents(orDistributiveOf)); + runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().has('OrFired')).toBe(false); expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); @@ -311,54 +294,50 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => /* ================================================================== */ /* 4. Input + SubmitButton — canonical case for OrDistributive. */ - /* A branch that doesn't reference ObjectA must keep ObjectA */ - /* available to the action. */ + /* A branch references A (and is false), the other branch does */ + /* not reference A. With OrDistributive every A is preserved at */ + /* action time; with the regular Or, parent.A collapses to empty. */ /* ================================================================== */ - describe('OrDistributive — Input/Button (act on ObjectA regardless)', () => { - // branch 1: pickA=999 — references A, false (no instance has var=999). - // branch 2: free, true — does not reference A. - // With OrDistributive, all three A instances stay available. - const events = [ - { - type: 'BuiltinCommonInstructions::Standard', - conditions: [orDistributiveOf(pickAByVar(999), freeCondition(true))], - actions: [touchA, setScene('OrFired', 1)], - events: [], - }, - ]; - - it('OrDistributive: all A instances stay picked when only the free branch is true', () => { + describe('OrDistributive vs Or — Input/Button (act on ObjectA regardless)', () => { + it('OrDistributive: every A stays picked when only the free branch is true', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orDistributiveOf(pickAByVar(999), freeCondition(true)), + ], + actions: [touchA, setScene('OrFired', 1)], + events: [], + }, + ]; const { runtimeScene, aInsts } = runEventsWithThreeObjectsThreeInstances(events); expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); expect(touchedFlags(aInsts)).toEqual([1, 1, 1]); }); - it('OrDistributive: when both branches true, every A is still picked (union over all branches)', () => { - const events2 = [ + it('OrDistributive: when both branches true, every A is still picked (union of constrained + unconstrained = all)', () => { + // Branch 1 contributes only A2. Branch 2 is unconstrained on A so + // it contributes the parent's full A list. Union = all three. + const events = [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [ - orDistributiveOf(pickAByVar(2), freeCondition(true)), - ], + conditions: [orDistributiveOf(pickAByVar(2), freeCondition(true))], actions: [touchA, setScene('OrFired', 1)], events: [], }, ]; - const { aInsts } = runEventsWithThreeObjectsThreeInstances(events2); - // Branch 1 contributes only A2. Branch 2 is unconstrained so it - // contributes the parent's full A list (A1, A2, A3). Union = all - // three. + const { aInsts } = runEventsWithThreeObjectsThreeInstances(events); expect(touchedFlags(aInsts)).toEqual([1, 1, 1]); }); - it('preserve-picks Or fails on this pattern (expected; documents why a separate condition exists)', () => { + it('preserve-picks Or fails on this pattern: action runs on zero A (documents why a separate condition is needed)', () => { // With the regular Or, branch 1 is false and branch 2 does not // reference A. No true branch contributed for A, so the bug-fixed // Or leaves A at whatever it was at the start of the Or — and - // because A is registered as "empty if just declared", that's an - // empty list. The action runs zero times. - const events3 = [ + // because A is registered by the Or as "empty if just declared", + // that's the empty list. The action runs zero times. + const events = [ { type: 'BuiltinCommonInstructions::Standard', conditions: [orOf(pickAByVar(999), freeCondition(true))], @@ -366,32 +345,55 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => events: [], }, ]; - const { aInsts } = runEventsWithThreeObjectsThreeInstances(events3); + const { runtimeScene, aInsts } = + runEventsWithThreeObjectsThreeInstances(events); + // The Or itself is true (the free branch is true), so the action + // block runs — but it sees no picked A. + expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); }); }); /* ================================================================== */ - /* 5. "Score on trigger" — OrDistributive lets unrelated objects be */ - /* available regardless of which branch fired. */ + /* 5. Distributive vs Or with the action acting on B, where exactly */ + /* one branch references B (and is false). This isolates the */ + /* distinguishing behavior on the action's target. */ /* ================================================================== */ - describe('OrDistributive — score-on-trigger (unrelated object stays available)', () => { - const events = [ - { - type: 'BuiltinCommonInstructions::Standard', - conditions: [orDistributiveOf(pickAByVar(2), freeCondition(true))], - // Action acts on B although neither branch references B. - actions: [touchB, setScene('OrFired', 1)], - events: [], - }, - ]; - - it('action acts on every B instance even though no branch references B', () => { - const { runtimeScene, bInsts } = - runEventsWithThreeObjectsThreeInstances(events); - expect(runtimeScene.getVariables().get('OrFired').getAsNumber()).toBe(1); + describe('OrDistributive vs Or — action on B, only one branch references B and is false', () => { + it('OrDistributive: every B is touched (the unconstrained free branch contributes parent.B)', () => { + // Branch 1 references B and fails (no B has var=999). Branch 2 + // is free and true. Distributive Or treats branch 2 as + // unconstrained on B, contributing parent's full B list. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orDistributiveOf(pickBByVar(999), freeCondition(true)), + ], + actions: [touchB], + events: [], + }, + ]; + const { bInsts } = runEventsWithThreeObjectsThreeInstances(events); expect(touchedFlags(bInsts)).toEqual([1, 1, 1]); }); + + it('Or: zero B is touched on the same shape (the false B-branch wipes parent.B)', () => { + // Same shape with the regular Or: parent.B starts as the + // "empty if just declared" list (because the Or registers B), + // branch 1 is false → no contribution for B, branch 2 does not + // reference B → no contribution. parent.B stays empty. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickBByVar(999), freeCondition(true))], + actions: [touchB], + events: [], + }, + ]; + const { bInsts } = runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); + }); }); /* ================================================================== */ @@ -399,19 +401,19 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => /* Or in the Door/Coin pattern. This test documents the leak. */ /* ================================================================== */ describe('OrDistributive — Door/Coin shows why both operators are needed', () => { - // Branch 1 picks A=2. Branch 2 picks B=999 (false). With distributive - // semantics, branch 1 leaves B unconstrained and contributes B's - // entire parent list, so the action ends up touching every B. - const events = [ - { - type: 'BuiltinCommonInstructions::Standard', - conditions: [orDistributiveOf(pickAByVar(2), pickBByVar(999))], - actions: [touchA, touchB], - events: [], - }, - ]; - it('only A=2 matches but distributive Or also touches every B (use regular Or for this case)', () => { + // Branch 1 picks A=2. Branch 2 picks B=999 (false). With + // distributive semantics, branch 1 leaves B unconstrained and + // contributes B's entire parent list, so the action ends up + // touching every B. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orDistributiveOf(pickAByVar(2), pickBByVar(999))], + actions: [touchA, touchB], + events: [], + }, + ]; const { aInsts, bInsts } = runEventsWithThreeObjectsThreeInstances(events); expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); @@ -427,10 +429,8 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => /* ================================================================== */ describe('Or — branches with And { picks A and B together }', () => { it('Or: each true And-branch contributes its own A and B picks; others untouched', () => { - // Branch 1: And{A=1, B=1} — both match → branch 1 true → contribute - // A1, B1. - // Branch 2: And{A=3, B=3} — both match → branch 2 true → contribute - // A3, B3. + // Branch 1: And{A=1, B=1} — both match → contribute A1, B1. + // Branch 2: And{A=3, B=3} — both match → contribute A3, B3. const events = [ { type: 'BuiltinCommonInstructions::Standard', @@ -452,8 +452,8 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => it('Or: And-branch is false if any sub-condition fails → no contribution from that branch', () => { // Branch 1: And{A=1, B=1} — both match → contribute A1, B1. - // Branch 2: And{A=2, B=999} — A=2 picks A2 but B=999 picks nothing, - // so And is false → no contribution. + // Branch 2: And{A=2, B=999} — A=2 picks A2 but B=999 picks + // nothing, so the And is false → no contribution. const events = [ { type: 'BuiltinCommonInstructions::Standard', @@ -475,10 +475,11 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => }); describe('OrDistributive — branches with And { picks A and B together }', () => { - it('OrDistributive: each true And-branch is symmetric to Or because every branch references both A and B', () => { - // When every branch references every object in the union, the - // unconstrained-fill of OrDistributive does not engage and the - // result is identical to the regular Or above. + it('OrDistributive: when every branch references both A and B, the result is identical to the regular Or', () => { + // The unconstrained-fill of OrDistributive only engages for an + // object that some branch leaves unreferenced. Here every branch + // references both A and B, so the result is identical to the + // regular Or above. const events = [ { type: 'BuiltinCommonInstructions::Standard', @@ -498,19 +499,20 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => expect(touchedFlags(bInsts)).toEqual([1, 0, 1]); }); - it('OrDistributive: asymmetric branches — branch with And{A=1} leaves B unconstrained, the lone B-branch leaves A unconstrained', () => { - // Branch 1: And{A=1} — references only A; B is unconstrained for - // this branch (contributes the parent's full B list when branch - // is true). - // Branch 2: pickB=2 — references only B; A is unconstrained for - // this branch (contributes the parent's full A list when branch - // is true). - // Both branches are true → every A and every B end up in the union. + it('OrDistributive: asymmetric branches — And{A=1,B=1} + pickA=3 → every B in the union, only the picked A', () => { + // Branch 1: And{A=1, B=1} — references both A and B, both true, + // contributes A1 + B1. + // Branch 2: pickA=3 — references only A, contributes A3 from + // its own pick AND parent's full B list (B is unconstrained + // on this branch). Union B = all three. const events = [ { type: 'BuiltinCommonInstructions::Standard', conditions: [ - orDistributiveOf(andOf(pickAByVar(1)), pickBByVar(2)), + orDistributiveOf( + andOf(pickAByVar(1), pickBByVar(1)), + pickAByVar(3) + ), ], actions: [touchA, touchB], events: [], @@ -518,94 +520,147 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => ]; const { aInsts, bInsts } = runEventsWithThreeObjectsThreeInstances(events); - expect(touchedFlags(aInsts)).toEqual([1, 1, 1]); + expect(touchedFlags(aInsts)).toEqual([1, 0, 1]); expect(touchedFlags(bInsts)).toEqual([1, 1, 1]); }); - it('OrDistributive: same asymmetric branches with the regular Or only touch the picked instances', () => { - // Same shape as above but with the preserve-picks Or — branches - // contribute only what they actually picked, so only A1 and B2 - // are touched. + it('Or: same asymmetric shape only touches the picked B (preserve-picks)', () => { + // Same branches as the asymmetric OrDistributive case above, but + // with the regular Or: branch 2 doesn't reference B, so it + // contributes nothing for B. Only A's union (A1, A3) and B1 + // survive. const events = [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [orOf(andOf(pickAByVar(1)), pickBByVar(2))], + conditions: [ + orOf(andOf(pickAByVar(1), pickBByVar(1)), pickAByVar(3)), + ], actions: [touchA, touchB], events: [], }, ]; const { aInsts, bInsts } = runEventsWithThreeObjectsThreeInstances(events); - expect(touchedFlags(aInsts)).toEqual([1, 0, 0]); - expect(touchedFlags(bInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(aInsts)).toEqual([1, 0, 1]); + expect(touchedFlags(bInsts)).toEqual([1, 0, 0]); }); }); /* ================================================================== */ - /* 8. Unrelated objects — picks for objects the Or doesn't reference */ - /* are never disturbed. ObjectC is picked by a sibling top-level */ - /* event whose conditions reference only C, and that should yield */ - /* the same C picks regardless of how the Or above behaved. */ + /* 8. Unrelated objects — within the same event, picks for objects */ + /* the Or doesn't reference must follow their own conditions */ + /* (and lack of conditions = unconstrained). The Or must not */ + /* register or alter the picked list of an object it doesn't */ + /* constrain. */ /* ================================================================== */ - describe('Or / OrDistributive — unrelated ObjectC is picked independently', () => { - const buildTwoEvents = (orWrapper) => [ - // Event 1 — exercises Or behavior on A and B. ObjectC is never - // mentioned in this event. - { - type: 'BuiltinCommonInstructions::Standard', - conditions: [orWrapper(pickAByVar(2), pickBByVar(2))], - actions: [touchA, touchB], - events: [], - }, - // Event 2 — sibling event picking ObjectC by its variable. This - // event has no relation to the Or above; its picks must be - // unaffected by what happened in event 1. - { - type: 'BuiltinCommonInstructions::Standard', - conditions: [pickCByVar(3)], - actions: [touchC], - events: [], - }, - ]; - - it('Or: ObjectC=3 is picked independently of the Or on A and B', () => { + describe('Or — unrelated ObjectC is untouched by the Or in the same event', () => { + it('no condition on C: action acts on every C even though the Or above on A/B is constrained', () => { + // The Or only references A and B. C has no condition, so the + // action should run on every C instance. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickAByVar(2), pickBByVar(999))], + actions: [touchA, touchB, touchC], + events: [], + }, + ]; const { aInsts, bInsts, cInsts } = - runEventsWithThreeObjectsThreeInstances(buildTwoEvents(orOf)); + runEventsWithThreeObjectsThreeInstances(events); expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); - expect(touchedFlags(bInsts)).toEqual([0, 1, 0]); - expect(touchedFlags(cInsts)).toEqual([0, 0, 1]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(cInsts)).toEqual([1, 1, 1]); }); - it('OrDistributive: ObjectC=3 is picked independently of the OrDistributive on A and B', () => { - const { cInsts } = runEventsWithThreeObjectsThreeInstances( - buildTwoEvents(orDistributiveOf) - ); - expect(touchedFlags(cInsts)).toEqual([0, 0, 1]); + it('C picked by a sibling condition BEFORE the Or: only the picked C is touched, the Or does not erase it', () => { + // pickC=2 picks C2 outside the Or. Then the Or runs (true via + // branch A). The Or does not reference C, so C2 must survive + // into the action. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [pickCByVar(2), orOf(pickAByVar(2), pickBByVar(999))], + actions: [touchA, touchC], + events: [], + }, + ]; + const { aInsts, cInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(cInsts)).toEqual([0, 1, 0]); }); - it('Or with Or false: ObjectC is still picked correctly by the sibling event', () => { - // The Or in event 1 is false, so event 1's actions do not run. The - // sibling event 2 picks ObjectC normally because event contexts - // are independent. + it('C picked by a sibling condition AFTER the Or: only the picked C is touched, the Or does not pre-empt it', () => { + // The Or runs first, then pickC=3 filters C. The Or must not + // have side-effected C's picked list in any way. const events = [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [orOf(pickAByVar(999), pickBByVar(999))], - actions: [touchA, touchB], + conditions: [orOf(pickAByVar(2), pickBByVar(999)), pickCByVar(3)], + actions: [touchA, touchC], events: [], }, + ]; + const { aInsts, cInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(cInsts)).toEqual([0, 0, 1]); + }); + }); + + describe('OrDistributive — unrelated ObjectC is untouched by the OrDistributive in the same event', () => { + it('no condition on C: action acts on every C even though OrDistributive above on A/B is constrained', () => { + const events = [ { type: 'BuiltinCommonInstructions::Standard', - conditions: [pickCByVar(1)], - actions: [touchC], + conditions: [orDistributiveOf(pickAByVar(2), pickBByVar(999))], + actions: [touchA, touchB, touchC], events: [], }, ]; const { aInsts, bInsts, cInsts } = runEventsWithThreeObjectsThreeInstances(events); - expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); - expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); - expect(touchedFlags(cInsts)).toEqual([1, 0, 0]); + // ObjectA picked from branch 1; ObjectB unconstrained on branch 1 + // (distributive) so all three B's contributed; C never referenced. + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(bInsts)).toEqual([1, 1, 1]); + expect(touchedFlags(cInsts)).toEqual([1, 1, 1]); + }); + + it('C picked by a sibling condition BEFORE the OrDistributive: only the picked C is touched', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + pickCByVar(2), + orDistributiveOf(pickAByVar(2), pickBByVar(999)), + ], + actions: [touchA, touchC], + events: [], + }, + ]; + const { aInsts, cInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(cInsts)).toEqual([0, 1, 0]); + }); + + it('C picked by a sibling condition AFTER the OrDistributive: only the picked C is touched', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orDistributiveOf(pickAByVar(2), pickBByVar(999)), + pickCByVar(3), + ], + actions: [touchA, touchC], + events: [], + }, + ]; + const { aInsts, cInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(cInsts)).toEqual([0, 0, 1]); }); }); }); From 9c1a0a5d1531bb360ea42faec3ca002f0aa9bf82 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 6 May 2026 23:01:19 +0000 Subject: [PATCH 05/12] Add edge-case Or picking tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds five new test categories addressing edge cases that weren't exercised by the existing patterns: - Empty Or / OrDistributive (zero sub-conditions): both must be false per the metadata; the parent event must fail and no action runs. - Inverted picking sub-condition inside Or / OrDistributive: object conditions like VarObjet, when inverted, pick the complement of the predicate. The Or must propagate that branch's complement picks correctly. (Direct `inverted: true` on Or/And/OrDistributive itself is a no-op in the existing custom lambdas — a pre-existing limitation, not introduced here. The canonical inversion path, Not wrapping Or, is already covered by the Boolean-operators test file.) - Parent pre-pick narrowing for OrDistributive: when an outside-Or condition narrows an object's picked list, OrDistributive must inherit the narrowed list rather than refilling from the scene. The unconstrained branch contributes the parent's already-narrowed list, so a pre-Or pickB=2 stays as [B2] after the OrDistributive. - Nested OrDistributive: an OrDistributive inside an And inside another Or / OrDistributive, with picking conditions at multiple depths. Verifies context depth and per-context object-list copies propagate correctly. Asserts on the per-instance Touched array. - Duplicate-instance union dedup: when two true branches both pick the same instance, the per-object indexOf check at union time must prevent a double-count, so the action runs once per instance (Touched=1, not 2). The other suggestions reviewed (all-branches-false OrDistributive, NOT-wrapping-Or for picking) are already covered: all-false is section 3 of this file; Not-wrapping is the canonical path tested by GDJSBooleanOperatorsCodeGenerationIntegrationTests.js. 33 tests in this file, all passing. --- ...ctPickingCodeGenerationIntegrationTests.js | 216 ++++++++++++++++++ 1 file changed, 216 insertions(+) diff --git a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js index 9d7851384ccc..af07124e95d9 100644 --- a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js +++ b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js @@ -663,4 +663,220 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => expect(touchedFlags(cInsts)).toEqual([0, 0, 1]); }); }); + + /* ================================================================== */ + /* 9. Empty Or / OrDistributive — per the metadata, an Or with no */ + /* sub-conditions is always false. Confirm the parent event fails */ + /* and no action runs. */ + /* ================================================================== */ + describe('Or / OrDistributive — empty (no sub-conditions) is always false', () => { + it('Or with zero sub-conditions: action does not run', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf()], + actions: [touchA, touchB, touchC, setScene('OrFired', 1)], + events: [], + }, + ]; + const { runtimeScene, aInsts, bInsts, cInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(runtimeScene.getVariables().has('OrFired')).toBe(false); + expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(cInsts)).toEqual([0, 0, 0]); + }); + + it('OrDistributive with zero sub-conditions: action does not run', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orDistributiveOf()], + actions: [touchA, touchB, touchC, setScene('OrFired', 1)], + events: [], + }, + ]; + const { runtimeScene, aInsts, bInsts, cInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(runtimeScene.getVariables().has('OrFired')).toBe(false); + expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); + expect(touchedFlags(cInsts)).toEqual([0, 0, 0]); + }); + }); + + /* ================================================================== */ + /* 10. Inverted sub-condition inside Or — for object-picking */ + /* conditions, the inversion picks the COMPLEMENT (instances */ + /* for which the predicate is false). The Or must propagate the */ + /* inverted branch's picks correctly. */ + /* */ + /* Note: setting `inverted: true` directly on Or/And/OrDistributive */ + /* itself is currently a no-op in GDevelop's custom lambdas (a */ + /* pre-existing limitation, not introduced by these changes); the */ + /* canonical way to invert an Or is to wrap it in a Not condition */ + /* and that path is covered by the existing Boolean-operators */ + /* test file. */ + /* ================================================================== */ + describe('Or / OrDistributive — inverted picking sub-condition picks the complement', () => { + const invertedPickAByVar = (v) => ({ + type: { inverted: true, value: 'VarObjet' }, + parameters: ['ObjectA', 'MyVariable', '=', String(v)], + }); + + it('Or { !pickA=2, false }: branch 1 picks A1 and A3 (complement of A=2)', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(invertedPickAByVar(2), freeCondition(false))], + actions: [touchA], + events: [], + }, + ]; + const { aInsts } = runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 0, 1]); + }); + + it('OrDistributive { !pickA=2, false }: branch 1 picks A1 and A3 (complement of A=2)', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orDistributiveOf(invertedPickAByVar(2), freeCondition(false)), + ], + actions: [touchA], + events: [], + }, + ]; + const { aInsts } = runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 0, 1]); + }); + }); + + /* ================================================================== */ + /* 11. Parent pre-pick narrowing — when an outside-Or condition */ + /* narrows an object's picked list, OrDistributive must inherit */ + /* the narrowed list (not refill from scene). The unconstrained */ + /* branch contributes the parent's already-narrowed list. */ + /* ================================================================== */ + describe('OrDistributive — respects parent pre-pick narrowing', () => { + it('OrDistributive { pickB=999 [false], free [true] } after outside pickB=2: only B2 stays picked', () => { + // Outside the OrDistributive, pickB=2 narrows parent.B to [B2]. + // Branch 1 references B and is false. Branch 2 is free (true) and + // unconstrained on B — it must contribute parent's narrowed B + // ([B2]), NOT the scene's full B list. After the OrDistributive, + // parent.B should still be [B2], so only B2 gets touched. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + pickBByVar(2), + orDistributiveOf(pickBByVar(999), freeCondition(true)), + ], + actions: [touchB], + events: [], + }, + ]; + const { bInsts } = runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(bInsts)).toEqual([0, 1, 0]); + }); + }); + + /* ================================================================== */ + /* 12. Nested OrDistributive — an OrDistributive inside an And */ + /* inside another OrDistributive must propagate context depth */ + /* and object-list copies correctly. */ + /* ================================================================== */ + describe('Or / OrDistributive — nested operators', () => { + it('Or wrapping And { pickA=1, inner OrDistributive { pickB=1, pickB=2 } }: inner OD union {B1,B2}, outer And true → A1, B1, B2 touched', () => { + // Outer Or has a single branch: And { pickA=1, OrDistributive { pickB=1, pickB=2 } }. + // The inner OrDistributive's two branches both reference B and + // pick B1 and B2 respectively → its union is [B1, B2]. The And's + // pickA=1 picks A1. Both And-children are true → outer branch + // contributes A1 and B1, B2. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orOf( + andOf( + pickAByVar(1), + orDistributiveOf(pickBByVar(1), pickBByVar(2)) + ) + ), + ], + actions: [touchA, touchB], + events: [], + }, + ]; + const { aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 0, 0]); + expect(touchedFlags(bInsts)).toEqual([1, 1, 0]); + }); + + it('OrDistributive wrapping And { pickA=1, inner OrDistributive { pickB=1, pickB=2 } } + pickA=3: distributive outer leaks unconstrained B for branch 2', () => { + // Outer is OrDistributive with two branches: + // branch 1: And { pickA=1, inner OrDistributive { pickB=1, pickB=2 } } + // → contributes A1 and B1, B2. + // branch 2: pickA=3 → references only A; B is unconstrained + // on this branch, so the parent's full B list (all + // three) is contributed. + // Final A = {A1, A3}; final B = union → all three. + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + orDistributiveOf( + andOf( + pickAByVar(1), + orDistributiveOf(pickBByVar(1), pickBByVar(2)) + ), + pickAByVar(3) + ), + ], + actions: [touchA, touchB], + events: [], + }, + ]; + const { aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 0, 1]); + expect(touchedFlags(bInsts)).toEqual([1, 1, 1]); + }); + }); + + /* ================================================================== */ + /* 13. Duplicate-instance union — two true branches that both pick */ + /* the same instance must not double-count it. The action is */ + /* declared once so the Touched variable should be 1 (not 2), */ + /* proving the per-object indexOf dedup at union time. */ + /* ================================================================== */ + describe('Or / OrDistributive — duplicate-instance union does not double-count', () => { + it('Or { pickA=2, pickA=2 }: A2 only touched once', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickAByVar(2), pickAByVar(2))], + actions: [touchA], + events: [], + }, + ]; + const { aInsts } = runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + }); + + it('OrDistributive { pickA=2, pickA=2 }: A2 only touched once', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orDistributiveOf(pickAByVar(2), pickAByVar(2))], + actions: [touchA], + events: [], + }, + ]; + const { aInsts } = runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + }); + }); }); From 908c6620b5a94072d122e342c3102afafc95d589 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 7 May 2026 21:03:06 +0000 Subject: [PATCH 06/12] Pin down Or vs OrDistributive residual difference (row 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a regression test pair for the canonical residual non- equivalence between fixed Or and OrDistributive even when an outside-Or condition pre-narrows the object: a true narrowing X-referencing branch combined with a true free branch. Outside narrows A to {A1, A2}. Branch 1 (pickA=1) narrows to {A1}. Branch 2 (free, true) does not reference A. - Or: parent.A narrows further to {A1} — branch-1 pick wins. - OrDistributive: parent.A stays at {A1, A2} — branch 2 contributes the parent's pre-pick unchanged, so the union is the outside pre-pick. This is the only "★" residual difference in the Setup B truth table; rows 13 / 15 are elaborations of the same shape. Also promotes the local invertedPickAByVar helper to a top-level shared helper so future tests on the complement-picking path can reuse it. --- ...ctPickingCodeGenerationIntegrationTests.js | 65 +++++++++++++++++-- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js index af07124e95d9..74381d0da47d 100644 --- a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js +++ b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js @@ -105,6 +105,14 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => const pickBByVar = (v) => pickByVar('ObjectB', v); const pickCByVar = (v) => pickByVar('ObjectC', v); + // Inverted picking: keeps the *complement* (instances where MyVariable + // is NOT equal to the supplied value). + const invertedPickByVar = (objectName, expectedValue) => ({ + type: { inverted: true, value: 'VarObjet' }, + parameters: [objectName, 'MyVariable', '=', String(expectedValue)], + }); + const invertedPickAByVar = (v) => invertedPickByVar('ObjectA', v); + // Free condition with constant truth value (does not reference any object). const freeCondition = (alwaysTrue) => ({ type: { value: 'Egal' }, @@ -719,11 +727,6 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => /* test file. */ /* ================================================================== */ describe('Or / OrDistributive — inverted picking sub-condition picks the complement', () => { - const invertedPickAByVar = (v) => ({ - type: { inverted: true, value: 'VarObjet' }, - parameters: ['ObjectA', 'MyVariable', '=', String(v)], - }); - it('Or { !pickA=2, false }: branch 1 picks A1 and A3 (complement of A=2)', () => { const events = [ { @@ -879,4 +882,56 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); }); }); + + /* ================================================================== */ + /* 14. Residual non-equivalence between Or and OrDistributive even */ + /* when an outside-Or condition pre-narrows X — pins down the */ + /* "★ row 8" of the Setup B truth table. */ + /* */ + /* Outside narrows ObjectA to {A1, A2} (= S0). Then: */ + /* branch 1: pickA=1 — references A, true, narrows to {A1}. */ + /* branch 2: free, true — does not reference A. */ + /* */ + /* - Or commits to the narrower branch-1 pick: parent.A = {A1}. */ + /* - OrDistributive un-narrows back to S0: branch 2 contributes */ + /* parent.A unchanged, so the union is {A1, A2}. */ + /* */ + /* This case maps to the original Door/Coin vs Input/Button */ + /* intent split at the "subset of subset" scale; it is the only */ + /* residual difference between fixed Or and OrDistributive when */ + /* parent already has a pre-pick. */ + /* ================================================================== */ + describe('Or vs OrDistributive — residual non-equivalence even with outside pre-pick', () => { + it('Or: a true narrowing X-ref branch + a true free branch → parent.A narrows further to {A1}', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + invertedPickAByVar(3), // narrow A to {A1, A2} + orOf(pickAByVar(1), freeCondition(true)), + ], + actions: [touchA], + events: [], + }, + ]; + const { aInsts } = runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 0, 0]); + }); + + it('OrDistributive: same shape → parent.A stays at the outside pre-pick {A1, A2}', () => { + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + invertedPickAByVar(3), // narrow A to {A1, A2} + orDistributiveOf(pickAByVar(1), freeCondition(true)), + ], + actions: [touchA], + events: [], + }, + ]; + const { aInsts } = runEventsWithThreeObjectsThreeInstances(events); + expect(touchedFlags(aInsts)).toEqual([1, 1, 0]); + }); + }); }); From 806613f67064e5740271d7c36d5a0f7292a6ff98 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 09:49:26 +0000 Subject: [PATCH 07/12] Add useDeprecatedOrConditionPicking project flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a backwards-compatibility flag for the "Or" condition object- picking fix landed earlier in this branch, mirroring the pattern of useDeprecatedZeroAsDefaultZOrder and useDeprecatedZeroAsDefaultStringVariable. When the flag is true, the runtime restores the pre-5.6.269 "Or" behavior: the final copy of the per-object union list to the parent's picked list is unconditional, so a branch that picks zero instances (or an Or branch composition where no true branch contributed for a given object) wipes the parent's outside-Or pick. Old projects rely on this behavior in spots and must keep producing the same runtime result without a hand-edit. When the flag is false (the default for new projects), the bug-fix path is taken: the final copy is gated on a per-object hasContrib flag set when at least one true branch referenced that object. Wiring: - Core/GDCore/Project/Project.{h,cpp}: storage, getter, setter, copy, serialization. Deserialization sets the flag to true automatically for projects authored on GD <= 5.6.268, so existing games keep their semantics on first load; explicitly-set values from project files override that. - GDevelop.js/Bindings/Bindings.idl: getter/setter exported to JS so the IDE can read/toggle the flag. - GDJS/Runtime/runtimegame.ts: declares `gdjs.useDeprecatedOrConditionPicking` (default false) and sets it on game startup from `data.properties.useDeprecatedOrConditionPicking`. - GDJS/Runtime/types/project-data.d.ts: types the new property. - GDJS/.../CommonInstructionsExtension.cpp: the Or codegen's final copy now reads `if (gdjs.useDeprecatedOrConditionPicking || hasContribX) copy(...)` so flipping the runtime flag forces the legacy unconditional copy. OrDistributive is intentionally untouched — it's a separate condition with its own semantics, and authors who use it have opted into the new behavior. - GDevelop.js/TestUtils/GDJSMocks.js: exposes the flag on the mock `gdjs` object so tests can flip it. - GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js: adds four test cases pinning the flag's behavior — flag OFF preserves the AllowedArea outside-Or pick (bug fix); flag ON wipes it (legacy); Door/Coin (which the pre-fix Or already handled correctly) is unchanged by the flag in either direction. 39 tests in the file, all passing. Full repo: 1191 passed. --- Core/GDCore/Project/Project.cpp | 19 +++ Core/GDCore/Project/Project.h | 27 ++++ .../Builtin/CommonInstructionsExtension.cpp | 14 ++- GDJS/Runtime/runtimegame.ts | 15 +++ GDJS/Runtime/types/project-data.d.ts | 1 + GDevelop.js/Bindings/Bindings.idl | 2 + GDevelop.js/TestUtils/GDJSMocks.js | 5 + ...ctPickingCodeGenerationIntegrationTests.js | 115 ++++++++++++++++++ GDevelop.js/types.d.ts | 2 + GDevelop.js/types/gdproject.js | 2 + 10 files changed, 199 insertions(+), 3 deletions(-) diff --git a/Core/GDCore/Project/Project.cpp b/Core/GDCore/Project/Project.cpp index f7c9a7042936..0f9ba4457503 100644 --- a/Core/GDCore/Project/Project.cpp +++ b/Core/GDCore/Project/Project.cpp @@ -67,6 +67,7 @@ Project::Project() projectUuid(""), useDeprecatedZeroAsDefaultZOrder(false), useDeprecatedZeroAsDefaultStringVariable(false), + useDeprecatedOrConditionPicking(false), isPlayableWithKeyboard(false), isPlayableWithGamepad(false), isPlayableWithMobile(false), @@ -803,6 +804,17 @@ void Project::UnserializeFrom(const SerializerElement& element) { } // end of compatibility code + // Compatibility with GD <= 5.6.268 + if (VersionWrapper::IsOlderOrEqual( + gdMajorVersion, gdMinorVersion, gdBuildVersion, 0, 5, 6, 268, 0) && + !propElement.HasAttribute("useDeprecatedOrConditionPicking")) { + useDeprecatedOrConditionPicking = true; + } else { + useDeprecatedOrConditionPicking = propElement.GetBoolAttribute( + "useDeprecatedOrConditionPicking", false); + } + // end of compatibility code + // Compatibility with GD <= 5.0.0-beta101 if (!propElement.HasAttribute("projectUuid") && !propElement.HasChild("projectUuid")) { @@ -1174,6 +1186,12 @@ void Project::SerializeTo(SerializerElement& element) const { } // end of compatibility code + // Compatibility with GD <= 5.6.268 + if (useDeprecatedOrConditionPicking) { + propElement.SetAttribute("useDeprecatedOrConditionPicking", true); + } + // end of compatibility code + extensionProperties.SerializeTo(propElement.AddChild("extensionProperties")); SerializerElement& platformsElement = propElement.AddChild("platforms"); @@ -1305,6 +1323,7 @@ void Project::Init(const gd::Project& game) { useDeprecatedZeroAsDefaultZOrder = game.useDeprecatedZeroAsDefaultZOrder; useDeprecatedZeroAsDefaultStringVariable = game.useDeprecatedZeroAsDefaultStringVariable; + useDeprecatedOrConditionPicking = game.useDeprecatedOrConditionPicking; author = game.author; authorIds = game.authorIds; diff --git a/Core/GDCore/Project/Project.h b/Core/GDCore/Project/Project.h index 95ae2685edf9..aa1cefd47a64 100644 --- a/Core/GDCore/Project/Project.h +++ b/Core/GDCore/Project/Project.h @@ -454,6 +454,25 @@ class GD_CORE_API Project { useDeprecatedZeroAsDefaultStringVariable = enable; } + /** + * \brief Check if the project should use the deprecated "Or" condition + * object-picking semantics (the pre-5.6.269 behavior, in which the Or + * unconditionally overwrote the parent event's picked object lists with + * the union of branch contributions, including when no branch actually + * contributed for a given object — wiping outside-Or picks). + */ + bool GetUseDeprecatedOrConditionPicking() const { + return useDeprecatedOrConditionPicking; + } + + /** + * \brief Set whether the project should use the deprecated "Or" condition + * object-picking semantics (the pre-5.6.269 behavior). + */ + void SetUseDeprecatedOrConditionPicking(bool enable) { + useDeprecatedOrConditionPicking = enable; + } + /** * \brief Change the project UUID. */ @@ -1147,6 +1166,14 @@ class GD_CORE_API Project { ///< no stored value default to "0" ///< at runtime (behavior before ///< 5.6.267). + bool useDeprecatedOrConditionPicking = + false; ///< If true, the "Or" condition uses + ///< the pre-5.6.269 picking semantics + ///< (always overwrite parent's picked + ///< list with the union of branch + ///< contributions, even when no branch + ///< contributed for a given object — + ///< wiping outside-Or picks). std::vector > scenes; ///< List of all scenes gd::VariablesContainer variables; ///< Initial global variables gd::ObjectsContainer objectsContainer; diff --git a/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp b/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp index 29db2b6bfe61..d699f67c6ef1 100644 --- a/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp +++ b/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp @@ -377,13 +377,21 @@ CommonInstructionsExtension::CommonInstructionsExtension() { // contribution (a true branch that actually referenced the object). // If no true branch contributed, leave the parent's picked list // untouched so picks established before the Or are preserved. + // + // Backwards compatibility: projects from before 5.6.269 expect the + // old "always overwrite parent.X with final.X" semantics. The + // runtime flag `gdjs.useDeprecatedOrConditionPicking` is set from + // the project's `useDeprecatedOrConditionPicking` property, and + // when true forces the unconditional overwrite — reproducing the + // pre-fix behavior for those projects. code += "{\n"; for (set::iterator it = emptyListsNeeded.begin(); it != emptyListsNeeded.end(); ++it) { gd::String finalObjList = finalListName(*it); - code += "if (" + hasContribName(*it) + ") gdjs.copyArray(" + - finalObjList + ", " + - codeGenerator.GetObjectListName(*it, parentContext) + ");\n"; + code += "if (gdjs.useDeprecatedOrConditionPicking || " + + hasContribName(*it) + ") gdjs.copyArray(" + finalObjList + + ", " + codeGenerator.GetObjectListName(*it, parentContext) + + ");\n"; } code += "}\n"; diff --git a/GDJS/Runtime/runtimegame.ts b/GDJS/Runtime/runtimegame.ts index 169137fcd596..b5d735ac9feb 100644 --- a/GDJS/Runtime/runtimegame.ts +++ b/GDJS/Runtime/runtimegame.ts @@ -9,6 +9,19 @@ namespace gdjs { const sleep = (ms: float) => new Promise((resolve) => setTimeout(resolve, ms)); + /** + * If true, the "Or" sub-condition uses the deprecated pre-5.6.269 + * object-picking semantics: it unconditionally overwrites the parent + * event's picked object lists with the union of branch contributions, + * even when no branch contributed for a given object — wiping + * outside-Or picks. Set by `RuntimeGame` on startup based on the + * project's `useDeprecatedOrConditionPicking` property. + * + * The generated event code reads this flag at the final copy step of + * the Or so old projects keep their existing behavior at runtime. + */ + export let useDeprecatedOrConditionPicking: boolean = false; + /** * Identify a script file, with its content hash (useful for hot-reloading). * @category Core Engine > Game @@ -309,6 +322,8 @@ namespace gdjs { this._updateSceneAndExtensionsData(); gdjs.Variable.useDeprecatedZeroAsDefaultStringVariable = !!data.properties.useDeprecatedZeroAsDefaultStringVariable; + gdjs.useDeprecatedOrConditionPicking = + !!data.properties.useDeprecatedOrConditionPicking; this._sceneResourcesPreloading = this._data.properties.sceneResourcesPreloading || 'at-startup'; diff --git a/GDJS/Runtime/types/project-data.d.ts b/GDJS/Runtime/types/project-data.d.ts index e8d4a6a8cac3..fed6e76906f8 100644 --- a/GDJS/Runtime/types/project-data.d.ts +++ b/GDJS/Runtime/types/project-data.d.ts @@ -559,6 +559,7 @@ declare interface ProjectPropertiesData { extensionProperties: Array; useDeprecatedZeroAsDefaultZOrder?: boolean; useDeprecatedZeroAsDefaultStringVariable?: boolean; + useDeprecatedOrConditionPicking?: boolean; projectUuid?: string; sceneResourcesPreloading?: 'at-startup' | 'never'; sceneResourcesUnloading?: 'at-scene-exit' | 'never'; diff --git a/GDevelop.js/Bindings/Bindings.idl b/GDevelop.js/Bindings/Bindings.idl index 53a9c97893ed..a8e23a1b3163 100644 --- a/GDevelop.js/Bindings/Bindings.idl +++ b/GDevelop.js/Bindings/Bindings.idl @@ -609,6 +609,8 @@ interface Project { boolean GetUseDeprecatedZeroAsDefaultZOrder(); void SetUseDeprecatedZeroAsDefaultStringVariable(boolean enable); boolean GetUseDeprecatedZeroAsDefaultStringVariable(); + void SetUseDeprecatedOrConditionPicking(boolean enable); + boolean GetUseDeprecatedOrConditionPicking(); boolean AreEffectsHiddenInEditor(); void SetEffectsHiddenInEditor(boolean enable); diff --git a/GDevelop.js/TestUtils/GDJSMocks.js b/GDevelop.js/TestUtils/GDJSMocks.js index 4b82d0c2527c..d0e12b505aa0 100644 --- a/GDevelop.js/TestUtils/GDJSMocks.js +++ b/GDevelop.js/TestUtils/GDJSMocks.js @@ -1080,6 +1080,11 @@ function makeMinimalGDJSMock(options) { ManuallyResolvableTask, Variable, VariablesContainer, + // Deprecated-Or-picking compatibility flag, defaults to false here. + // Tests that need to exercise the pre-5.6.269 picking semantics flip + // this on the returned `gdjs` object before calling the compiled + // events, and reset it afterwards. + useDeprecatedOrConditionPicking: false, }, mocks: { runRuntimeScenePreEventsCallbacks: () => { diff --git a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js index 74381d0da47d..e29765a71586 100644 --- a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js +++ b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationTests.js @@ -934,4 +934,119 @@ describe('libGD.js - GDJS Or object picking semantics integration tests', () => expect(touchedFlags(aInsts)).toEqual([1, 1, 0]); }); }); + + /* ================================================================== */ + /* 15. Backwards compatibility — the project flag */ + /* `useDeprecatedOrConditionPicking` (set automatically for */ + /* projects from before 5.6.269) restores the pre-fix Or behavior */ + /* of unconditionally overwriting the parent's picked list with */ + /* the union of branch contributions, including empty ones. */ + /* */ + /* The runtime flag is read at the Or's final copy step. With */ + /* it set, parent.A gets wiped to ∅ in the AllowedArea pattern */ + /* (the original bug); with it unset (the default), the bug fix */ + /* preserves parent.A. The flag does not affect OrDistributive, */ + /* which is a separate condition with its own semantics. */ + /* ================================================================== */ + describe('Or — useDeprecatedOrConditionPicking project flag', () => { + // Run the supplied events with the flag temporarily flipped on, then + // restore so the rest of the suite stays in default-mode. + const runWithDeprecatedFlag = (events) => { + const serializerElement = gd.Serializer.fromJSObject(events); + const runCompiledEvents = generateCompiledEventsFromSerializedEvents( + gd, + serializerElement, + { + parameterTypes: { ObjectA: 'object', ObjectB: 'object', ObjectC: 'object' }, + logCode: false, + } + ); + const { gdjs, runtimeScene } = makeMinimalGDJSMock(); + + const aInsts = [1, 2, 3].map((value) => { + const obj = runtimeScene.createObject('ObjectA'); + obj.getVariables().get('MyVariable').setNumber(value); + return obj; + }); + const aLists = new gdjs.Hashtable(); + aLists.put('ObjectA', aInsts.slice()); + + const bInsts = [1, 2, 3].map((value) => { + const obj = runtimeScene.createObject('ObjectB'); + obj.getVariables().get('MyVariable').setNumber(value); + return obj; + }); + const bLists = new gdjs.Hashtable(); + bLists.put('ObjectB', bInsts.slice()); + + const cInsts = [1, 2, 3].map((value) => { + const obj = runtimeScene.createObject('ObjectC'); + obj.getVariables().get('MyVariable').setNumber(value); + return obj; + }); + const cLists = new gdjs.Hashtable(); + cLists.put('ObjectC', cInsts.slice()); + + gdjs.useDeprecatedOrConditionPicking = true; + try { + runCompiledEvents(gdjs, runtimeScene, [aLists, bLists, cLists]); + } finally { + gdjs.useDeprecatedOrConditionPicking = false; + } + return { aInsts, bInsts, cInsts }; + }; + + // Same shape as test 2 above (AllowedArea preservation): outside + // narrows A to A2; the Or is true via a free branch; an X-ref + // branch is false. With the flag OFF (default), the fix preserves + // A2. With the flag ON (legacy), the Or wipes A → ∅ — reproducing + // the pre-5.6.269 bug for projects that depend on it. + const allowedAreaEvents = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [ + pickAByVar(2), + orOf(pickAByVar(999), freeCondition(true)), + ], + actions: [touchA], + events: [], + }, + ]; + + it('flag OFF (default): bug-fix path — outside-Or pick of A2 is preserved', () => { + const { aInsts } = + runEventsWithThreeObjectsThreeInstances(allowedAreaEvents); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + }); + + it('flag ON: pre-fix legacy path — outside-Or pick of A2 is wiped (action sees no A)', () => { + const { aInsts } = runWithDeprecatedFlag(allowedAreaEvents); + expect(touchedFlags(aInsts)).toEqual([0, 0, 0]); + }); + + // Door/Coin pattern is unchanged by the flag — the pre-fix Or + // already produced the right result here, so flipping the flag must + // not regress it. + const doorCoinEvents = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(pickAByVar(2), pickBByVar(999))], + actions: [touchA, touchB], + events: [], + }, + ]; + + it('flag OFF: Door/Coin only-A-true → only A2 touched', () => { + const { aInsts, bInsts } = + runEventsWithThreeObjectsThreeInstances(doorCoinEvents); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); + }); + + it('flag ON: Door/Coin only-A-true → still only A2 touched (no regression on this pattern)', () => { + const { aInsts, bInsts } = runWithDeprecatedFlag(doorCoinEvents); + expect(touchedFlags(aInsts)).toEqual([0, 1, 0]); + expect(touchedFlags(bInsts)).toEqual([0, 0, 0]); + }); + }); }); diff --git a/GDevelop.js/types.d.ts b/GDevelop.js/types.d.ts index 5712f7deed46..26c70fa25558 100644 --- a/GDevelop.js/types.d.ts +++ b/GDevelop.js/types.d.ts @@ -597,6 +597,8 @@ export class Project extends EmscriptenObject { getUseDeprecatedZeroAsDefaultZOrder(): boolean; setUseDeprecatedZeroAsDefaultStringVariable(enable: boolean): void; getUseDeprecatedZeroAsDefaultStringVariable(): boolean; + setUseDeprecatedOrConditionPicking(enable: boolean): void; + getUseDeprecatedOrConditionPicking(): boolean; areEffectsHiddenInEditor(): boolean; setEffectsHiddenInEditor(enable: boolean): void; setLastCompilationDirectory(path: string): void; diff --git a/GDevelop.js/types/gdproject.js b/GDevelop.js/types/gdproject.js index eb689fade349..63f9af61dbcf 100644 --- a/GDevelop.js/types/gdproject.js +++ b/GDevelop.js/types/gdproject.js @@ -54,6 +54,8 @@ declare class gdProject { getUseDeprecatedZeroAsDefaultZOrder(): boolean; setUseDeprecatedZeroAsDefaultStringVariable(enable: boolean): void; getUseDeprecatedZeroAsDefaultStringVariable(): boolean; + setUseDeprecatedOrConditionPicking(enable: boolean): void; + getUseDeprecatedOrConditionPicking(): boolean; areEffectsHiddenInEditor(): boolean; setEffectsHiddenInEditor(enable: boolean): void; setLastCompilationDirectory(path: string): void; From 039ff1d325217e822672bebe231119c8e38a7a63 Mon Sep 17 00:00:00 2001 From: Florian Rival Date: Fri, 8 May 2026 15:56:55 +0200 Subject: [PATCH 08/12] Optimize Or with many objects --- .../Builtin/CommonInstructionsExtension.cpp | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp b/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp index d699f67c6ef1..991b4b222081 100644 --- a/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp +++ b/GDJS/GDJS/Extensions/Builtin/CommonInstructionsExtension.cpp @@ -326,15 +326,19 @@ CommonInstructionsExtension::CommonInstructionsExtension() { emptyListsNeeded.insert(*it); gd::String objList = codeGenerator.GetObjectListName(*it, context); gd::String finalObjList = finalListName(*it); + gd::String finalObjSet = finalObjList + "Set"; // Mark that this object got a contribution from a true branch // that referenced it. conditionsCode += " " + hasContribName(*it) + " = true;\n"; conditionsCode += " for (let j = 0, jLen = " + objList + ".length; j < jLen ; ++j) {\n"; - conditionsCode += " if ( " + finalObjList + ".indexOf(" + - objList + "[j]) === -1 )\n"; + conditionsCode += " if (!" + finalObjSet + ".has(" + + objList + "[j])) {\n"; + conditionsCode += " " + finalObjSet + ".add(" + + objList + "[j]);\n"; conditionsCode += " " + finalObjList + ".push(" + objList + "[j]);\n"; + conditionsCode += " }\n"; conditionsCode += " }\n"; } conditionsCode += "}\n"; @@ -356,8 +360,11 @@ CommonInstructionsExtension::CommonInstructionsExtension() { // incidence on further conditions, as conditions use "normal" // ones. gd::String finalObjList = finalListName(*it); + gd::String finalObjSet = finalObjList + "Set"; codeGenerator.AddGlobalDeclaration(finalObjList + " = [];\n"); + codeGenerator.AddGlobalDeclaration(finalObjSet + " = new Set();\n"); declarationsCode += finalObjList + ".length = 0;\n"; + declarationsCode += finalObjSet + ".clear();\n"; // Per-object contribution flag, reset at the start of every Or. codeGenerator.AddGlobalDeclaration(hasContribName(*it) + " = false;\n"); declarationsCode += hasContribName(*it) + " = false;\n"; @@ -495,12 +502,16 @@ CommonInstructionsExtension::CommonInstructionsExtension() { gd::String objList = codeGenerator.GetObjectListName(objectName, context); gd::String finalObjList = finalListName(objectName); + gd::String finalObjSet = finalObjList + "Set"; conditionsCode += " for (let j = 0, jLen = " + objList + ".length; j < jLen ; ++j) {\n"; - conditionsCode += " if ( " + finalObjList + - ".indexOf(" + objList + "[j]) === -1 )\n"; + conditionsCode += " if (!" + finalObjSet + ".has(" + + objList + "[j])) {\n"; + conditionsCode += " " + finalObjSet + ".add(" + + objList + "[j]);\n"; conditionsCode += " " + finalObjList + ".push(" + objList + "[j]);\n"; + conditionsCode += " }\n"; conditionsCode += " }\n"; } conditionsCode += "}\n"; @@ -511,8 +522,11 @@ CommonInstructionsExtension::CommonInstructionsExtension() { gd::String declarationsCode; for (auto &objectName : allReferencedObjects) { gd::String finalObjList = finalListName(objectName); + gd::String finalObjSet = finalObjList + "Set"; codeGenerator.AddGlobalDeclaration(finalObjList + " = [];\n"); + codeGenerator.AddGlobalDeclaration(finalObjSet + " = new Set();\n"); declarationsCode += finalObjList + ".length = 0;\n"; + declarationsCode += finalObjSet + ".clear();\n"; } declarationsCode += "let " + codeGenerator.GenerateBooleanFullName( From 9324e924caed008ab79cefd6cfdb1a90cb4a3dbb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 14:19:34 +0000 Subject: [PATCH 09/12] Add stress tests for the Or / OrDistributive Set-based dedup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds GDJSOrObjectPickingCodeGenerationIntegrationStressTests.js with six scaling correctness tests that exercise the per-object union dedup heavily. The dedup loop pushes each branch's contributed instances into the per-object "final" list while skipping ones already present; the codegen now uses a Set for the membership check, turning the overall work for K branches × N instances from O(K · N²) into O(K · N). Each test asserts on the per-instance Touched variable so the dedup is verified for correctness at scale, not just for runtime: 1. 10 identical Or-branches × 1000 instances each → every instance touched exactly once (full overlap; 10 000 dedup attempts). 2. OrDistributive with 5 branches × 5 objects × 500 instances per object → every object fully picked once (12 500 dedup attempts per Or, deduped to 2500). 3. 10 partition Or-branches × 200 instances each (no overlap) → every instance touched once across the K · N = 2000-instance union. 4. OrDistributive with 1 picking branch + 19 free branches × 500 instances → free branches' unconstrained-fill of parent.A is deduped K · N times, all collapsing to N = 500 touches. 5. 8 Or-branches each an And{ pickA=i, pickB=i } over 250 A and 250 B per branch → both per-object dedups (A and B) stay independent and correct at scale. 6. 100 redundant branches all picking the same single instance → Touched = 1 (smoke test for many redundant pushes). Per-test runtime is dominated by setup and emitted-event eval, not by the dedup itself; the suite as a whole completes in ~2 s. --- ...ingCodeGenerationIntegrationStressTests.js | 333 ++++++++++++++++++ 1 file changed, 333 insertions(+) create mode 100644 GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationStressTests.js diff --git a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationStressTests.js b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationStressTests.js new file mode 100644 index 000000000000..d58861db2ef0 --- /dev/null +++ b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationStressTests.js @@ -0,0 +1,333 @@ +/** + * Stress tests for the object-picking semantics of the "Or" and + * "OrDistributive" sub-event conditions, focused on the per-object + * union dedup that runs at the end of every Or branch. + * + * The dedup loop pushes each branch's contributed instances into the + * per-object "final" list while skipping ones already present. The + * Or codegen uses a Set to do the "already present" check in O(1), + * which turns the overall dedup work for an Or with K branches each + * contributing N instances from O(K · N²) (pre-optimization, when the + * check was an `indexOf` scan over the growing final array) to + * O(K · N). + * + * The tests below pick K and N high enough that the difference between + * O(K · N²) and O(K · N) is meaningful (millions of comparisons vs. + * thousands of Set ops). Each test asserts on the per-instance Touched + * variable so the dedup at scale is verified for correctness, not just + * for runtime. + */ + +const initializeGDevelopJs = require('../../Binaries/embuild/GDevelop.js/libGD.js'); +const { makeMinimalGDJSMock } = require('../TestUtils/GDJSMocks'); +const { + generateCompiledEventsFromSerializedEvents, +} = require('../TestUtils/CodeGenerationHelpers.js'); + +describe('libGD.js - GDJS Or object picking stress tests', () => { + let gd = null; + beforeAll(async () => { + gd = await initializeGDevelopJs(); + }); + + /** + * Generic stress-test fixture. Pass an object spec of the form + * { ObjectName: arrayOfMyVariableValuesPerInstance, ... } + * and the supplied events run with `ObjectName` parameters and the + * matching number of pre-picked instances. Returns the runtime scene + * and the per-object instance arrays so tests can inspect each + * instance's Touched variable. + */ + const runEventsWithObjects = (events, objectsSpec) => { + const serializerElement = gd.Serializer.fromJSObject(events); + const parameterTypes = {}; + for (const objectName in objectsSpec) { + parameterTypes[objectName] = 'object'; + } + const runCompiledEvents = generateCompiledEventsFromSerializedEvents( + gd, + serializerElement, + { parameterTypes, logCode: false } + ); + + const { gdjs, runtimeScene } = makeMinimalGDJSMock(); + + const allInsts = {}; + const argLists = []; + for (const objectName in objectsSpec) { + const insts = objectsSpec[objectName].map((value) => { + const obj = runtimeScene.createObject(objectName); + obj.getVariables().get('MyVariable').setNumber(value); + return obj; + }); + const lists = new gdjs.Hashtable(); + // Pre-pick every instance so the picked list starts populated. + lists.put(objectName, insts.slice()); + allInsts[objectName] = insts; + argLists.push(lists); + } + + runCompiledEvents(gdjs, runtimeScene, argLists); + return { runtimeScene, allInsts }; + }; + + const touchedFlags = (insts) => + insts.map((o) => o.getVariables().get('Touched').getAsNumber()); + + const pickByVar = (objectName, expectedValue) => ({ + type: { value: 'VarObjet' }, + parameters: [objectName, 'MyVariable', '=', String(expectedValue)], + }); + const freeCondition = (alwaysTrue) => ({ + type: { value: 'Egal' }, + parameters: ['1', '=', alwaysTrue ? '1' : '0'], + }); + const touch = (objectName) => ({ + type: { value: 'ModVarObjet' }, + parameters: [objectName, 'Touched', '+', '1'], + }); + const orOf = (...subInstructions) => ({ + type: { value: 'BuiltinCommonInstructions::Or' }, + parameters: [], + subInstructions, + }); + const orDistributiveOf = (...subInstructions) => ({ + type: { value: 'BuiltinCommonInstructions::OrDistributive' }, + parameters: [], + subInstructions, + }); + const andOf = (...subInstructions) => ({ + type: { value: 'BuiltinCommonInstructions::And' }, + parameters: [], + subInstructions, + }); + + /* ------------------------------------------------------------------ */ + /* 1. Or — many branches all picking the same large set of instances. */ + /* */ + /* Worst-case dedup scenario for the regular Or: every branch */ + /* contributes the same N instances, so the final list never */ + /* grows but every push is rejected by the dedup check. This is */ + /* what shifted from O(K · N²) (indexOf on the growing array) to */ + /* O(K · N) (Set membership check). At K = 10, N = 1000 the */ + /* pre-optimization cost is 10⁷ comparisons; post-optimization it */ + /* is 10⁴ Set ops, while the observable result is the same: */ + /* every instance touched exactly once. */ + /* ------------------------------------------------------------------ */ + it('Or: 10 identical branches each picking 1000 instances dedupes to 1000 unique touches', () => { + const N = 1000; + const K = 10; + const aValues = new Array(N).fill(1); // every instance matches MyVariable=1 + const branches = new Array(K).fill(null).map(() => pickByVar('ObjectA', 1)); + + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(...branches)], + actions: [touch('ObjectA')], + events: [], + }, + ]; + const { allInsts } = runEventsWithObjects(events, { ObjectA: aValues }); + + const touched = touchedFlags(allInsts.ObjectA); + expect(touched.length).toBe(N); + // Every instance touched exactly once — the dedup must not let any + // duplicate through, and must not drop any either. + expect(touched.every((t) => t === 1)).toBe(true); + // Sum sanity-check (cheap to compute and protects against the + // tautological `every === 1` if Touched ever became something odd). + expect(touched.reduce((a, b) => a + b, 0)).toBe(N); + }); + + /* ------------------------------------------------------------------ */ + /* 2. OrDistributive — many branches, one referencing each of many */ + /* objects, plus one free branch that contributes parent.X for */ + /* every object it doesn't reference. */ + /* */ + /* OrDistributive's per-branch contribution iterates over */ + /* `allReferencedObjects`, so every true branch pushes every */ + /* union object's list into the corresponding final list. With K */ + /* objects, M branches, and N instances per object that's */ + /* M · K · N dedup attempts. With M = 5, K = 5, N = 500 that's */ + /* 12 500 attempts of which 2500 are unique — the Set keeps each */ + /* O(1). */ + /* ------------------------------------------------------------------ */ + it('OrDistributive: 5 branches × 5 objects × 500 instances → every object fully picked once', () => { + const N = 500; + const objectNames = [ + 'ObjectA', + 'ObjectB', + 'ObjectC', + 'ObjectD', + 'ObjectE', + ]; + const objectsSpec = {}; + objectNames.forEach((name) => { + objectsSpec[name] = new Array(N).fill(1); + }); + + // 5 branches, each picks one of the 5 objects. Every branch is + // true (every instance has MyVariable=1), so each branch contributes + // its own filtered object's instances and parent's full list for the + // four other "unconstrained" objects → 5 × 5 × 500 = 12 500 + // contributions per Or, deduplicated to 5 × 500 = 2500 picks. + const branches = objectNames.map((name) => pickByVar(name, 1)); + + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orDistributiveOf(...branches)], + actions: objectNames.map(touch), + events: [], + }, + ]; + const { allInsts } = runEventsWithObjects(events, objectsSpec); + + for (const objectName of objectNames) { + const touched = touchedFlags(allInsts[objectName]); + expect(touched.length).toBe(N); + expect(touched.every((t) => t === 1)).toBe(true); + } + }); + + /* ------------------------------------------------------------------ */ + /* 3. Or — many branches each picking a partially overlapping subset */ + /* of one large object. Shows that dedup correctness is preserved */ + /* across non-trivial overlap, not just full overlap. */ + /* */ + /* Each instance has MyVariable in [0, K). Branch i picks */ + /* MyVariable = i, so each instance ends up in exactly one */ + /* branch's filtered list — the union is a partition of the */ + /* instances. With K = 10, N = 200 per branch, the union is */ + /* K · N = 2000 instances and every one is touched exactly once. */ + /* ------------------------------------------------------------------ */ + it('Or: 10 partition branches over 2000 instances → every instance touched once', () => { + const K = 10; + const N = 200; + const aValues = []; + for (let i = 0; i < K; i++) { + for (let j = 0; j < N; j++) aValues.push(i); + } + const branches = []; + for (let i = 0; i < K; i++) branches.push(pickByVar('ObjectA', i)); + + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(...branches)], + actions: [touch('ObjectA')], + events: [], + }, + ]; + const { allInsts } = runEventsWithObjects(events, { ObjectA: aValues }); + + const touched = touchedFlags(allInsts.ObjectA); + expect(touched.length).toBe(K * N); + expect(touched.every((t) => t === 1)).toBe(true); + }); + + /* ------------------------------------------------------------------ */ + /* 4. OrDistributive — many free branches that all leave A */ + /* unconstrained. Every true free branch contributes the parent's */ + /* full A list, so the union accumulates K copies of N instances */ + /* that all dedup back to N. Pre-optimization: K · N² indexOf */ + /* scans over a list that stays at size N. Post-optimization: */ + /* K · N Set ops. With K = 20, N = 500: 5 · 10⁶ vs 10⁴. */ + /* ------------------------------------------------------------------ */ + it('OrDistributive: 1 picking branch + 19 free branches over 500 instances → all 500 touched once', () => { + const N = 500; + const numFreeBranches = 19; + const aValues = new Array(N).fill(1); + + const branches = [pickByVar('ObjectA', 1)]; + for (let i = 0; i < numFreeBranches; i++) { + branches.push(freeCondition(true)); + } + + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orDistributiveOf(...branches)], + actions: [touch('ObjectA')], + events: [], + }, + ]; + const { allInsts } = runEventsWithObjects(events, { ObjectA: aValues }); + + const touched = touchedFlags(allInsts.ObjectA); + expect(touched.length).toBe(N); + expect(touched.every((t) => t === 1)).toBe(true); + }); + + /* ------------------------------------------------------------------ */ + /* 5. Or with And-inside — multi-condition branches that pick several */ + /* objects together at scale. Each true And-branch contributes its */ + /* own A and B picks; the dedup happens independently per object */ + /* in the per-object final lists. Confirms the two final lists */ + /* do not interfere and that both Set-keyed dedups stay correct. */ + /* ------------------------------------------------------------------ */ + it('Or: 8 And-branches × 250 A and 250 B per branch → 8 × 250 = 2000 touches each, deduped to 2000', () => { + const K = 8; + const PER_BRANCH = 250; + // Instances are partitioned by branch: aValues = [0,...,0, 1,...,1, ...]. + // Branch i is And{ pickA=i, pickB=i }, picking its own A-slice and + // B-slice. Every instance is in exactly one branch's slice, so the + // unions across branches add up to K · PER_BRANCH for both A and B. + const aValues = []; + const bValues = []; + for (let i = 0; i < K; i++) { + for (let j = 0; j < PER_BRANCH; j++) { + aValues.push(i); + bValues.push(i); + } + } + const branches = []; + for (let i = 0; i < K; i++) { + branches.push(andOf(pickByVar('ObjectA', i), pickByVar('ObjectB', i))); + } + + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(...branches)], + actions: [touch('ObjectA'), touch('ObjectB')], + events: [], + }, + ]; + const { allInsts } = runEventsWithObjects(events, { + ObjectA: aValues, + ObjectB: bValues, + }); + + const total = K * PER_BRANCH; + const aTouched = touchedFlags(allInsts.ObjectA); + const bTouched = touchedFlags(allInsts.ObjectB); + expect(aTouched.length).toBe(total); + expect(bTouched.length).toBe(total); + expect(aTouched.every((t) => t === 1)).toBe(true); + expect(bTouched.every((t) => t === 1)).toBe(true); + }); + + /* ------------------------------------------------------------------ */ + /* 6. Pathological — many branches all picking the SAME single */ + /* instance. The final list never grows past 1, so dedup work is */ + /* bounded; this is essentially a smoke test that the Set-based */ + /* path doesn't double-count even after a large number of */ + /* redundant pushes. */ + /* ------------------------------------------------------------------ */ + it('Or: 100 branches all picking the same single instance → Touched = 1', () => { + const K = 100; + const branches = new Array(K).fill(null).map(() => pickByVar('ObjectA', 1)); + const events = [ + { + type: 'BuiltinCommonInstructions::Standard', + conditions: [orOf(...branches)], + actions: [touch('ObjectA')], + events: [], + }, + ]; + const { allInsts } = runEventsWithObjects(events, { ObjectA: [1] }); + expect(touchedFlags(allInsts.ObjectA)).toEqual([1]); + }); +}); From 926380fe0bc843892ba07780fceb3d0053e6f48a Mon Sep 17 00:00:00 2001 From: Florian Rival Date: Fri, 8 May 2026 16:57:33 +0200 Subject: [PATCH 10/12] Improve tests --- ...ingCodeGenerationIntegrationStressTests.js | 50 +++++++------------ 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationStressTests.js b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationStressTests.js index d58861db2ef0..c72ce591e56b 100644 --- a/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationStressTests.js +++ b/GDevelop.js/__tests__/GDJSOrObjectPickingCodeGenerationIntegrationStressTests.js @@ -107,16 +107,12 @@ describe('libGD.js - GDJS Or object picking stress tests', () => { /* */ /* Worst-case dedup scenario for the regular Or: every branch */ /* contributes the same N instances, so the final list never */ - /* grows but every push is rejected by the dedup check. This is */ - /* what shifted from O(K · N²) (indexOf on the growing array) to */ - /* O(K · N) (Set membership check). At K = 10, N = 1000 the */ - /* pre-optimization cost is 10⁷ comparisons; post-optimization it */ - /* is 10⁴ Set ops, while the observable result is the same: */ - /* every instance touched exactly once. */ + /* grows but every push is rejected by the dedup check. + /* O(K · N) (Set membership check). /* ------------------------------------------------------------------ */ - it('Or: 10 identical branches each picking 1000 instances dedupes to 1000 unique touches', () => { - const N = 1000; - const K = 10; + it('Or: 100 identical branches each picking 20000 instances', () => { + const N = 20000; + const K = 100; const aValues = new Array(N).fill(1); // every instance matches MyVariable=1 const branches = new Array(K).fill(null).map(() => pickByVar('ObjectA', 1)); @@ -199,11 +195,10 @@ describe('libGD.js - GDJS Or object picking stress tests', () => { /* Each instance has MyVariable in [0, K). Branch i picks */ /* MyVariable = i, so each instance ends up in exactly one */ /* branch's filtered list — the union is a partition of the */ - /* instances. With K = 10, N = 200 per branch, the union is */ - /* K · N = 2000 instances and every one is touched exactly once. */ + /* instances. /* ------------------------------------------------------------------ */ - it('Or: 10 partition branches over 2000 instances → every instance touched once', () => { - const K = 10; + it('Or: many partition branches over many instances → every instance touched once', () => { + const K = 100; const N = 200; const aValues = []; for (let i = 0; i < K; i++) { @@ -231,13 +226,12 @@ describe('libGD.js - GDJS Or object picking stress tests', () => { /* 4. OrDistributive — many free branches that all leave A */ /* unconstrained. Every true free branch contributes the parent's */ /* full A list, so the union accumulates K copies of N instances */ - /* that all dedup back to N. Pre-optimization: K · N² indexOf */ - /* scans over a list that stays at size N. Post-optimization: */ - /* K · N Set ops. With K = 20, N = 500: 5 · 10⁶ vs 10⁴. */ + /* that all dedup back to N. + /* K · N Set ops. /* ------------------------------------------------------------------ */ - it('OrDistributive: 1 picking branch + 19 free branches over 500 instances → all 500 touched once', () => { - const N = 500; - const numFreeBranches = 19; + it('OrDistributive: 1 picking branch + 100 free branches over 20000 instances → all touched once', () => { + const N = 20000; + const numFreeBranches = 100; const aValues = new Array(N).fill(1); const branches = [pickByVar('ObjectA', 1)]; @@ -262,13 +256,10 @@ describe('libGD.js - GDJS Or object picking stress tests', () => { /* ------------------------------------------------------------------ */ /* 5. Or with And-inside — multi-condition branches that pick several */ - /* objects together at scale. Each true And-branch contributes its */ - /* own A and B picks; the dedup happens independently per object */ - /* in the per-object final lists. Confirms the two final lists */ - /* do not interfere and that both Set-keyed dedups stay correct. */ + /* objects together at scale. /* ------------------------------------------------------------------ */ - it('Or: 8 And-branches × 250 A and 250 B per branch → 8 × 250 = 2000 touches each, deduped to 2000', () => { - const K = 8; + it('Or: And-branches inside', () => { + const K = 15; const PER_BRANCH = 250; // Instances are partitioned by branch: aValues = [0,...,0, 1,...,1, ...]. // Branch i is And{ pickA=i, pickB=i }, picking its own A-slice and @@ -311,13 +302,10 @@ describe('libGD.js - GDJS Or object picking stress tests', () => { /* ------------------------------------------------------------------ */ /* 6. Pathological — many branches all picking the SAME single */ - /* instance. The final list never grows past 1, so dedup work is */ - /* bounded; this is essentially a smoke test that the Set-based */ - /* path doesn't double-count even after a large number of */ - /* redundant pushes. */ + /* instance. /* ------------------------------------------------------------------ */ - it('Or: 100 branches all picking the same single instance → Touched = 1', () => { - const K = 100; + it('Or: many branches all picking the same single instance → Touched = 1', () => { + const K = 1000; const branches = new Array(K).fill(null).map(() => pickByVar('ObjectA', 1)); const events = [ { From ab8807e18607ae16a66fe7795ddc40dc099db658 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 16:13:13 +0000 Subject: [PATCH 11/12] Simplify Or / OrDistributive descriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrites the metadata descriptions to be shorter, with a single "Use this when…" decision rule and a one-line example, so that authors (and AI assistants generating events) can pick between the two operators without having to read the full picking-semantics explanation. Or: - Lead with the truth value. - One paragraph on what stays picked vs. not. - One sentence "Use this when…" rule. - Example: "Door collided with Player OR Coin collided with Player" → hide the touched object. Or (independent object picking): - Same shape, framed by contrast: "stay the same as before the Or, unless a true branch explicitly filters them". - "Use this when…" rule. - Example: "Text input is submitted OR Submit button is clicked" → read the text input's value, with the parenthetical that explains why regular Or would lose the input. The "rarely used — multiple events and sub-events are usually a better approach" caveat from the old Or description is dropped: it was opinion rather than rule, and authors who reach for Or vs. an event-list version are usually doing so for a reason. --- .../Builtin/CommonInstructionsExtension.cpp | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp b/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp index ae60f231375d..4cb44a1aa79b 100644 --- a/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp +++ b/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp @@ -64,13 +64,14 @@ BuiltinExtensionsImplementer::ImplementsCommonInstructionsExtension( .AddCondition( "Or", _("Or"), - _("Checks if at least one sub-condition is true. If no " - "sub-condition is specified, it will always be false. " - "This is rarely used — multiple events and sub-events are " - "usually a better approach.\n\n" - "Picked objects: each branch contributes the objects it " - "actually picked. Objects that no true branch referenced are " - "left as they were before the Or."), + _("True if at least one sub-condition is true.\n\n" + "Picked objects: only the instances picked by the true " + "branches. Objects not referenced by any true branch keep " + "what they had before the Or.\n\n" + "Use this when the action acts on the specific object whose " + "condition was tested.\n" + "Example: \"Door collided with Player OR Coin collided with " + "Player\" → hide the touched object."), _("If one of these conditions is true:"), "", "res/conditions/or24_black.png", @@ -82,18 +83,17 @@ BuiltinExtensionsImplementer::ImplementsCommonInstructionsExtension( .AddCondition( "OrDistributive", _("Or (independent object picking)"), - _("Checks if at least one sub-condition is true, while keeping " - "object picking independent across branches. A branch that " - "does not constrain a given object behaves as if all instances " - "of that object were picked for that branch (instead of " - "contributing nothing).\n\n" - "Use this when the action acts on objects that are unrelated " - "to the branch that fired (for example: \"text input was " - "submitted OR submit button was clicked\" → read the text " - "input). Do not use this when the action should act only on " - "the specific object whose state was tested in the branch " - "that fired (for example: \"door collided OR coin collided\" " - "→ hide the touched object) — use the regular Or for that."), + _("True if at least one sub-condition is true.\n\n" + "Picked objects stay the same as before the Or, unless a " + "true branch explicitly filters them. A true branch that " + "does not reference an object leaves that object's " + "selection alone.\n\n" + "Use this when the action acts on an object that some " + "branches do not mention.\n" + "Example: \"Text input is submitted OR Submit button is " + "clicked\" → set a variable to the text input's value. " + "(The button branch doesn't pick the text input; with the " + "regular Or the input would no longer be selected.)"), _("If one of these conditions is true (independent object " "picking):"), "", From 554e4301062310b2eeba91aa564c2aa2ac497544 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 16:32:20 +0000 Subject: [PATCH 12/12] Sharpen the Or description to spell out unselected-objects case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous wording "Objects not referenced by any true branch keep what they had before the Or" was technically correct but obscured the practical consequence: in a top-level Or (no outside-Or pre-pick), the Or itself initializes the selection of every mentioned object to empty before the branches run, so "what they had before the Or" is empty for those objects — which an author reading the description is unlikely to realize. Replace with a concrete consequence-first phrasing — "objects mentioned only by false branches end up with no selection" — and extend the example with a parenthetical that walks through what happens to each of Door, Player, and Coin when the Door branch fires: the Door and Player from that collision are picked (Player IS referenced by the collision condition, contrary to a possible misreading), and the Coin is not selected, so an action on Coin does nothing. --- .../Builtin/CommonInstructionsExtension.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp b/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp index 4cb44a1aa79b..bda02cd3e8ce 100644 --- a/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp +++ b/Core/GDCore/Extensions/Builtin/CommonInstructionsExtension.cpp @@ -65,13 +65,16 @@ BuiltinExtensionsImplementer::ImplementsCommonInstructionsExtension( "Or", _("Or"), _("True if at least one sub-condition is true.\n\n" - "Picked objects: only the instances picked by the true " - "branches. Objects not referenced by any true branch keep " - "what they had before the Or.\n\n" + "Picked objects: the action only sees objects picked by a " + "true branch. Objects mentioned only by false branches end " + "up with no selection.\n\n" "Use this when the action acts on the specific object whose " "condition was tested.\n" "Example: \"Door collided with Player OR Coin collided with " - "Player\" → hide the touched object."), + "Player\" → hide the touched object. (When the Door branch " + "fires, the Door and Player from that collision are picked; " + "the Coin is not selected, so an action on Coin does " + "nothing.)"), _("If one of these conditions is true:"), "", "res/conditions/or24_black.png",