Skip to content

Commit 88e3389

Browse files
endelendel
committed
minor encoding fixes. add failing tests for TypeScript enum and ChangeTree (ArraySchema + DELETE_AND_ADD)
1 parent 1503d8a commit 88e3389

16 files changed

+379
-117
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.11",
3+
"version": "3.0.12",
44
"description": "Binary state serializer with delta encoding for games",
55
"bin": {
66
"schema-codegen": "./bin/schema-codegen",

‎src/Metadata.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,6 @@ export const Metadata = {
157157

158158
for (const field in fields) {
159159
const type = fields[field];
160-
const normalizedType = getNormalizedType(type);
161160

162161
// FIXME: this code is duplicated from @type() annotation
163162
const complexTypeKlass = (Array.isArray(type))
@@ -166,7 +165,7 @@ export const Metadata = {
166165

167166
const childType = (complexTypeKlass)
168167
? Object.values(type)[0]
169-
: normalizedType;
168+
: getNormalizedType(type);
170169

171170
Metadata.addField(
172171
metadata,

‎src/annotations.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -400,14 +400,14 @@ export function getPropertyDescriptor(
400400

401401
//
402402
// Replacing existing "ref", remove it from root.
403-
// TODO: if there are other references to this instance, we should not remove it from root.
404403
//
405404
if (previousValue !== undefined && previousValue[$changes]) {
406405
changeTree.root?.remove(previousValue[$changes]);
407-
}
406+
this.constructor[$track](changeTree, fieldIndex, OPERATION.DELETE_AND_ADD);
408407

409-
// flag the change for encoding.
410-
this.constructor[$track](changeTree, fieldIndex, OPERATION.ADD);
408+
} else {
409+
this.constructor[$track](changeTree, fieldIndex, OPERATION.ADD);
410+
}
411411

412412
//
413413
// call setParent() recursively for this and its child

‎src/decoder/DecodeOperation.ts

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -59,20 +59,6 @@ export function decodeValue(
5959
//
6060
if (operation !== OPERATION.DELETE_AND_ADD) {
6161
ref[$deleteByIndex](index);
62-
63-
// //
64-
// // FIXME: is this in the correct place?
65-
// // (This is sounding like a workaround just for ArraySchema, see
66-
// // "should splice and move" test on ArraySchema.test.ts)
67-
// //
68-
// allChanges.push({
69-
// ref,
70-
// refId: decoder.currentRefId,
71-
// op: OPERATION.DELETE,
72-
// field: index as unknown as string,
73-
// value: undefined,
74-
// previousValue,
75-
// });
7662
}
7763

7864
value = undefined;

‎src/encoder/ChangeTree.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@ import { Root } from "./Root";
1111
import { Metadata } from "../Metadata";
1212
import type { EncodeOperation } from "./EncodeOperation";
1313
import type { DecodeOperation } from "../decoder/DecodeOperation";
14-
import { TypeContext } from "../types/TypeContext";
15-
import { ReferenceTracker } from "../decoder/ReferenceTracker";
1614

1715
declare global {
1816
interface Object {

‎src/encoder/Encoder.ts

Lines changed: 25 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { Schema } from "../Schema";
22
import { TypeContext } from "../types/TypeContext";
3-
import { $changes, $encoder, $filter, $onEncodeEnd } from "../types/symbols";
3+
import { $changes, $encoder, $filter } from "../types/symbols";
44

55
import { encode } from "../encoding/encode";
66
import type { Iterator } from "../encoding/decode";
@@ -58,14 +58,6 @@ export class Encoder<T extends Schema = any> {
5858
for (let i = 0, numChangeTrees = changeTrees.length; i < numChangeTrees; i++) {
5959
const changeTree = changeTrees[i];
6060

61-
const operations = changeTree[changeSetName];
62-
const ref = changeTree.ref;
63-
64-
const ctor = ref.constructor;
65-
const encoder = ctor[$encoder];
66-
const filter = ctor[$filter];
67-
const metadata = ctor[Symbol.metadata];
68-
6961
if (hasView) {
7062
if (!view.items.has(changeTree)) {
7163
view.invisible.add(changeTree);
@@ -76,14 +68,26 @@ export class Encoder<T extends Schema = any> {
7668
}
7769
}
7870

71+
const operations = changeTree[changeSetName];
72+
const ref = changeTree.ref;
73+
74+
// TODO: avoid iterating over change tree if no changes were made
75+
const numChanges = operations.operations.length;
76+
if (numChanges === 0) { continue; }
77+
78+
const ctor = ref.constructor;
79+
const encoder = ctor[$encoder];
80+
const filter = ctor[$filter];
81+
const metadata = ctor[Symbol.metadata];
82+
7983
// skip root `refId` if it's the first change tree
8084
// (unless it "hasView", which will need to revisit the root)
8185
if (hasView || it.offset > initialOffset || changeTree !== rootChangeTree) {
8286
buffer[it.offset++] = SWITCH_TO_STRUCTURE & 255;
8387
encode.number(buffer, changeTree.refId, it);
8488
}
8589

86-
for (let j = 0, numChanges = operations.operations.length; j < numChanges; j++) {
90+
for (let j = 0; j < numChanges; j++) {
8791
const fieldIndex = operations.operations[j];
8892

8993
const operation = (fieldIndex < 0)
@@ -199,17 +203,18 @@ export class Encoder<T extends Schema = any> {
199203
const viewOffset = it.offset;
200204

201205
// encode visibility changes (add/remove for this view)
202-
const refIds = Object.keys(view.changes);
203-
204-
for (let i = 0, numRefIds = refIds.length; i < numRefIds; i++) {
205-
const refId = refIds[i];
206-
const changes = view.changes[refId];
206+
for (const [refId, changes] of view.changes) {
207207
const changeTree = this.root.changeTrees[refId];
208208

209-
if (
210-
changeTree === undefined ||
211-
Object.keys(changes).length === 0 // FIXME: avoid having empty changes if no changes were made
212-
) {
209+
if (changeTree === undefined) {
210+
// detached instance, remove from view and skip.
211+
view.changes.delete(refId);
212+
continue;
213+
}
214+
215+
const keys = Object.keys(changes);
216+
if (keys.length === 0) {
217+
// FIXME: avoid having empty changes if no changes were made
213218
// console.log("changes.size === 0, skip", changeTree.ref.constructor.name);
214219
continue;
215220
}
@@ -223,7 +228,6 @@ export class Encoder<T extends Schema = any> {
223228
bytes[it.offset++] = SWITCH_TO_STRUCTURE & 255;
224229
encode.number(bytes, changeTree.refId, it);
225230

226-
const keys = Object.keys(changes);
227231
for (let i = 0, numChanges = keys.length; i < numChanges; i++) {
228232
const key = keys[i];
229233
const operation = changes[key];
@@ -239,7 +243,7 @@ export class Encoder<T extends Schema = any> {
239243
// (to allow re-using StateView's for multiple clients)
240244
//
241245
// clear "view" changes after encoding
242-
view.changes = {};
246+
view.changes.clear();
243247

244248
// try to encode "filtered" changes
245249
this.encode(it, view, bytes, "filteredChanges", false, viewOffset);
@@ -250,26 +254,6 @@ export class Encoder<T extends Schema = any> {
250254
]);
251255
}
252256

253-
onEndEncode(changeTrees = this.root.changes) {
254-
// changeTrees.forEach(function(changeTree) {
255-
// changeTree.endEncode();
256-
// });
257-
258-
259-
// for (const refId in changeTrees) {
260-
// const changeTree = this.root.changeTrees[refId];
261-
// changeTree.endEncode();
262-
263-
// // changeTree.changes.clear();
264-
265-
// // // ArraySchema and MapSchema have a custom "encode end" method
266-
// // changeTree.ref[$onEncodeEnd]?.();
267-
268-
// // // Not a new instance anymore
269-
// // delete changeTree[$isNew];
270-
// }
271-
}
272-
273257
discardChanges() {
274258
// discard shared changes
275259
let length = this.root.changes.length;

‎src/encoder/Root.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ export class Root {
100100
if (spliceOne(changeSet, changeSet.indexOf(changeTree))) {
101101
changeTree[changeSetName].queueRootIndex = -1;
102102
// changeSet[index] = undefined;
103+
return true;
103104
}
104105
}
105106

‎src/encoder/StateView.ts

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ export class StateView {
2525
* Manual "ADD" operations for changes per ChangeTree, specific to this view.
2626
* (This is used to force encoding a property, even if it was not changed)
2727
*/
28-
changes: { [refId: number]: IndexedOperations } = {};
28+
// TODO: use map here!? may fix encode ordering issue
29+
changes = new Map<number, IndexedOperations>();
2930

3031
// TODO: allow to set multiple tags at once
3132
add(obj: Ref, tag: number = DEFAULT_VIEW_TAG, checkIncludeParent: boolean = true) {
@@ -43,17 +44,17 @@ export class StateView {
4344
// - if it was invisible to this view
4445
// - if it were previously filtered out
4546
if (checkIncludeParent && changeTree.parent) {
46-
this.addParent(changeTree.parent[$changes], changeTree.parentIndex, tag);
47+
this.addParentOf(changeTree, tag);
4748
}
4849

4950
//
5051
// TODO: when adding an item of a MapSchema, the changes may not
5152
// be set (only the parent's changes are set)
5253
//
53-
let changes = this.changes[changeTree.refId];
54+
let changes = this.changes.get(changeTree.refId);
5455
if (changes === undefined) {
5556
changes = {};
56-
this.changes[changeTree.refId] = changes;
57+
this.changes.set(changeTree.refId, changes);
5758
}
5859

5960
// set tag
@@ -118,28 +119,34 @@ export class StateView {
118119
return this;
119120
}
120121

121-
protected addParent(changeTree: ChangeTree, parentIndex: number, tag: number) {
122+
protected addParentOf(childChangeTree: ChangeTree, tag: number) {
123+
const changeTree = childChangeTree.parent[$changes];
124+
const parentIndex = childChangeTree.parentIndex;
125+
122126
// view must have all "changeTree" parent tree
123127
this.items.add(changeTree);
124128

125129
// add parent's parent
126130
const parentChangeTree: ChangeTree = changeTree.parent?.[$changes];
127131
if (parentChangeTree && (parentChangeTree.filteredChanges !== undefined)) {
128-
this.addParent(parentChangeTree, changeTree.parentIndex, tag);
132+
this.addParentOf(changeTree, tag);
129133
}
130134

131-
// parent is already available, no need to add it!
132-
if (!this.invisible.has(changeTree)) {
135+
if (
136+
// parent is already available, no need to add it!
137+
!this.invisible.has(changeTree) &&
138+
// item is being replaced, no need to add parent
139+
changeTree.indexedOperations[parentIndex] !== OPERATION.DELETE_AND_ADD
140+
) {
133141
return;
134142
}
135143

136144
// add parent's tag properties
137145
if (changeTree.getChange(parentIndex) !== OPERATION.DELETE) {
138-
139-
let changes = this.changes[changeTree.refId];
146+
let changes = this.changes.get(changeTree.refId);
140147
if (changes === undefined) {
141148
changes = {};
142-
this.changes[changeTree.refId] = changes;
149+
this.changes.set(changeTree.refId, changes);
143150
}
144151

145152
if (!this.tags) {
@@ -171,21 +178,21 @@ export class StateView {
171178
const ref = changeTree.ref;
172179
const metadata: Metadata = ref.constructor[Symbol.metadata];
173180

174-
let changes = this.changes[changeTree.refId];
181+
let changes = this.changes.get(changeTree.refId);
175182
if (changes === undefined) {
176183
changes = {};
177-
this.changes[changeTree.refId] = changes;
184+
this.changes.set(changeTree.refId, changes);
178185
}
179186

180187
if (tag === DEFAULT_VIEW_TAG) {
181188
// parent is collection (Map/Array)
182189
const parent = changeTree.parent;
183190
if (!Metadata.isValidInstance(parent)) {
184191
const parentChangeTree = parent[$changes];
185-
let changes = this.changes[parentChangeTree.refId];
192+
let changes = this.changes.get(parentChangeTree.refId);
186193
if (changes === undefined) {
187194
changes = {};
188-
this.changes[parentChangeTree.refId] = changes;
195+
this.changes.set(parentChangeTree.refId, changes);
189196
}
190197
// DELETE / DELETE BY REF ID
191198
changes[changeTree.parentIndex] = OPERATION.DELETE;

‎src/types/custom/ArraySchema.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,8 @@ export class ArraySchema<V = any> implements Array<V>, Collection<number, V> {
9292
if (setValue[$changes]) {
9393
assertInstanceType(setValue, obj[$childType] as typeof Schema, obj, key);
9494

95-
if (obj.items[key as unknown as number] !== undefined) {
95+
const previousValue = obj.items[key as unknown as number];
96+
if (previousValue !== undefined) {
9697
if (setValue[$changes].isNew) {
9798
this[$changes].indexedOperation(Number(key), OPERATION.MOVE_AND_ADD);
9899

@@ -103,10 +104,16 @@ export class ArraySchema<V = any> implements Array<V>, Collection<number, V> {
103104
this[$changes].indexedOperation(Number(key), OPERATION.MOVE);
104105
}
105106
}
107+
108+
// remove root reference from previous value
109+
previousValue[$changes].root?.remove(previousValue[$changes]);
110+
106111
} else if (setValue[$changes].isNew) {
107112
this[$changes].indexedOperation(Number(key), OPERATION.ADD);
108113
}
109114

115+
setValue[$changes].setParent(this, obj[$changes].root, key);
116+
110117
} else {
111118
obj.$changeAt(Number(key), setValue);
112119
}

‎src/types/custom/MapSchema.ts

Lines changed: 24 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -86,41 +86,38 @@ export class MapSchema<V=any, K extends string = string> implements Map<K, V>, C
8686
key = key.toString() as K;
8787

8888
const changeTree = this[$changes];
89+
const isRef = (value[$changes]) !== undefined;
8990

90-
// get "index" for this value.
91-
const isReplace = typeof(changeTree.indexes[key]) !== "undefined";
91+
let index: number;
92+
let operation: OPERATION;
9293

93-
const index = (isReplace)
94-
? changeTree.indexes[key]
95-
: changeTree.indexes[$numFields] ?? 0;
94+
// IS REPLACE?
95+
if (typeof(changeTree.indexes[key]) !== "undefined") {
96+
index = changeTree.indexes[key];
97+
operation = OPERATION.REPLACE;
9698

97-
let operation: OPERATION = (isReplace)
98-
? OPERATION.REPLACE
99-
: OPERATION.ADD;
99+
const previousValue = this.$items.get(key);
100+
if (previousValue === value) {
101+
// if value is the same, avoid re-encoding it.
102+
return;
100103

101-
const isRef = (value[$changes]) !== undefined;
104+
} else if (isRef) {
105+
// if is schema, force ADD operation if value differ from previous one.
106+
operation = OPERATION.DELETE_AND_ADD;
107+
108+
// remove reference from previous value
109+
if (previousValue !== undefined) {
110+
previousValue[$changes].root?.remove(previousValue[$changes]);
111+
}
112+
}
113+
114+
} else {
115+
index = changeTree.indexes[$numFields] ?? 0;
116+
operation = OPERATION.ADD;
102117

103-
//
104-
// (encoding)
105-
// set a unique id to relate directly with this key/value.
106-
//
107-
if (!isReplace) {
108118
this.$indexes.set(index, key);
109119
changeTree.indexes[key] = index;
110120
changeTree.indexes[$numFields] = index + 1;
111-
112-
} else if (
113-
!isRef &&
114-
this.$items.get(key) === value
115-
) {
116-
// if value is the same, avoid re-encoding it.
117-
return;
118-
119-
} else if (
120-
isRef && // if is schema, force ADD operation if value differ from previous one.
121-
this.$items.get(key) !== value
122-
) {
123-
operation = OPERATION.ADD;
124121
}
125122

126123
this.$items.set(key, value);

0 commit comments

Comments
 (0)