Skip to content

Commit 68117f1

Browse files
endelendel
committed
improve refId reference tracking. closes #200 
1 parent 3e3684a commit 68117f1

14 files changed

+407
-58
lines changed

‎package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@colyseus/schema",
3-
"version": "3.0.36",
3+
"version": "3.0.37",
44
"description": "Binary state serializer with delta encoding for games",
55
"bin": {
66
"schema-codegen": "bin/schema-codegen",

‎src/Schema.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,10 @@ export class Schema {
158158
return obj as ToJSON<typeof this>;
159159
}
160160

161+
/**
162+
* Used in tests only
163+
* @internal
164+
*/
161165
discardAllChanges() {
162166
this[$changes].discardAll();
163167
}
@@ -185,8 +189,12 @@ export class Schema {
185189
const changeTree: ChangeTree = ref[$changes];
186190
const refId = changeTree.refId;
187191

188-
let output = "";
189-
output += `${getIndent(level)}${ref.constructor.name} (refId: ${refId})${contents}\n`;
192+
// log reference count if > 1
193+
const refCount = (changeTree.root?.refCount?.[refId] > 1)
194+
? ` [×${changeTree.root.refCount[refId]}]`
195+
: '';
196+
197+
let output = `${getIndent(level)}${ref.constructor.name} (refId: ${refId})${refCount}${contents}\n`;
190198

191199
changeTree.forEachChild((childChangeTree) =>
192200
output += this.debugRefIds(childChangeTree.ref, showContents, level + 1));

‎src/encoder/ChangeTree.ts

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -160,22 +160,36 @@ export class ChangeTree<T extends Ref=any> {
160160
this.root = root;
161161
this.checkIsFiltered(this.parent, this.parentIndex);
162162

163+
//
164+
// TODO: refactor and possibly unify .setRoot() and .setParent()
165+
//
166+
163167
// Recursively set root on child structures
164168
const metadata: Metadata = this.ref.constructor[Symbol.metadata];
165169
if (metadata) {
166170
metadata[$refTypeFieldIndexes]?.forEach((index) => {
167171
const field = metadata[index as any as number];
168-
const value = this.ref[field.name];
169-
value?.[$changes].setRoot(root);
172+
const changeTree: ChangeTree = this.ref[field.name]?.[$changes];
173+
if (changeTree) {
174+
if (changeTree.root !== root) {
175+
changeTree.setRoot(root);
176+
} else {
177+
root.add(changeTree); // increment refCount
178+
}
179+
}
170180
});
171181

172182
} else if (this.ref[$childType] && typeof(this.ref[$childType]) !== "string") {
173183
// MapSchema / ArraySchema, etc.
174184
(this.ref as MapSchema).forEach((value, key) => {
175-
value[$changes].setRoot(root);
185+
const changeTree: ChangeTree = value[$changes];
186+
if (changeTree.root !== root) {
187+
changeTree.setRoot(root);
188+
} else {
189+
root.add(changeTree); // increment refCount
190+
}
176191
});
177192
}
178-
179193
}
180194

181195
setParent(
@@ -203,14 +217,19 @@ export class ChangeTree<T extends Ref=any> {
203217
if (metadata) {
204218
metadata[$refTypeFieldIndexes]?.forEach((index) => {
205219
const field = metadata[index as any as number];
206-
const value = this.ref[field.name];
207-
value?.[$changes].setParent(this.ref, root, index);
220+
const changeTree: ChangeTree = this.ref[field.name]?.[$changes];
221+
if (changeTree && changeTree.root !== root) {
222+
changeTree.setParent(this.ref, root, index);
223+
}
208224
});
209225

210226
} else if (this.ref[$childType] && typeof(this.ref[$childType]) !== "string") {
211227
// MapSchema / ArraySchema, etc.
212228
(this.ref as MapSchema).forEach((value, key) => {
213-
value[$changes].setParent(this.ref, root, this.indexes[key] ?? key);
229+
const changeTree: ChangeTree = value[$changes];
230+
if (changeTree.root !== root) {
231+
changeTree.setParent(this.ref, root, this.indexes[key] ?? key);
232+
}
214233
});
215234
}
216235

@@ -478,15 +497,12 @@ export class ChangeTree<T extends Ref=any> {
478497
this.allFilteredChanges.indexes = {};
479498
this.allFilteredChanges.operations.length = 0;
480499
}
481-
482-
// remove children references
483-
this.forEachChild((changeTree, _) =>
484-
this.root?.remove(changeTree));
485500
}
486501
}
487502

488503
/**
489504
* Recursively discard all changes from this, and child structures.
505+
* (Used in tests only)
490506
*/
491507
discardAll() {
492508
const keys = Object.keys(this.indexedOperations);

‎src/encoder/Root.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ export class Root {
6969

7070
this.refCount[changeTree.refId] = 0;
7171

72+
changeTree.forEachChild((child, _) => this.remove(child));
73+
7274
} else {
7375
this.refCount[changeTree.refId] = refCount;
7476

@@ -90,8 +92,6 @@ export class Root {
9092
}
9193
}
9294

93-
changeTree.forEachChild((child, _) => this.remove(child));
94-
9595
return refCount;
9696
}
9797

‎src/types/custom/ArraySchema.ts

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,7 @@ export class ArraySchema<V = any> implements Array<V>, Collection<number, V> {
185185

186186
const changeTree = this[$changes];
187187

188-
// values.forEach((value, i) => {
189-
190-
for (let i = 0, l = values.length; i < values.length; i++, length++) {
188+
for (let i = 0, l = values.length; i < l; i++, length++) {
191189
const value = values[i];
192190

193191
if (value === undefined || value === null) {
@@ -211,9 +209,6 @@ export class ArraySchema<V = any> implements Array<V>, Collection<number, V> {
211209
value[$changes]?.setParent(this, changeTree.root, length);
212210
}
213211

214-
// length++;
215-
// });
216-
217212
return length;
218213
}
219214

@@ -310,22 +305,9 @@ export class ArraySchema<V = any> implements Array<V>, Collection<number, V> {
310305
// discard previous operations.
311306
const changeTree = this[$changes];
312307

313-
// discard children
314-
changeTree.forEachChild((changeTree, _) => {
315-
changeTree.discard(true);
316-
317-
//
318-
// TODO: add tests with instance sharing + .clear()
319-
// FIXME: this.root? is required because it is being called at decoding time.
320-
//
321-
// TODO: do not use [$changes] at decoding time.
322-
//
323-
const root = changeTree.root;
324-
if (root !== undefined) {
325-
root.removeChangeFromChangeSet("changes", changeTree);
326-
root.removeChangeFromChangeSet("allChanges", changeTree);
327-
root.removeChangeFromChangeSet("allFilteredChanges", changeTree);
328-
}
308+
// remove children references
309+
changeTree.forEachChild((childChangeTree, _) => {
310+
changeTree.root?.remove(childChangeTree);
329311
});
330312

331313
changeTree.discard(true);

‎src/types/custom/CollectionSchema.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,11 @@ export class CollectionSchema<V=any> implements Collection<K, V>{
116116
changeTree.discard(true);
117117
changeTree.indexes = {};
118118

119+
// remove children references
120+
changeTree.forEachChild((childChangeTree, _) => {
121+
changeTree.root?.remove(childChangeTree);
122+
});
123+
119124
// clear previous indexes
120125
this.$indexes.clear();
121126

‎src/types/custom/MapSchema.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,11 @@ export class MapSchema<V=any, K extends string = string> implements Map<K, V>, C
155155
changeTree.discard(true);
156156
changeTree.indexes = {};
157157

158+
// remove children references
159+
changeTree.forEachChild((childChangeTree, _) => {
160+
changeTree.root?.remove(childChangeTree);
161+
});
162+
158163
// clear previous indexes
159164
this.$indexes.clear();
160165

‎test/ArraySchema.test.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,82 @@ describe("ArraySchema Tests", () => {
186186
assertDeepStrictEqualEncodeAll(state);
187187
});
188188

189+
it("2: shift + repopulate should not trigger refId not found", () => {
190+
const Entity = schema({
191+
i: "number"
192+
});
193+
const MyState = schema({
194+
entities: [Entity]
195+
});
196+
197+
const state = new MyState();
198+
state.entities = [];
199+
200+
const populate = (count: number) => {
201+
for (let i = 0; i < count; i++) {
202+
const ent = new Entity();
203+
ent.i = i;
204+
state.entities.push(ent);
205+
}
206+
}
207+
populate(100);
208+
209+
// Define the sequence of lengths and their corresponding encode actions
210+
const encodeActions = [
211+
{ length: 100, action: "FULL!" },
212+
{ length: 95, action: "PATCH!" },
213+
{ length: 89, action: "PATCH!" },
214+
{ length: 83, action: "FULL!" },
215+
{ length: 83, action: "PATCH!" },
216+
{ length: 77, action: "PATCH!" },
217+
{ length: 71, action: "PATCH!" },
218+
{ length: 66, action: "FULL!" },
219+
{ length: 65, action: "PATCH!" },
220+
{ length: 59, action: "PATCH!" },
221+
{ length: 53, action: "PATCH!" },
222+
{ length: 46, action: "PATCH!" },
223+
{ length: 46, action: "FULL!" },
224+
{ length: 41, action: "PATCH!" },
225+
{ length: 35, action: "PATCH!" }
226+
];
227+
228+
let actionIndex = 0;
229+
let newClient = createInstanceFromReflection(state); // Initial client
230+
newClient.decode(state.encodeAll()); // Initial FULL! decode
231+
232+
for (let i = 0; i < 100; i++) {
233+
state.entities.forEach((entity) => entity.i++);
234+
state.entities.shift();
235+
236+
// Check if the current length matches the next action in the sequence
237+
if (actionIndex < encodeActions.length && state.entities.length === encodeActions[actionIndex].length) {
238+
if (encodeActions[actionIndex].action === "FULL!") {
239+
newClient = createInstanceFromReflection(state); // New client for FULL!
240+
newClient.decode(state.encodeAll());
241+
} else if (encodeActions[actionIndex].action === "PATCH!") {
242+
newClient.decode(state.encode()); // Use existing client for PATCH!
243+
}
244+
actionIndex++;
245+
}
246+
247+
if (state.entities.length === 0) {
248+
populate(100);
249+
// After repopulating, check if the new length matches an action
250+
if (actionIndex < encodeActions.length && state.entities.length === encodeActions[actionIndex].length) {
251+
if (encodeActions[actionIndex].action === "FULL!") {
252+
newClient = createInstanceFromReflection(state); // New client for FULL!
253+
newClient.decode(state.encodeAll());
254+
} else if (encodeActions[actionIndex].action === "PATCH!") {
255+
newClient.decode(state.encode()); // Use existing client for PATCH!
256+
}
257+
actionIndex++;
258+
}
259+
}
260+
}
261+
262+
assertDeepStrictEqualEncodeAll(state);
263+
});
264+
189265
it("mutate previous instance + shift", () => {
190266
/**
191267
* This test shows that flagging the `changeSet` item as `undefined`

‎test/ChangeTree.test.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import * as assert from "assert";
22

33
import { ChangeTree } from "../src/encoder/ChangeTree";
44
import { Schema, type, view, MapSchema, ArraySchema, $changes, OPERATION } from "../src";
5-
import { assertDeepStrictEqualEncodeAll, getDecoder, getEncoder } from "./Schema";
5+
import { assertDeepStrictEqualEncodeAll, assertRefIdCounts, getDecoder, getEncoder } from "./Schema";
66
import { nanoid } from "nanoid";
77

88
describe("ChangeTree", () => {
@@ -255,11 +255,7 @@ describe("ChangeTree", () => {
255255

256256
});
257257

258-
xit("using ArraySchema: replace should be DELETE_AND_ADD operation", () => {
259-
//
260-
// TODO: this test is not passing!
261-
//
262-
258+
it("using ArraySchema: replace should be DELETE_AND_ADD operation", () => {
263259
class Entity extends Schema {
264260
@type("string") id: string = nanoid(9);
265261
}
@@ -280,6 +276,7 @@ describe("ChangeTree", () => {
280276
const entity2 = new Entity();
281277
state.entities[0] = entity2;
282278
decodedState.decode(state.encode());
279+
assertRefIdCounts(state, decodedState);
283280

284281
assert.strictEqual(1, encoder.root.refCount[entity2[$changes].refId]);
285282
assert.strictEqual(0, encoder.root.refCount[entity1[$changes].refId]);

0 commit comments

Comments
 (0)