Skip to content

Commit f605cc2

Browse files
claudeclaude
committed
Give each editor its own copy of the merged variables of an object group
The container returned by gd.ObjectRefactorer.mergeVariableContainers is returned "by value", which means the bindings store it in a single instance shared by every call. All the editors were therefore working on the same container: as soon as anything else merged the variables of a group (the object group editor dialog opening while the properties panel is displayed, the AI features building a simplified project...), the container being edited was silently replaced by the variables of the objects - i.e: the state before the changes, which were then lost as they were never applied to the objects. Each editor (group properties panel, object group editor dialog, object group variables dialog) now works on its own copy of the merged container, made by the new makeObjectGroupMergedVariablesContainer helper (and freed when the editor is closed). The copy is done through serialization, which preserves the variable values and types, the persistent UUIDs (used to compute the refactoring changesets) and the editor-only "mixed values" markers. Also fix Variable::UnserializeFrom losing the "mixed values" marker for the variables having no value and no children to read: a structure marked as having mixed values (the marker clears its children) and a variable with entirely mixed types. Without this, the copy of a merged container would show such a variable as an empty structure instead of "mixed values". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015aTvsAxmtZZ1V6bNtXsga6
1 parent 9a08f27 commit f605cc2

7 files changed

Lines changed: 250 additions & 13 deletions

File tree

