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 tweaks/fixes #3058

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AvarianKnight
Copy link
Contributor

@AvarianKnight AvarianKnight commented Jan 8, 2025

Goal of this PR

Fix some possible edge cases where players could be invalid but we would still send a properly formatted mumble name

Removes swapping between std::wstring and std::string

Reset the mumble connection in cases where the client loses their TCP connection.

Fix some cases where the client would send duplicates of the same targets to the server, leading to some excessively large packets.

Wrap any native that needs to be used while connected in MakeMumbleNative

This change set is currently untested

This PR applies to the following area(s)

FiveM, RedM

Successfully tested on

Game builds: ..

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.

@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Jan 8, 2025
@AvarianKnight AvarianKnight force-pushed the tweak/mumble-tweaks branch 3 times, most recently from 1c1dbe5 to 55cb212 Compare January 29, 2025 03:51
@AvarianKnight AvarianKnight marked this pull request as ready for review January 29, 2025 03:54
@github-actions github-actions bot added the RedM Issues/PRs related to RedM label Jan 29, 2025
@github-actions github-actions bot added invalid Requires changes before it's considered valid and can be (re)triaged and removed triage Needs a preliminary assessment to determine the urgency and required action labels Jan 29, 2025
@AvarianKnight
Copy link
Contributor Author

This should be ready to be reviewed now, newer changes remove conversions between string types and resets the players mumble connection if the server doesn't respond to TCP pings.

@AvarianKnight AvarianKnight changed the title Cleanup voice targetting bits of mumble to not have duplicates Mumble tweaks/fixes Feb 1, 2025
@AvarianKnight AvarianKnight force-pushed the tweak/mumble-tweaks branch 2 times, most recently from ebfb2f4 to 33651e4 Compare February 5, 2025 19:34
Copy link
Contributor

@Nobelium-cfx Nobelium-cfx left a comment

Choose a reason for hiding this comment

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

Looks like a really nice refactoring. Thank you!

Overall LGTM, just left some style related comments and asked some clarifying questions. PTAL.

It is a big change tho, so before merging we would probably need to tests it on some server.

code/components/gta-net-five/src/MumbleVoice.cpp Outdated Show resolved Hide resolved
return g_mumbleClient->GetAudioContext(name);
}

std::wstring getMumbleName(int playerId)
std::optional<std::string> getMumbleName(int playerId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this seems to be very similar to getAudioContext. Can we use getMumbleName in getAudioContext to avoid duplication?

Copy link
Contributor Author

@AvarianKnight AvarianKnight Feb 10, 2025

Choose a reason for hiding this comment

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

iirc this was actually needed, getAudioContext doesn't check for if they're connected before hand, while everywhere that uses getMumbleName does, we can just do the connected check again if wanted though.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that it seems that we could refactor getAudioContext as below to reduce some duplication.

static std::shared_ptr<lab::AudioContext> getAudioContext(int playerId)
{
        auto name = getMumbleName(playerId);
	if (name  == nullptr || !IsMumbleConnected())
	{
		return {};
	}
	return g_mumbleClient->GetAudioContext(name);
}

code/components/gta-net-five/src/MumbleVoice.cpp Outdated Show resolved Hide resolved
code/components/voip-mumble/include/MumbleClientImpl.h Outdated Show resolved Hide resolved
code/components/voip-mumble/src/MumbleClient.cpp Outdated Show resolved Hide resolved
code/components/voip-mumble/src/MumbleClient.cpp Outdated Show resolved Hide resolved
code/components/voip-mumble/src/MumbleClient.cpp Outdated Show resolved Hide resolved
@AvarianKnight AvarianKnight force-pushed the tweak/mumble-tweaks branch 2 times, most recently from 442d156 to aefe578 Compare February 10, 2025 16:24
@github-actions github-actions bot added triage Needs a preliminary assessment to determine the urgency and required action and removed invalid Requires changes before it's considered valid and can be (re)triaged labels Feb 10, 2025
…eTarget packet

The previous implementation allowed duplicates, and was more complex than what we need since we don't care about channel linking (we would always set it to false)

This will also reset connections if they have too many in flight TCP pings

This removes all the swaps from std::wstring <-> std::string

Send Auth packet immediately after Version

In the spec we have to send the Auth packet after Version, we opted to
do it whenever we get the version packet back, this isn't really needed
and slows down the exchange, since the server will only send `CryptoSetup`
after we send this packet.
@github-actions github-actions bot added invalid Requires changes before it's considered valid and can be (re)triaged and removed triage Needs a preliminary assessment to determine the urgency and required action labels Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Requires changes before it's considered valid and can be (re)triaged RedM Issues/PRs related to RedM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants