-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add multiple voice chat activation modes #1506
Conversation
- Push-to-Talk (default) - Push-to-Mute - Toggle - Toggle (Activate on Join)
Right now the voice color is only set once. I could change this in the linked PR though. So it sets a mode (team or global) and the color is set each frame
I'm fine with this being done in a secondary PR. But in general I'm a bit confused. Is this a draft PR? Or are these todos things that should be done in further PRs? Because you say "Things left to do that should probably be new PRs" but also have todos in code, which should be removed |
I've removed the todo and made the affected utility function local. To clarify, this is not a draft, I just wrote down what interactions with existing code I'd want to change as well (didn't want to do all of that here to prevent merge conflicts). Regarding voice color, my intuition would've been to somehow hook into role assignment/changing and modify the color then but if updating the color every frame isn't a big deal performance-wise than that's probably a way simpler solution. |
All UI elements read the color on a frame by frame basis, so this is fine |
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.
Only a small nit, otherwise very very good!
Thanks for the amazing PR and clean code, that was really nice to review!
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. Just a note for the future: Please do not resolve the conversations yourself. We - as the reviewer - close them to keep track of it.
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.
Nice thank you very much 👍
Things left to do that should probably be new PRs: