Fix pinning state changes #4721
Conversation
| @@ -0,0 +1,89 @@ | |||
| import pinnedEntityToGltf from "./pinned-entity-to-gltf"; | |||
There was a problem hiding this comment.
The functions in the PinningHelper class below are almost identical to the code pulled out of scene-entry-manager.js, except that _pinElement and unpinElement now set the "pinned" state of the "pinnable" component and also emit the "pinned" and "unpinned" events accordingly when un/pinning has actually succeeded.
| @@ -3,7 +3,7 @@ import configs from "./configs"; | |||
|
|
|||
| const nonCorsProxyDomains = (configs.NON_CORS_PROXY_DOMAINS || "").split(","); | |||
| if (configs.CORS_PROXY_SERVER) { | |||
| nonCorsProxyDomains.push(configs.CORS_PROXY_SERVER); | |||
| nonCorsProxyDomains.push(configs.CORS_PROXY_SERVER.split(":")[0]); | |||
There was a problem hiding this comment.
Small fix for when your CORS_PROXY_SERVER has a port number in it, like "hubs-proxy.local:4000". This was breaking the loading of GLB files.
| }); | ||
|
|
||
| if (this.el.components["body-helper"] && !this.el.sceneEl.systems.interaction.isHeld(this.el)) { | ||
| this.el.setAttribute("body-helper", { type: "kinematic" }); |
There was a problem hiding this comment.
Mostly indentation changes here. I'd suggest viewing the diff with whitespace hidden.
| @@ -188,7 +187,7 @@ export default class SceneEntryManager { | |||
|
|
|||
| if (entity.components.networked.data.persistent) { | |||
| NAF.utils.takeOwnership(entity); | |||
| this._unpinElement(entity); | |||
| window.APP.pinningHelper.unpinElement(entity); | |||
| entity.parentNode.removeChild(entity); | |||
| } else { | |||
| NAF.entities.removeEntity(id); | |||
There was a problem hiding this comment.
(Unrelated to your changes)
I wonder if this else branch is needed here, or if there is already a code path that will remove (non-persistent) networked entities after their creator leaves the room.
There was a problem hiding this comment.
I think you're right that objects will be removed when the kicked client leaves, but I think there's merit to begin explicit and immediate here, since the intent is to get rid of unwanted content forcefully.
This fixes the flow of events when an object is pinned in a room.
Previously, pinning an object would cause the "pinned" state to be set on the "pinnable" component, which in turn would emit a "pinned" event. The "pinned" event was caught by a global handler which would start the sign-in flow and then actually persist the state to the server with a "pin" message on the hubChannel. This caused issues because the "pinned" state would have been set optimistically, before the user had signed in, and before the state had been persisted to the server, which meant that various parts of the UI would reflect this state early and incorrectly.
This PR changes the entry point of the pinning action. It now starts by attempting a sign-in flow, persisting the state to the server, and only then setting the "pinned" state and emitting the "pinned" event.