Skip to content

Commit 7f7c875

Browse files
endelendel
committed
StateView: add 'isVisibilitySharedWithParent' to ChangeTree
Add "should not be required to manually call view.add() items to child arrays without @view() tag" test scenario, which covers automatically adding Schema child items to StateView if parent "collection" type is already part of the StateView.
1 parent 5b17088 commit 7f7c875

File tree

9 files changed

+99
-27
lines changed

9 files changed

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

‎src/Schema.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ export class Schema {
6969

7070
} else if (tag === DEFAULT_VIEW_TAG) {
7171
// view pass: default tag
72-
return view.visible.has(ref[$changes]);
72+
return view.isChangeTreeVisible(ref[$changes]);
7373

7474
} else {
7575
// view pass: custom tag

‎src/encoder/ChangeTree.ts

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ export class ChangeTree<T extends Ref=any> {
122122
* Whether this structure is parent of a filtered structure.
123123
*/
124124
isFiltered: boolean = false;
125+
isVisibilitySharedWithParent?: boolean; // See test case: 'should not be required to manually call view.add() items to child arrays without @view() tag'
125126

126127
indexedOperations: IndexedOperations = {};
127128

@@ -327,20 +328,6 @@ export class ChangeTree<T extends Ref=any> {
327328
for (const key in changeSet.indexes) {
328329
newIndexes[newKey++] = changeSet.indexes[key];
329330
}
330-
331-
// const newIndexes = {};
332-
// let newKey = 0;
333-
// for (const key in changeSet.indexes) {
334-
// const index = changeSet.indexes[key];
335-
// newIndexes[newKey++] = changeSet.indexes[key];
336-
// console.log("...shiftAllChangeIndexes", { index: key, targetIndex: index, startIndex, shiftIndex });
337-
// if (index > startIndex) {
338-
// newIndexes[Number(key) + shiftIndex] = index;
339-
// } else {
340-
// newIndexes[Number(key)] = index;
341-
// }
342-
// }
343-
344331
changeSet.indexes = newIndexes;
345332

346333
for (let i = 0; i < changeSet.operations.length; i++) {
@@ -564,10 +551,16 @@ export class ChangeTree<T extends Ref=any> {
564551
? this.ref.constructor
565552
: this.ref[$childType];
566553

567-
if (!Metadata.isValidInstance(parent)) {
568-
const parentChangeTree = parent[$changes];
554+
let parentChangeTree: ChangeTree;
555+
556+
let parentIsCollection = !Metadata.isValidInstance(parent);
557+
if (parentIsCollection) {
558+
parentChangeTree = parent[$changes];
569559
parent = parentChangeTree.parent;
570560
parentIndex = parentChangeTree.parentIndex;
561+
562+
} else {
563+
parentChangeTree = parent[$changes]
571564
}
572565

573566
const parentConstructor = parent.constructor as typeof Schema;
@@ -578,15 +571,25 @@ export class ChangeTree<T extends Ref=any> {
578571
}
579572
key += `-${parentIndex}`;
580573

574+
const fieldHasViewTag = parentConstructor?.[Symbol.metadata]?.[$viewFieldIndexes]?.includes(parentIndex);
575+
581576
this.isFiltered = parent[$changes].isFiltered // in case parent is already filtered
582577
|| this.root.types.parentFiltered[key]
583-
|| parentConstructor?.[Symbol.metadata]?.[$viewFieldIndexes]?.includes(parentIndex);
578+
|| fieldHasViewTag;
584579

585580
//
586581
// "isFiltered" may not be imedialely available during `change()` due to the instance not being attached to the root yet.
587582
// when it's available, we need to enqueue the "changes" changeset into the "filteredChanges" changeset.
588583
//
589584
if (this.isFiltered) {
585+
586+
this.isVisibilitySharedWithParent = (
587+
parentChangeTree.isFiltered &&
588+
typeof (refType) !== "string" &&
589+
!fieldHasViewTag &&
590+
parentIsCollection
591+
);
592+
590593
if (!this.filteredChanges) {
591594
this.filteredChanges = createChangeSet();
592595
this.allFilteredChanges = createChangeSet();

‎src/encoder/Encoder.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,12 @@ export class Encoder<T extends Schema = any> {
6161
if (!changeTree) { continue; }
6262

6363
if (hasView) {
64-
if (!view.visible.has(changeTree)) {
64+
if (!view.isChangeTreeVisible(changeTree)) {
65+
// console.log("MARK AS INVISIBLE:", { ref: changeTree.ref.constructor.name, refId: changeTree.refId, raw: changeTree.ref.toJSON() });
6566
view.invisible.add(changeTree);
6667
continue; // skip this change tree
67-
68-
} else {
69-
view.invisible.delete(changeTree); // remove from invisible list
7068
}
69+
view.invisible.delete(changeTree); // remove from invisible list
7170
}
7271

7372
const changeSet = changeTree[changeSetName];

‎src/encoder/StateView.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,4 +291,28 @@ export class StateView {
291291
// clear items array
292292
this.items.length = 0;
293293
}
294+
295+
isChangeTreeVisible(changeTree: ChangeTree) {
296+
let isVisible = this.visible.has(changeTree);
297+
298+
//
299+
// TODO: avoid checking for parent visibility, most of the time it's not needed
300+
// See test case: 'should not be required to manually call view.add() items to child arrays without @view() tag'
301+
//
302+
if (!isVisible && changeTree.isVisibilitySharedWithParent){
303+
304+
// console.log("CHECK AGAINST PARENT...", {
305+
// ref: changeTree.ref.constructor.name,
306+
// refId: changeTree.refId,
307+
// parent: changeTree.parent.constructor.name,
308+
// });
309+
310+
if (this.visible.has(changeTree.parent[$changes])) {
311+
this.visible.add(changeTree);
312+
isVisible = true;
313+
}
314+
}
315+
316+
return isVisible;
317+
}
294318
}

‎src/types/custom/ArraySchema.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,7 @@ export class ArraySchema<V = any> implements Array<V>, Collection<number, V> {
4141
return (
4242
!view ||
4343
typeof (ref[$childType]) === "string" ||
44-
// view.items.has(ref[$getByIndex](index)[$changes])
45-
view.visible.has(ref['tmpItems'][index]?.[$changes])
44+
view.isChangeTreeVisible(ref['tmpItems'][index]?.[$changes])
4645
);
4746
}
4847

‎src/types/custom/CollectionSchema.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ export class CollectionSchema<V=any> implements Collection<K, V>{
3333
return (
3434
!view ||
3535
typeof (ref[$childType]) === "string" ||
36-
view.visible.has((ref[$getByIndex](index) ?? ref.deletedItems[index])[$changes])
36+
view.isChangeTreeVisible((ref[$getByIndex](index) ?? ref.deletedItems[index])[$changes])
3737
);
3838
}
3939

‎src/types/custom/MapSchema.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ export class MapSchema<V=any, K extends string = string> implements Map<K, V>, C
3434
return (
3535
!view ||
3636
typeof (ref[$childType]) === "string" ||
37-
view.visible.has((ref[$getByIndex](index) ?? ref.deletedItems[index])[$changes])
37+
view.isChangeTreeVisible((ref[$getByIndex](index) ?? ref.deletedItems[index])[$changes])
3838
);
3939
}
4040

‎test/StateView.test.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1411,6 +1411,53 @@ describe("StateView", () => {
14111411
assert.doesNotThrow(() =>
14121412
encodeAllMultiple(encoder, state, [client1]));
14131413
});
1414+
1415+
it("should not be required to manually call view.add() items to child arrays without @view() tag", () => {
1416+
class Item extends Schema {
1417+
@type("string") name: string;
1418+
}
1419+
class Entity extends Schema {
1420+
@type(["string"]) strings: string[];
1421+
@type([Item]) items: Item[];
1422+
}
1423+
1424+
class State extends Schema {
1425+
@view() @type([Entity]) entities = new ArraySchema<Entity>();
1426+
}
1427+
1428+
const state = new State();
1429+
for (let i = 0; i < 5; i++) {
1430+
state.entities.push(new Entity().assign({
1431+
strings: ["one"],
1432+
items: [new Item().assign({ name: "one" })]
1433+
}));
1434+
}
1435+
1436+
const encoder = getEncoder(state);
1437+
1438+
const client = createClientWithView(state);
1439+
client.view.add(state.entities.at(3));
1440+
1441+
encodeMultiple(encoder, state, [client]);
1442+
1443+
assert.strictEqual(client.state.entities.length, 1);
1444+
assert.strictEqual(1, client.state.entities[0].strings.length);
1445+
assert.strictEqual(1, client.state.entities[0].items.length);
1446+
assert.strictEqual("one", client.state.entities[0].strings[0]);
1447+
assert.strictEqual("one", client.state.entities[0].items[0].name);
1448+
1449+
assert.ok(client.view.isChangeTreeVisible(state.entities.at(3).items[$changes]))
1450+
assert.ok(client.view.isChangeTreeVisible(state.entities.at(3).strings[$changes]))
1451+
1452+
state.entities.at(3).strings.push("two");
1453+
state.entities.at(3).items.push(new Item().assign({ name: "two" }));
1454+
1455+
encodeMultiple(encoder, state, [client]);
1456+
assert.strictEqual(2, client.state.entities[0].strings.length);
1457+
assert.strictEqual(2, client.state.entities[0].items.length);
1458+
1459+
assertEncodeAllMultiple(encoder, state, [client])
1460+
});
14141461
});
14151462

14161463
describe("Deep and nested structures", () => {

0 commit comments

Comments
 (0)