StateView: fix more scenarios / add more tests.

Author: endel

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@colyseus/schema",
-  "version": "3.0.12",
+  "version": "3.0.13",
   "description": "Binary state serializer with delta encoding for games",
   "bin": {
     "schema-codegen": "./bin/schema-codegen",

src/encoder/ChangeTree.ts

@@ -394,6 +394,8 @@ export class ChangeTree<T extends Ref=any> {
         } else {
             enqueueChangeTree(this.root, this, 'changes');
         }
+
+        return previousValue;
     }
 
     endEncode() {

src/encoder/Encoder.ts

@@ -1,6 +1,6 @@
 import type { Schema } from "../Schema";
 import { TypeContext } from "../types/TypeContext";
-import { $changes, $encoder, $filter } from "../types/symbols";
+import { $changes, $encoder, $filter, $getByIndex } from "../types/symbols";
 
 import { encode } from "../encoding/encode";
 import type { Iterator } from "../encoding/decode";
@@ -10,6 +10,7 @@ import { Root } from "./Root";
 
 import type { StateView } from "./StateView";
 import type { Metadata } from "../Metadata";
+import type { ChangeTree } from "./ChangeTree";
 
 export class Encoder<T extends Schema = any> {
     static BUFFER_SIZE = (typeof(Buffer) !== "undefined") && Buffer.poolSize || 8 * 1024; // 8KB
@@ -204,7 +205,7 @@ export class Encoder<T extends Schema = any> {
 
         // encode visibility changes (add/remove for this view)
         for (const [refId, changes] of view.changes) {
-            const changeTree = this.root.changeTrees[refId];
+            const changeTree: ChangeTree = this.root.changeTrees[refId];
 
             if (changeTree === undefined) {
                 // detached instance, remove from view and skip.
@@ -229,12 +230,14 @@ export class Encoder<T extends Schema = any> {
             encode.number(bytes, changeTree.refId, it);
 
             for (let i = 0, numChanges = keys.length; i < numChanges; i++) {
-                const key = keys[i];
-                const operation = changes[key];
+                const index = Number(keys[i]);
+                // workaround when using view.add() on item that has been deleted from state (see test "adding to view item that has been removed from state")
+                const value = changeTree.ref[$getByIndex](index);
+                const operation = (value !== undefined && changes[index]) || OPERATION.DELETE;
 
                 // isEncodeAll = false
                 // hasView = true
-                encoder(this, bytes, changeTree, Number(key), operation, it, false, true, metadata);
+                encoder(this, bytes, changeTree, index, operation, it, false, true, metadata);
             }
         }
 

src/encoder/StateView.ts

@@ -123,22 +123,18 @@ export class StateView {
         const changeTree = childChangeTree.parent[$changes];
         const parentIndex = childChangeTree.parentIndex;
 
-        // view must have all "changeTree" parent tree
-        this.items.add(changeTree);
-
-        // add parent's parent
-        const parentChangeTree: ChangeTree = changeTree.parent?.[$changes];
-        if (parentChangeTree && (parentChangeTree.filteredChanges !== undefined)) {
-            this.addParentOf(changeTree, tag);
-        }
+        if (!this.items.has(changeTree)) {
+            // view must have all "changeTree" parent tree
+            this.items.add(changeTree);
+
+            // add parent's parent
+            const parentChangeTree: ChangeTree = changeTree.parent?.[$changes];
+            if (parentChangeTree && (parentChangeTree.filteredChanges !== undefined)) {
+                this.addParentOf(changeTree, tag);
+            }
 
-        if (
             // parent is already available, no need to add it!
-            !this.invisible.has(changeTree) &&
-            // item is being replaced, no need to add parent
-            changeTree.indexedOperations[parentIndex] !== OPERATION.DELETE_AND_ADD
-        ) {
-            return;
+            if (!this.invisible.has(changeTree)) { return; }
         }
 
         // add parent's tag properties
@@ -229,4 +225,13 @@ export class StateView {
 
         return this;
     }
+
+    has(obj: Ref) {
+        return this.items.has(obj[$changes]);
+    }
+
+    hasTag(ob: Ref, tag: number = DEFAULT_VIEW_TAG) {
+        const tags = this.tags?.get(ob[$changes]);
+        return tags?.has(tag) ?? false;
+    }
 }

src/types/custom/MapSchema.ts

@@ -14,6 +14,7 @@ export class MapSchema<V=any, K extends string = string> implements Map<K, V>, C
 
     protected $items: Map<K, V> = new Map<K, V>();
     protected $indexes: Map<number, K> = new Map<number, K>();
+    protected deletedItems: { [field: string]: V } = {};
 
     protected [$changes]: ChangeTree;
 
@@ -31,9 +32,9 @@ export class MapSchema<V=any, K extends string = string> implements Map<K, V>, C
      */
     static [$filter] (ref: MapSchema, index: number, view: StateView) {
         return (
-            !view  ||
+            !view ||
             typeof (ref[$childType]) === "string" ||
-            view.items.has(ref[$getByIndex](index)[$changes])
+            view.items.has((ref[$getByIndex](index) ?? ref.deletedItems[index])[$changes])
         );
     }
 
@@ -142,7 +143,7 @@ export class MapSchema<V=any, K extends string = string> implements Map<K, V>, C
     delete(key: K) {
         const index = this[$changes].indexes[key];
 
-        this[$changes].delete(index);
+        this.deletedItems[index] = this[$changes].delete(index);;
 
         return this.$items.delete(key);
     }
@@ -206,19 +207,7 @@ export class MapSchema<V=any, K extends string = string> implements Map<K, V>, C
     }
 
     protected [$onEncodeEnd]() {
-        const changeTree = this[$changes];
-        const keys = Object.keys(changeTree.indexedOperations);
-
-        for (let i = 0, len = keys.length; i < len; i++) {
-            const key = keys[i];
-            const fieldIndex = Number(key);
-            const operation = changeTree.indexedOperations[key];
-
-            if (operation === OPERATION.DELETE) {
-                const index = this[$getByIndex](fieldIndex) as string;
-                delete changeTree.indexes[index];
-            }
-        }
+        this.deletedItems = {};
     }
 
     toJSON() {

test/Schema.ts

@@ -120,11 +120,13 @@ export function encodeMultiple<T extends Schema>(encoder: Encoder<T>, state: T,
     const it = { offset: 0 };
 
     // perform shared encode
+    // console.log("\n\n\n\n>> SHARED ENCODE!");
     encoder.encode(it);
 
     const sharedOffset = it.offset;
     const encodedViews = clients.map((client, i) => {
         // encode each view
+        // console.log(">> ENCODE VIEW!", i + 1);
         const encoded = encoder.encodeView(client.view, sharedOffset, it);
         client.state.decode(encoded);
         return encoded;

test/StateView.test.ts

@@ -280,13 +280,16 @@ describe("StateView", () => {
             client1.view.add(state.item, Tag.TWO);
             const encodedTag2 = encodeMultiple(encoder, state, [client1])[0];
 
+            // TODO: can we reduce the amount of bytes here?
+            // assert.strictEqual(4, Array.from(encodedTag1).length, "should encode only the new field");
+
             // compare encode1 with encode2
-            assert.strictEqual(4, Array.from(encodedTag1).length, "should encode only the new field");
+            assert.strictEqual(8, Array.from(encodedTag1).length, "should encode only the new field");
             assert.strictEqual(Array.from(encodedTag1).length, Array.from(encodedTag2).length, "encode size should be the same");
             assert.strictEqual(Array.from(encodedTag1)[0], Array.from(encodedTag2)[0]);
             assert.strictEqual(Array.from(encodedTag1)[1], Array.from(encodedTag2)[1]);
-            assert.strictEqual(Array.from(encodedTag1)[2] + 1, Array.from(encodedTag2)[2]); // field index (+1 so 1 -> 2)
-            assert.strictEqual(Array.from(encodedTag1)[3] + 10, Array.from(encodedTag2)[3]); // value (+ 10 so 20 -> 30)
+            assert.strictEqual(Array.from(encodedTag1)[2], Array.from(encodedTag2)[2]);
+            assert.strictEqual(Array.from(encodedTag1)[3], Array.from(encodedTag2)[3]);
 
             assert.strictEqual(10, client1.state.item.amount);
             assert.strictEqual(20, client1.state.item.fov1);
@@ -432,7 +435,6 @@ describe("StateView", () => {
 
             assert.strictEqual(client2.state.prop1, state.prop1);
             assert.strictEqual(client2.state.items, undefined);
-
             assertEncodeAllMultiple(encoder, state, [client1])
         });
 
@@ -479,7 +481,17 @@ describe("StateView", () => {
             assert.strictEqual(client2.state.items.size, 2);
             assert.strictEqual(client2.state.items.get("4").amount, state.items.get("4").amount);
             assert.strictEqual(client2.state.items.get("5").amount, state.items.get("5").amount);
-            //
+
+            assertEncodeAllMultiple(encoder, state, [client1, client2])
+
+            // removing items...
+            state.items.delete("2"); // none of the clients have this item
+            state.items.delete("4"); // shared item between client1 and client2
+            state.items.delete("6"); // none of the clients have this item
+            encodeMultiple(encoder, state, [client1, client2]);
+
+            assert.strictEqual(undefined, client1.state.items.get("4"));
+            assert.strictEqual(undefined, client2.state.items.get("4"));
 
             assertEncodeAllMultiple(encoder, state, [client1, client2])
         });
@@ -614,6 +626,39 @@ describe("StateView", () => {
 
             assert.strictEqual(undefined, client2.state.entities);
         });
+
+        it("adding to view item that has been removed from state", () => {
+            class Item extends Schema {
+                @type("number") amount: number;
+            }
+
+            class State extends Schema {
+                @view() @type({ map: Item }) items = new MapSchema<Item>();
+            }
+
+            const state = new State();
+            const encoder = getEncoder(state);
+
+            const client1 = createClientWithView(state);
+            encodeMultiple(encoder, state, [client1]);
+
+            for (let i = 0; i < 5; i++) {
+                state.items.set(i.toString(), new Item().assign({ amount: i }));
+            }
+
+            client1.view.add(state.items.get("3"));
+            encodeMultiple(encoder, state, [client1]);
+
+            assert.strictEqual(client1.state.items.size, 1);
+            assert.strictEqual(client1.state.items.get("3").amount, state.items.get("3").amount);
+
+            client1.view.add(state.items.get("3"));
+            state.items.delete("3");
+
+            encodeMultiple(encoder, state, [client1]);
+            assert.strictEqual(client1.state.items.size, 0);
+
+        });
     });
 
     describe("ArraySchema", () => {
@@ -832,6 +877,47 @@ describe("StateView", () => {
             assertEncodeAllMultiple(encoder, state, [client1])
         });
 
+        it("removing and item should remove from their views", () => {
+            class Item extends Schema {
+                @type("number") amount: number;
+            }
+
+            class State extends Schema {
+                @view() @type([Item]) items = new ArraySchema<Item>();
+            }
+
+            const state = new State();
+            for (let i = 0; i < 5; i++) {
+                state.items.push(new Item().assign({ amount: i }));
+            }
+
+            const encoder = getEncoder(state);
+
+            const client1 = createClientWithView(state);
+            const client2 = createClientWithView(state);
+
+            client1.view.add(state.items.at(2));
+            client1.view.add(state.items.at(3));
+
+            client2.view.add(state.items.at(3));
+            client2.view.add(state.items.at(4));
+
+            encodeMultiple(encoder, state, [client1, client2]);
+
+            assert.deepStrictEqual(client1.state.items.map(i => i.amount), [2, 3]);
+            assert.deepStrictEqual(client2.state.items.map(i => i.amount), [3, 4]);
+
+            state.items.splice(3, 1)
+            assert.deepStrictEqual(state.items.map(i => i.amount), [0, 1, 2, 4]);
+
+            encodeMultiple(encoder, state, [client1, client2]);
+
+            assert.deepStrictEqual(client1.state.items.map(i => i.amount), [2]);
+            assert.deepStrictEqual(client2.state.items.map(i => i.amount), [4]);
+
+            assertEncodeAllMultiple(encoder, state, [client1, client2])
+        });
+
         it("visibility change should trigger onAdd/onRemove on arrays", () => {
             class Item extends Schema {
                 @type("number") amount: number;
@@ -925,74 +1011,6 @@ describe("StateView", () => {
                 encodeAllMultiple(encoder, state, [client1]));
         });
 
-        xit("nested with MapSchema", () => {
-            // TODO: couldn't reproduce original issue reported by @Indalamar
-            class Component extends Schema {
-                @type('string') name: string;
-            }
-            class Spell extends Schema {
-                @type('number') id: number;
-                @type('string') name: string;
-                @type(['number']) relation = new ArraySchema<number>();
-                constructor() {
-                    super();
-                    this.relation.push(1, 2, 3);
-                }
-            }
-            class SpellBook extends Component {
-                @type({ map: Spell }) spells: Map<string, Spell>;
-                constructor() {
-                    super();
-                    this.name = "SpellBook";
-                    this.spells = new MapSchema<Spell>();
-                    this.spells.set("one", new Spell().assign({ id: 1, name: "Fireball", }));
-                    this.spells.set("two", new Spell().assign({ id: 2, name: "Iceball", }));
-                    this.spells.set("three", new Spell().assign({ id: 3, name: "Thunderball", }));
-                }
-            }
-            class Entity extends Schema {
-                @type("string") id: string = nanoid(9);
-                @type([Component]) components: Component[];
-                constructor() {
-                    super();
-                    this.components = new ArraySchema<Component>()
-                    this.components.push(new SpellBook());
-                }
-            }
-            class State extends Schema {
-                @view() @type({ map: Entity }) entities = new MapSchema<Entity>();
-            }
-
-            const state = new State();
-            const encoder = getEncoder(state);
-
-            const client1 = createClientWithView(state);
-            const client2 = createClientWithView(state);
-
-            encodeMultiple(encoder, state, [client1, client2]);
-
-            const entity1 = new Entity();
-            entity1.components.push(new Component().assign({ name: "health" }));
-            state.entities.set("one", entity1);
-
-            client1.view.add(entity1);
-            encodeMultiple(encoder, state, [client1, client2]);
-
-            const entity2 = new Entity();
-            entity2.components.push(new Component().assign({ name: "health" }));
-            state.entities.set("one", entity2);
-
-            client2.view.add(entity2);
-            client2.view.add(entity1);
-
-            encodeMultiple(encoder, state, [client1, client2]);
-
-            console.log(client1.state.toJSON());
-            console.log(client2.state.toJSON());
-
-            assertEncodeAllMultiple(encoder, state, [client1, client2])
-        })
-
         it("setting a non-view field to undefined should not interfere on encoding", () => {
             class Song extends Schema {
                 @type("string") url: string;