From 2aba6b1c271287f1a26fe5b0c50b68494985d26b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Fri, 23 Jul 2021 11:50:03 +0200 Subject: [PATCH 1/3] Fixing loop when setting variables Setting a variable would makes the application enter in an infinite loop of events (between all the scripts and the back) This fix makes sure a variable does not emit any event if it is changed to a value it already has. --- back/src/Model/GameRoom.ts | 5 +++ back/src/RoomManager.ts | 13 ++++++-- back/src/Services/VariablesManager.ts | 17 +++++++++- front/src/Api/IframeListener.ts | 33 ++++++++++--------- front/src/Api/iframe/state.ts | 6 ++-- .../src/Phaser/Game/SharedVariablesManager.ts | 10 +++++- 6 files changed, 61 insertions(+), 23 deletions(-) diff --git a/back/src/Model/GameRoom.ts b/back/src/Model/GameRoom.ts index 2892a7bd..491dd4af 100644 --- a/back/src/Model/GameRoom.ts +++ b/back/src/Model/GameRoom.ts @@ -327,6 +327,11 @@ export class GameRoom { const readableBy = variableManager.setVariable(name, value, user); + // If the variable was not changed, let's not dispatch anything. + if (readableBy === false) { + return; + } + // TODO: should we batch those every 100ms? const variableMessage = new VariableWithTagMessage(); variableMessage.setName(name); diff --git a/back/src/RoomManager.ts b/back/src/RoomManager.ts index e4a7af39..0465ade6 100644 --- a/back/src/RoomManager.ts +++ b/back/src/RoomManager.ts @@ -77,7 +77,11 @@ const roomManager: IRoomManagerServer = { } else if (message.hasSilentmessage()) { socketManager.handleSilentMessage(room, user, message.getSilentmessage() as SilentMessage); } else if (message.hasItemeventmessage()) { - socketManager.handleItemEvent(room, user, message.getItemeventmessage() as ItemEventMessage); + socketManager.handleItemEvent( + room, + user, + message.getItemeventmessage() as ItemEventMessage + ); } else if (message.hasVariablemessage()) { await socketManager.handleVariableEvent( room, @@ -97,7 +101,10 @@ const roomManager: IRoomManagerServer = { message.getWebrtcscreensharingsignaltoservermessage() as WebRtcSignalToServerMessage ); } else if (message.hasPlayglobalmessage()) { - socketManager.emitPlayGlobalMessage(room, message.getPlayglobalmessage() as PlayGlobalMessage); + socketManager.emitPlayGlobalMessage( + room, + message.getPlayglobalmessage() as PlayGlobalMessage + ); } else if (message.hasQueryjitsijwtmessage()) { socketManager.handleQueryJitsiJwtMessage( user, @@ -128,7 +135,7 @@ const roomManager: IRoomManagerServer = { emitError(call, e); call.end(); } - })().catch(e => console.error(e)); + })().catch((e) => console.error(e)); }); call.on("end", () => { diff --git a/back/src/Services/VariablesManager.ts b/back/src/Services/VariablesManager.ts index 5137a32d..c6b6f5fe 100644 --- a/back/src/Services/VariablesManager.ts +++ b/back/src/Services/VariablesManager.ts @@ -134,7 +134,17 @@ export class VariablesManager { return variable; } - setVariable(name: string, value: string, user: User): string | undefined { + /** + * Sets the variable. + * + * Returns who is allowed to read the variable (the readableby property) or "undefined" if anyone can read it. + * Also, returns "false" if the variable was not modified (because we set it to the value it already has) + * + * @param name + * @param value + * @param user + */ + setVariable(name: string, value: string, user: User): string | undefined | false { let readableBy: string | undefined; if (this.variableObjects) { const variableObject = this.variableObjects.get(name); @@ -159,6 +169,11 @@ export class VariablesManager { readableBy = variableObject.readableBy; } + // If the value is not modified, return false + if (this._variables.get(name) === value) { + return false; + } + this._variables.set(name, value); variablesRepository .saveVariable(this.roomUrl, name, value) diff --git a/front/src/Api/IframeListener.ts b/front/src/Api/IframeListener.ts index d8559aa0..d0c82253 100644 --- a/front/src/Api/IframeListener.ts +++ b/front/src/Api/IframeListener.ts @@ -32,9 +32,9 @@ import type { HasPlayerMovedEvent } from "./Events/HasPlayerMovedEvent"; import { isLoadPageEvent } from "./Events/LoadPageEvent"; import { handleMenuItemRegistrationEvent, isMenuItemRegisterIframeEvent } from "./Events/ui/MenuItemRegisterEvent"; import { SetTilesEvent, isSetTilesEvent } from "./Events/SetTilesEvent"; -import { isSetVariableIframeEvent, SetVariableEvent } from "./Events/SetVariableEvent"; +import type { SetVariableEvent } from "./Events/SetVariableEvent"; -type AnswererCallback = (query: IframeQueryMap[T]['query']) => IframeQueryMap[T]['answer']|PromiseLike; +type AnswererCallback = (query: IframeQueryMap[T]["query"], source: MessageEventSource | null) => IframeQueryMap[T]["answer"] | PromiseLike; /** * Listens to messages from iframes and turn those messages into easy to use observables. @@ -187,7 +187,7 @@ class IframeListener { }; try { - Promise.resolve(answerer(query.data)).then((value) => { + Promise.resolve(answerer(query.data, message.source)).then((value) => { iframe?.contentWindow?.postMessage({ id: queryId, type: query.type, @@ -197,18 +197,6 @@ class IframeListener { } catch (reason) { errorHandler(reason); } - - if (isSetVariableIframeEvent(payload.query)) { - // Let's dispatch the message to the other iframes - for (iframe of this.iframes) { - if (iframe.contentWindow !== message.source) { - iframe.contentWindow?.postMessage({ - 'type': 'setVariable', - 'data': payload.query.data - }, '*'); - } - } - } } else if (isIframeEventWrapper(payload)) { if (payload.type === "showLayer" && isLayerEvent(payload.data)) { this._showLayerStream.next(payload.data); @@ -439,6 +427,21 @@ class IframeListener { public unregisterAnswerer(key: keyof IframeQueryMap): void { delete this.answerers[key]; } + + dispatchVariableToOtherIframes(key: string, value: unknown, source: MessageEventSource | null) { + // Let's dispatch the message to the other iframes + for (const iframe of this.iframes) { + if (iframe.contentWindow !== source) { + iframe.contentWindow?.postMessage({ + 'type': 'setVariable', + 'data': { + key, + value, + } + }, '*'); + } + } + } } export const iframeListener = new IframeListener(); diff --git a/front/src/Api/iframe/state.ts b/front/src/Api/iframe/state.ts index 011be1bc..3b551864 100644 --- a/front/src/Api/iframe/state.ts +++ b/front/src/Api/iframe/state.ts @@ -23,11 +23,11 @@ export const initVariables = (_variables: Map): void => { setVariableResolvers.subscribe((event) => { const oldValue = variables.get(event.key); - // If we are setting the same value, no need to do anything. - if (oldValue === event.value) { + // No need to do this check since it is already performed in SharedVariablesManager + /*if (JSON.stringify(oldValue) === JSON.stringify(event.value)) { return; - } + }*/ variables.set(event.key, event.value); const subject = variableSubscribers.get(event.key); diff --git a/front/src/Phaser/Game/SharedVariablesManager.ts b/front/src/Phaser/Game/SharedVariablesManager.ts index 2d015246..6a06d97e 100644 --- a/front/src/Phaser/Game/SharedVariablesManager.ts +++ b/front/src/Phaser/Game/SharedVariablesManager.ts @@ -54,7 +54,7 @@ export class SharedVariablesManager { }); // When a variable is modified from an iFrame - iframeListener.registerAnswerer("setVariable", (event) => { + iframeListener.registerAnswerer("setVariable", (event, source) => { const key = event.key; const object = this.variableObjects.get(key); @@ -82,10 +82,18 @@ export class SharedVariablesManager { throw new Error(errMsg); } + // Let's stop any propagation of the value we set is the same as the existing value. + if (JSON.stringify(event.value) === JSON.stringify(this._variables.get(key))) { + return; + } + this._variables.set(key, event.value); // Dispatch to the room connection. this.roomConnection.emitSetVariableEvent(key, event.value); + + // Dispatch to other iframes + iframeListener.dispatchVariableToOtherIframes(key, event.value, source); }); } From 88f2bfdf3974815c67e9b485d960c26f1498697b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Fri, 23 Jul 2021 12:19:47 +0200 Subject: [PATCH 2/3] Taking into account persist property The "persist" property was not taken into account and all variables were stored in DB. This change makes sure only variables tagged with "persist" are actually persisted. --- back/src/Services/VariablesManager.ts | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/back/src/Services/VariablesManager.ts b/back/src/Services/VariablesManager.ts index c6b6f5fe..e8aaef25 100644 --- a/back/src/Services/VariablesManager.ts +++ b/back/src/Services/VariablesManager.ts @@ -51,6 +51,17 @@ export class VariablesManager { } const variables = await variablesRepository.loadVariables(this.roomUrl); for (const key in variables) { + // Let's only set variables if they are in the map (if the map has changed, maybe stored variables do not exist anymore) + if (this.variableObjects) { + const variableObject = this.variableObjects.get(key); + if (variableObject === undefined) { + continue; + } + if (!variableObject.persist) { + continue; + } + } + this._variables.set(key, variables[key]); } return this; @@ -146,8 +157,9 @@ export class VariablesManager { */ setVariable(name: string, value: string, user: User): string | undefined | false { let readableBy: string | undefined; + let variableObject: Variable | undefined; if (this.variableObjects) { - const variableObject = this.variableObjects.get(name); + variableObject = this.variableObjects.get(name); if (variableObject === undefined) { throw new Error('Trying to set a variable "' + name + '" that is not defined as an object in the map.'); } @@ -175,9 +187,13 @@ export class VariablesManager { } this._variables.set(name, value); - variablesRepository - .saveVariable(this.roomUrl, name, value) - .catch((e) => console.error("Error while saving variable in Redis:", e)); + + if (variableObject !== undefined && variableObject.persist) { + variablesRepository + .saveVariable(this.roomUrl, name, value) + .catch((e) => console.error("Error while saving variable in Redis:", e)); + } + return readableBy; } From c1cd464a7bdd312bb8cc8c7ed1b74e2a591f9877 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Fri, 23 Jul 2021 12:26:18 +0200 Subject: [PATCH 3/3] Fixing reference to deprecated method in doc --- docs/maps/scripting.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/maps/scripting.md b/docs/maps/scripting.md index 5f645b81..5be57ee1 100644 --- a/docs/maps/scripting.md +++ b/docs/maps/scripting.md @@ -55,10 +55,10 @@ Start by testing this with a simple message sent to the chat. **script.js** ```javascript -WA.sendChatMessage('Hello world', 'Mr Robot'); +WA.chat.sendChatMessage('Hello world', 'Mr Robot'); ``` -The `WA` objects contains a number of useful methods enabling you to interact with the WorkAdventure game. For instance, `WA.sendChatMessage` opens the chat and adds a message in it. +The `WA` objects contains a number of useful methods enabling you to interact with the WorkAdventure game. For instance, `WA.chat.sendChatMessage` opens the chat and adds a message in it. In your browser console, when you open the map, the chat message should be displayed right away.