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.

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

Audio zone fixes #4278

Merged
merged 7 commits into from
Jun 3, 2021
Merged

Audio zone fixes #4278

merged 7 commits into from
Jun 3, 2021

Conversation

netpro2k
Copy link
Contributor

@netpro2k netpro2k commented May 21, 2021

  • Fixes Audio Source Components Do Not Move With Objects #4276 by taking into account the world space position of zone audio sources when doing distances checks
  • Uses the new amplify_audio permission added in Add amplify_audio permission reticulum#499 to enforce modOnly on zone audio sources. Previously this was using "owner" but this does not work well for bound rooms.
  • Fixed an unrelated bug I noticed in hub-channel.js that was incorrectly concatenating arrays using the + operator.. It turned out this mostly just "worked" anyway since the arrays were contacted into a big string, and strings also have an includes method which happened to also be correct for this case. Oh javascript.

@netpro2k netpro2k requested a review from brianpeiris May 21, 2021 03:57
Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

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

Spotted a typo that needs fixing. Other minor comments, but looks good otherwise.

src/components/player-info.js Outdated Show resolved Hide resolved
@@ -120,12 +120,16 @@ AFRAME.registerComponent("player-info", {
this.updateDisplayNameFromPresenceMeta(e.detail);
},
updateDisplayNameFromPresenceMeta(presenceMeta) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the contents of this function has drifted from its behavior. Maybe updatePlayerInfoFromPresenceMeta?

src/components/player-info.js Show resolved Hide resolved
"pin_objects",
"spawn_emoji",
"fly"
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wow, nice catch 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah at first I was like "Wait, js has an array concatenation operator? And then I realized what was going on... Oh javascript. Funny that it just happened to work mostly correctly by chance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good to see it's fixed, noticed that a year ago and I was just about to push a PR for that!
I wondered too at first if I was missing some obscure syntax XD

netpro2k and others added 3 commits May 24, 2021 11:54
@netpro2k netpro2k merged commit e1dcbff into master Jun 3, 2021
@netpro2k netpro2k deleted the audio-zone-fixes branch June 3, 2021 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audio Source Components Do Not Move With Objects
3 participants