Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Re-implemented NAF player position syncing #17

Merged
merged 3 commits into from
Sep 14, 2020
Merged

Re-implemented NAF player position syncing #17

merged 3 commits into from
Sep 14, 2020

Conversation

JoshuaKGoldberg
Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg commented Sep 7, 2020 •
edited
Loading

Fixes #5.

This was the real deal to get it working again:

-   occupants: Object.fromEntries(room.occupants),
+   occupants: mapValues(
+     Object.fromEntries(room.occupants),
+       (person) => person.joinedTime
+   ),

I was hoping audio: true; and the nametag syncing would also work. No such luck 😒

@JoshuaKGoldberg JoshuaKGoldberg changed the title Re-implemented NAF player positionin syncing Re-implemented NAF player position syncing Sep 7, 2020
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review September 7, 2020 14:16
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for reviewer This PR should be reviewed by another developer. label Sep 7, 2020
@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Sep 7, 2020
Open
JoshuaKGoldberg
JoshuaKGoldberg commented Sep 7, 2020
View reviewed changes
src/connection/useRoomConnection.ts
@@ -33,7 +33,6 @@ export const useRoomConnection = (roomName: string) => {
// The `onConnect` setting tells it to call the global function under that name when ready.
sceneElement.setAttribute("networked-scene", {
adapter: "webrtc",
audio: "true",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#18: you should try this out locally. When this line is added back in, player positions don't sync (on localhost)...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what the.... and why's that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha I would very much like to know. It might be an issue with using WebRTC over localhost, which would explain why it didn't happen on Glitch.

@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Sep 7, 2020
Open
fedGL
fedGL approved these changes Sep 9, 2020
View reviewed changes
Copy link

@fedGL fedGL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In src/pages/room/RoomBody/template.html you got rid of dynamic-room attribute in a-scene because...? I'm assuming it has to do with the way you are refactoring NAF? Other than that, LGTM!

@JoshuaKGoldberg
Copy link
Collaborator Author

@fedGL great question! dynamic-room doesn't actually do anything for NAF: it's not referenced in the source code at all (search). It seems to me that it was just added in the Glitch examples to make it easier or more clear how to query the a-scene.

Adding it back in didn't change any of the behavior in this PR for me, including the audio bug.

@fedGL
Copy link

fedGL commented Sep 11, 2020

Interesting... I found this dynamic room component that was doing AFRAME.registerComponent('dynamic-room' and I thought that's why we had it

@JoshuaKGoldberg
Copy link
Collaborator Author

Oh yes that's right! And now that we attach the logic via JavaScript, the AFRAME.registerComponent doesn't do much for us... although I'd be curious to see if there's logic from that Glitch that would help with the audio issues...

@sonnynomnom sonnynomnom merged commit 0114314 into main Sep 14, 2020
@JoshuaKGoldberg JoshuaKGoldberg deleted the naf-player-sync branch September 14, 2020 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedGL fedGL fedGL approved these changes

@sonnynomnom sonnynomnom Awaiting requested review from sonnynomnom

Assignees
No one assigned
Labels
status: waiting for reviewer This PR should be reviewed by another developer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Networked-Aframe isn't syncing player positions
3 participants
@JoshuaKGoldberg @fedGL @sonnynomnom