Fix "Or" when already picked object + add a Or for independant object picking#8579
Open
Fix "Or" when already picked object + add a Or for independant object picking#8579
Conversation
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).
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).
- 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).
- 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.
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.
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.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
=> This is line 8 of truth table "B"
=> This is line 10 of truth table "A"
In other words, the traditional Or tends to reduce the selection to what is only picked by branches referring to X, while the new "distributive" or tends to keep the selection as large as seen by branches not referring to X.