-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lobby #594
Lobby #594
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments I left about loadedSceneUrl
and environment-root
probably need to be addressed.
src/scene-entry-manager.js
Outdated
}); | ||
const hudController = this.playerRig.querySelector("[hud-controller]"); | ||
hudController.setAttribute("hud-controller", { showTip: !this.store.state.activity.hasFoundFreeze }); | ||
document.querySelector("a-scene").emit("username-changed", { username: displayName }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use this.scene
instead of querying the selector again.
src/scene-entry-manager.js
Outdated
if (screenEntity) { | ||
screenEntity.setAttribute("visible", sharingScreen); | ||
} else if (sharingScreen) { | ||
const sceneEl = document.querySelector("a-scene"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use this.scene
src/hub.js
Outdated
e.preventDefault(); | ||
}); | ||
if (isBotMode) { | ||
entryManager.enterSceneWhenLoaded(new MediaStream(), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is never called with true
instead of false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's OK -- i defined the public interface of this function to match that of enterScene
|
||
// Connect to reticulum over phoenix channels to get hub info. | ||
const hubId = qs.get("hub_id") || document.location.pathname.substring(1).split("/")[0]; | ||
console.log(`Hub ID: ${hubId}`); | ||
|
||
const socket = connectToReticulum(isDebug); | ||
const channel = socket.channel(`hub:${hubId}`, {}); | ||
let loadedSceneUrl = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this loadedSceneUrl
mechanism was removed in the refactor. Wasn't it needed for a proper re-join?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, yeah so the way I detect if its a reconnect is via NAF.connection.isConnected()
-- I think that incidentally works but probably not the best thing to check. any suggestions?
@@ -459,7 +456,9 @@ | |||
id="environment-root" | |||
nav-mesh-helper | |||
static-body="shape: none;" | |||
></a-entity> | |||
> | |||
<a-entity id="environment-scene"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've replaced environment-root
with environment-scene
? Looks like environment-root
is not referenced anywhere now. You'll probably want to move the nav-mesh-helper
and static-body
components over if that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I believe the scene structure is unchanged, but previously the join code created the environment-scene
element and a few things closed over it and used it (it did not have a name in the dom, it was passed around in a var.) so i just moved that into the DOM directly so those closures could get lifted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add a pre entry lobby so users can see what is going on before they chose to join a room.
Does a ton of refactoring and cleanup of
hub.js
, breaking some code out intoSceneEntryManager
and lifting a bunch of closures into top level functions, among other things. (hopefully this makes @mquander happy :)) There's more I could probably do but this is a good first step.Removes the speculative Daydream entry mode
Changes the hub landing page to connect to Janus and put the user into a spectator mode which automatically pans the camera through the scene, but allows the user to rotate it.
More visual design changes forthcoming but this is a basic "lobby" mode. This has two benefits: it allows the user to see what is going on in the room before entering, and also as a bonus causes avatars, objects, etc, to load before entering VR, which avoids the worst hitching.