‎Core/GDCore/Project/Variable.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -324,12 +324,10 @@ void Variable::UnserializeFrom(const SerializerElement& element) {
324324
} else if (type == Type::Boolean) {
325325
SetBool(element.GetBoolAttribute("value", false, "Value"));
326326
}
327-
} else {
327+
} else if (type == Type::Structure || type == Type::Array) {
328328
const SerializerElement& childrenElement =
329329
element.GetChild("children", 0, "Children");
330330
childrenElement.ConsiderAsArrayOf("variable", "Variable");
331-
if (childrenElement.GetChildrenCount() == 0) return;
332-
333331
for (int i = 0; i < childrenElement.GetChildrenCount(); ++i) {
334332
const SerializerElement& childElement = childrenElement.GetChild(i);
335333
if (type == Type::Structure) {
@@ -340,6 +338,10 @@ void Variable::UnserializeFrom(const SerializerElement& element) {
340338
PushNew().UnserializeFrom(childElement);
341339
}
342340
}
341+
// The editor-only "mixed values" marker is restored last, and whatever the
342+
// type is: a variable marked as having mixed values has no children to read
343+
// (they are cleared by MarkAsMixedValues), and a variable with entirely
344+
// mixed types ("mixed" type) has no value and no children either.
343345
if (element.GetBoolAttribute("hasMixedValues", false)) {
344346
MarkAsMixedValues();
345347
}

‎Core/tests/Variable.cpp‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
#include "GDCore/CommonTools.h"
1414
#include "GDCore/Project/VariablesContainer.h"
15+
#include "GDCore/Serialization/SerializerElement.h"
1516
#include "catch.hpp"
1617

1718
TEST_CASE("Variable", "[common][variables]") {
@@ -171,4 +172,48 @@ TEST_CASE("Variable", "[common][variables]") {
171172
REQUIRE(variable.GetChild("MyChild").GetPersistentUuid() == childUuid);
172173
REQUIRE(variable.GetChild("MyNewChild").GetPersistentUuid() != "");
173174
}
175+
SECTION("Serialization keeps the editor-only \"mixed values\" marker") {
176+
// The editor merges the variables of the objects of a group into a
177+
// temporary container, marking the variables that don't have the same
178+
// value (or type) on all objects as having "mixed values". This container
179+
// is copied and snapshotted through serialization, so the marker must
180+
// survive a serialization round trip - including for the variables that
181+
// have no value and no children to read (a structure marked as having
182+
// mixed values, as the marker clears its children, or a variable with
183+
// entirely mixed types).
184+
gd::Variable numberVariable;
185+
numberVariable.SetValue(123);
186+
numberVariable.MarkAsMixedValues();
187+
188+
gd::Variable structureVariable;
189+
structureVariable.GetChild("MyChild").SetValue(456);
190+
structureVariable.MarkAsMixedValues();
191+
192+
gd::Variable mixedTypesVariable;
193+
mixedTypesVariable.CastTo(gd::Variable::Type::MixedTypes);
194+
195+
gd::SerializerElement element;
196+
numberVariable.SerializeTo(element.AddChild("number"));
197+
structureVariable.SerializeTo(element.AddChild("structure"));
198+
mixedTypesVariable.SerializeTo(element.AddChild("mixedTypes"));
199+
200+
gd::Variable unserializedNumberVariable;
201+
unserializedNumberVariable.UnserializeFrom(element.GetChild("number"));
202+
REQUIRE(unserializedNumberVariable.GetType() == gd::Variable::Type::Number);
203+
REQUIRE(unserializedNumberVariable.HasMixedValues());
204+
205+
gd::Variable unserializedStructureVariable;
206+
unserializedStructureVariable.UnserializeFrom(
207+
element.GetChild("structure"));
208+
REQUIRE(unserializedStructureVariable.GetType() ==
209+
gd::Variable::Type::Structure);
210+
REQUIRE(unserializedStructureVariable.HasMixedValues());
211+
212+
gd::Variable unserializedMixedTypesVariable;
213+
unserializedMixedTypesVariable.UnserializeFrom(
214+
element.GetChild("mixedTypes"));
215+
REQUIRE(unserializedMixedTypesVariable.GetType() ==
216+
gd::Variable::Type::MixedTypes);
217+
REQUIRE(unserializedMixedTypesVariable.HasMixedValues());
218+
}
174219
}

‎newIDE/app/src/ObjectGroupEditor/CompactObjectGroupPropertiesEditor.js‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import Window from '../Utils/Window';
2424
import CompactTextField from '../UI/CompactTextField';
2525
import Link from '../UI/Link';
2626
import useVariablesContainerRefactoring from '../VariablesList/useVariablesContainerRefactoring';
27+
import { makeObjectGroupMergedVariablesContainer } from '../Utils/VariablesUtils';
2728
import { type ObjectGroupEditorTab } from './EditedObjectGroupEditorDialog';
2829
import CompactObjectGroupEditor from './CompactObjectGroupEditor';
2930
import { CollapsibleSubPanel } from '../ObjectEditor/CompactObjectPropertiesEditor';
@@ -127,17 +128,27 @@ export const CompactObjectGroupPropertiesEditor = ({
127128
const variablesListRef = React.useRef<?VariablesListInterface>(null);
128129

129130
const groupVariablesContainer = React.useMemo(
130-
// The VariablesContainer is returned by value.
131-
// Thus, the same instance is reused every time.
131+
// This merged container is a temporary container, owned by this editor,
132+
// that the user edits in place - edits only reach the objects of the
133+
// group when the (debounced) refactoring is applied.
132134
() => {
133-
return gd.ObjectRefactorer.mergeVariableContainers(
135+
return makeObjectGroupMergedVariablesContainer(
134136
projectScopedContainersAccessor.get().getObjectsContainersList(),
135137
objectGroup
136138
);
137139
},
138140
[objectGroup, projectScopedContainersAccessor]
139141
);
140142

143+
// Free the C++ memory of the merged container when it's replaced or when
144+
// this editor is unmounted.
145+
React.useEffect(
146+
() => () => {
147+
groupVariablesContainer.delete();
148+
},
149+
[groupVariablesContainer]
150+
);
151+
141152
const openFullEditor = React.useCallback(
142153
() => onEditObjectGroup(objectGroup, 'objects'),
143154
[objectGroup, onEditObjectGroup]

‎newIDE/app/src/ObjectGroupEditor/EditedObjectGroupEditorDialog.js‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import VariablesList from '../VariablesList/VariablesList';
1414
import HelpButton from '../UI/HelpButton';
1515
import useValueWithInit from '../Utils/UseRefInitHook';
1616
import Text from '../UI/Text';
17+
import { makeObjectGroupMergedVariablesContainer } from '../Utils/VariablesUtils';
1718

1819
const gd: libGDevelop = global.gd;
1920

@@ -62,10 +63,11 @@ const EditedObjectGroupEditorDialog = ({
6263
);
6364

6465
const groupVariablesContainer = useValueWithInit(
65-
// The VariablesContainer is returned by value.
66-
// Thus, the same instance is reused every time.
66+
// This merged container is a temporary container, owned by this dialog,
67+
// that the user edits in place - edits only reach the objects of the
68+
// group when the refactoring is applied.
6769
() =>
68-
gd.ObjectRefactorer.mergeVariableContainers(
70+
makeObjectGroupMergedVariablesContainer(
6971
projectScopedContainersAccessor.get().getObjectsContainersList(),
7072
group
7173
)
@@ -85,6 +87,16 @@ const EditedObjectGroupEditorDialog = ({
8587
ensurePersistentUuids: true,
8688
});
8789

90+
// Free the C++ memory of the merged container when the dialog is closed
91+
// (declared after other hooks using the container, so their cleanups run
92+
// before the container is deleted).
93+
React.useEffect(
94+
() => () => {
95+
groupVariablesContainer.delete();
96+
},
97+
[groupVariablesContainer]
98+
);
99+
88100
const apply = async () => {
89101
onApply();
90102
if (!initialInstances) {

‎newIDE/app/src/Utils/VariablesUtils.js‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,43 @@ export const hasChildThatContainsStringInNameOrValue = (
4343
}
4444
};
4545

46+
/**
47+
* Merge the variables of the objects of a group into a new, caller-owned
48+
* variables container: the intersection of the variables of all the objects
49+
* of the group, with "mixed values"/"mixed types" markers when they differ
50+
* between objects.
51+
*
52+
* `gd.ObjectRefactorer.mergeVariableContainers` returns a `VariablesContainer`
53+
* "by value", which means the same C++ instance is shared by every call (it's
54+
* stored in a static variable by the bindings). Keeping it in an editor is
55+
* unsafe: any other call (from another editor, an AI editor function, etc.)
56+
* would overwrite it, making the variables being edited seemingly "reset" to
57+
* their previous state. This helper copies the merged result into a new
58+
* container owned by the caller - which must call `delete` on it when done,
59+
* to free the C++ memory.
60+
*/
61+
export const makeObjectGroupMergedVariablesContainer = (
62+
objectsContainersList: gdObjectsContainersList,
63+
objectGroup: gdObjectGroup
64+
): gdVariablesContainer => {
65+
const sharedMergedVariablesContainer = gd.ObjectRefactorer.mergeVariableContainers(
66+
objectsContainersList,
67+
objectGroup
68+
);
69+
const mergedVariablesContainer = new gd.VariablesContainer(
70+
sharedMergedVariablesContainer.getSourceType()
71+
);
72+
// Serialization preserves everything needed for editing and refactoring:
73+
// variable types and values (including the editor-only "mixed values"
74+
// markers) and persistent UUIDs (of the container and its variables).
75+
const serializedElement = new gd.SerializerElement();
76+
sharedMergedVariablesContainer.serializeTo(serializedElement);
77+
mergedVariablesContainer.unserializeFrom(serializedElement);
78+
serializedElement.delete();
79+
80+
return mergedVariablesContainer;
81+
};
82+
4683
export const insertInVariablesContainer = (
4784
variablesContainer: gdVariablesContainer,
4885
name: string,

‎newIDE/app/src/Utils/VariablesUtils.spec.js‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
// @flow
2+
import { makeObjectGroupMergedVariablesContainer } from './VariablesUtils';
3+
4+
const gd: libGDevelop = global.gd;
5+
6+
describe('makeObjectGroupMergedVariablesContainer', () => {
7+
const setUpProjectWithObjectGroup = () => {
8+
const project = gd.ProjectHelper.createNewGDJSProject();
9+
const layout = project.insertNewLayout('Scene', 0);
10+
const objectsContainer = layout.getObjects();
11+
const objectA = objectsContainer.insertNewObject(
12+
project,
13+
'Sprite',
14+
'ObjectA',
15+
0
16+
);
17+
const objectB = objectsContainer.insertNewObject(
18+
project,
19+
'Sprite',
20+
'ObjectB',
21+
1
22+
);
23+
const objectGroup = objectsContainer
24+
.getObjectGroups()
25+
.insertNew('Group', 0);
26+
objectGroup.addObject('ObjectA');
27+
objectGroup.addObject('ObjectB');
28+
29+
const makeMergedVariablesContainer = () =>
30+
makeObjectGroupMergedVariablesContainer(
31+
gd.ObjectsContainersList.makeNewObjectsContainersListForProjectAndLayout(
32+
project,
33+
layout
34+
),
35+
objectGroup
36+
);
37+
38+
return { project, objectA, objectB, makeMergedVariablesContainer };
39+
};
40+
41+
it('returns a new, caller-owned container at each call (never a shared instance)', () => {
42+
const {
43+
project,
44+
objectA,
45+
objectB,
46+
makeMergedVariablesContainer,
47+
} = setUpProjectWithObjectGroup();
48+
objectA
49+
.getVariables()
50+
.insertNew('Health', 0)
51+
.setValue(100);
52+
objectB
53+
.getVariables()
54+
.insertNew('Health', 0)
55+
.setValue(100);
56+
57+
const container1 = makeMergedVariablesContainer();
58+
const container2 = makeMergedVariablesContainer();
59+
expect(container1.ptr).not.toBe(container2.ptr);
60+
61+
// A mutation of one container (or a new merge) does not affect the other.
62+
container2.remove('Health');
63+
const container3 = makeMergedVariablesContainer();
64+
expect(container1.has('Health')).toBe(true);
65+
expect(container3.has('Health')).toBe(true);
66+
67+
container1.delete();
68+
container2.delete();
69+
container3.delete();
70+
project.delete();
71+
});
72+
73+
it('keeps the intersection of variables, with mixed values markers and persistent UUIDs', () => {
74+
const {
75+
project,
76+
objectA,
77+
objectB,
78+
makeMergedVariablesContainer,
79+
} = setUpProjectWithObjectGroup();
80+
objectA
81+
.getVariables()
82+
.insertNew('Health', 0)
83+
.setValue(100);
84+
objectB
85+
.getVariables()
86+
.insertNew('Health', 0)
87+
.setValue(50);
88+
objectA
89+
.getVariables()
90+
.insertNew('OnlyOnA', 1)
91+
.setValue(1);
92+
objectA.getVariables().ensurePersistentUuids();
93+
objectB.getVariables().ensurePersistentUuids();
94+
95+
const mergedVariablesContainer = makeMergedVariablesContainer();
96+
97+
// Only the common variables are kept.
98+
expect(mergedVariablesContainer.has('Health')).toBe(true);
99+
expect(mergedVariablesContainer.has('OnlyOnA')).toBe(false);
100+
101+
// Different values are marked as "mixed" (an editor-only marker).
102+
expect(mergedVariablesContainer.get('Health').hasMixedValues()).toBe(true);
103+
104+
// The persistent UUIDs of the first object variables are kept, so that
105+
// refactoring changesets can be computed against them.
106+
expect(mergedVariablesContainer.get('Health').getPersistentUuid()).toBe(
107+
objectA
108+
.getVariables()
109+
.get('Health')
110+
.getPersistentUuid()
111+
);
112+
113+
mergedVariablesContainer.delete();
114+
project.delete();
115+
});
116+
});

‎newIDE/app/src/VariablesList/ObjectGroupVariablesDialog.js‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,10 @@ import VariablesList from './VariablesList';
1313
import HelpButton from '../UI/HelpButton';
1414
import { getVariablePathFromNodeId } from './VariableToTreeNodeHandling';
1515
import { ProjectScopedContainersAccessor } from '../InstructionOrExpression/EventsScope';
16-
import { insertInVariablesContainer } from '../Utils/VariablesUtils';
16+
import {
17+
insertInVariablesContainer,
18+
makeObjectGroupMergedVariablesContainer,
19+
} from '../Utils/VariablesUtils';
1720
import { getRootVariableName } from '../EventsSheet/ParameterFields/VariableField';
1821
import { getNodeIdFromVariableName } from './VariableToTreeNodeHandling';
1922
import useValueWithInit from '../Utils/UseRefInitHook';
@@ -54,10 +57,11 @@ const ObjectGroupVariablesDialog = ({
5457
isListLocked,
5558
}: Props): React.Node => {
5659
const groupVariablesContainer = useValueWithInit(
57-
// The VariablesContainer is returned by value.
58-
// Thus, the same instance is reused every time.
60+
// This merged container is a temporary container, owned by this dialog,
61+
// that the user edits in place - edits only reach the objects of the
62+
// group when the refactoring is applied.
5963
() =>
60-
gd.ObjectRefactorer.mergeVariableContainers(
64+
makeObjectGroupMergedVariablesContainer(
6165
projectScopedContainersAccessor.get().getObjectsContainersList(),
6266
objectGroup
6367
)
@@ -77,6 +81,16 @@ const ObjectGroupVariablesDialog = ({
7781
ensurePersistentUuids: true,
7882
});
7983

84+
// Free the C++ memory of the merged container when the dialog is closed
85+
// (declared after other hooks using the container, so their cleanups run
86+
// before the container is deleted).
87+
React.useEffect(
88+
() => () => {
89+
groupVariablesContainer.delete();
90+
},
91+
[groupVariablesContainer]
92+
);
93+
8094
const apply = async () => {
8195
onApply(
8296
lastSelectedVariableNodeId.current &&

0 commit comments

Comments
 (0)