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

Mumble fixes #2990

Merged
merged 9 commits into from
Dec 26, 2024
Merged

Mumble fixes #2990

merged 9 commits into from
Dec 26, 2024

Conversation

AvarianKnight
Copy link
Contributor

@AvarianKnight AvarianKnight commented Dec 7, 2024

Goal of this PR

  1. Fix a bunch of issues where players would not be able to talk to other users, or hear other users
  2. Fix music mode intent having cut outs for quieter music

How is this PR achieving the goal

Fixes a bunch of different implementation errors, each commit should explain what it does relatively well in the description.

This PR applies to the following area(s)

FiveM, RedM

Successfully tested on

Game builds: 3258

Platforms: Windows

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

- this fixes audio cutting in/out when playing music that is quiet
…s receiving UDP packets

This looks to have been needed for an old issue where FiveM's mumble implementation didn't properly handle resetting the sockets on stream close, which is handled properly now.

Mumble hits a "fast path" for reconnecting a client if it reuses the same socket, ad it can just find the client off of the socket and reset the clients crypt state

Whenever we recreate the socket we instead hit the "slow path" where the server has to iterate over *every* client connected to mumble and try to see if any of them can decrypt the UDP packet that was sent, which is not optimal.
@github-actions github-actions bot added RedM Issues/PRs related to RedM triage Needs a preliminary assessment to determine the urgency and required action labels Dec 7, 2024
Copy link
Contributor

@dalekenium-cfx dalekenium-cfx left a comment

Choose a reason for hiding this comment

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

Nothing jumps out at me, so I will approve this MR; however, I'd like another reviewer to take a look since this is a big set of changes.

@joelwurtz
Copy link
Contributor

joelwurtz commented Dec 9, 2024

Nice to see some improvements, feel correct for most part, and i think having a check on the udp packet like mumble does will indeed improve some connection issue that were not detected before.

If you want to try other implementations, there is this one https://github.com/SOZ-Faut-etre-Sub/ZUMBLE that mimic fivem server mumble implementation, (we use it at lot for some years now without any major issue, don't have the ability to compile the fivem client but since we have user on expiremental build we will be able to give you feedback once it's merged)

- while this *should* never get hit this would prevent us from sending a packet that would get ignored or, in FiveM uMurmurs case disconnected
- swaps the voice check to use `m_hasUdp` instead of checking the time
- this should improve how fast the server swaps to/from UDP -> TCP whenever the client is having issues sending UDP
The server will call `CryptSetup` with no data whenever the client has not responded to a message in a while, this would previously invalidate the players crypt setup and not let them Encrypt/Decrypt data at all

This also uses the local crypt state to determine of we should use UDP or TCP

This should fix one case of clients having to toggle off/on their voice chat to be able to hear/talk to other players
`HandleClientConnect` expects the `MumbleUser` to hae a valid username and session, but this was only sending the session id, so the audio sink was getting set to an empty user name, resulting in the client never being able to hear anyone until they turned off their voice chat and turned it back on (or got lucky enough that their internet was slow, or initialization was slow and that this wouldn't get hit).
@AvarianKnight
Copy link
Contributor Author

Nice to see some improvements, feel correct for most part, and i think having a check on the udp packet like mumble does will indeed improve some connection issue that were not detected before.

If you want to try other implementations, there is this one https://github.com/SOZ-Faut-etre-Sub/ZUMBLE that mimic fivem server mumble implementation, (we use it at lot for some years now without any major issue, don't have the ability to compile the fivem client but since we have user on expiremental build we will be able to give you feedback once it's merged)

Most of my testing was done on my fork of your project, although I did test to make sure the internal uMurmur didn't break because of any of these changes.

@IllInuz
Copy link

IllInuz commented Dec 10, 2024

Looks all good from what i can tell, most of it has been addressed above.
Thanks @AvarianKnight

@prikolium-cfx prikolium-cfx added ready-to-merge This PR is enqueued for merging feature-branch PR will be merged to feature branch first and removed triage Needs a preliminary assessment to determine the urgency and required action labels Dec 17, 2024
@prikolium-cfx prikolium-cfx merged commit 8b97411 into citizenfx:master Dec 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-branch PR will be merged to feature branch first ready-to-merge This PR is enqueued for merging RedM Issues/PRs related to RedM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants