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

Assorted Steam presence fixes #2324

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

blattersturm
Copy link
Contributor

This contains a fix for my test server's non-ASCII host name being broken in Steam presence, and some assorted fixes for issues caught along the way (steamname.txt returning mojibake, and leftover SteamChild processes lingering).

These changes may not be as meaningful if/when FiveM and RedM are assigned an actual AppID and can use the native rich presence functionality, but for the time being they solve stuff being broken randomly.

Goal of this PR

Fix some issues that showed up regarding Steam presence.

(.. also, the 'consice' typo is still there in the template)

How is this PR achieving the goal

See commit messages for details.

This PR applies to the following area(s)

Client (both FiveM and RedM), steam component. Also updates a vendor library, this wasn't tested on the Linux build as I do not have a Linux system available at this time - this should however work.

Successfully tested on

Game builds: FiveM

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. (why is this not an automated check?)

The 'steamname.txt' feature, which allows overriding the product name
shown in Steam rich presence, would originally read files as CP_ACP due
to a mistaken use of 'wstringstream'/'wifstream'.

This means that trying to replicate the existing product name (such as
'FiveM 🐌') would show mojibake instead. Since we can assume people will
save files as UTF-8 (even Windows Notepad has this as a default), just
not converting this should be safe.
In some cases, needRefresh would be set to true before the existing
child process would've picked up on it and exited. Instead, we use a PID
match to make sure only the latest process remains.
The Steam work needed utf8::find_invalid(const std::string&), which was
introduced in v3.0.0.
With a host name such as "Êîðïîðàöèÿ Ìàéêðîñîôò; Âñå ïðàâà çàùèùåíû", we
will try to set a presence name like "test: Êîðïîðàöèÿ Ìàéêðîñîôò; Âñå
ïðàâà çàùèùåíû", assuming `steamname.txt` contains "test".

Steam, internally, truncates app names to fit in a 64-byte buffer with a
null terminator. However, the code which truncates does not ensure the
resulting UTF-8 sequence is valid, which might lead to the update being
rejected further down the line, which is especially likely with a name
consisting solely of multi-byte sequences, like the example name.

In this changeset, we circumvent this by truncating the name to 64 bytes
ourselves, and subsequently chopping off at the first invalid UTF-8
sequence.
@blattersturm
Copy link
Contributor Author

Coincidentally, some user posted about an issue similar to this on the forums: https://forum.cfx.re/t/steam-rich-presence-doesnt-work-specifically-on-my-server/5199043

@blattersturm
Copy link
Contributor Author

An additional thought as a result of the topic above applies regarding whether presence - both Steam and Discord - should try to show the project name instead of the raw/legacy hostname, where available.

The user's host name was ^5[Israel] ^0Gamers-Israel Serious RP ^0| ^5V4 ^0| ^1Mumble 🎤 ^0| ^4Custom Cars 🚗 ^0| ^2 30,000💸 ^0| https://discord.gg/fivemil ^0| ^5Gamers-Israel.co.il, whereas the project name was ^5Gamers-Israel RolePlay, which would make more sense to display in rich presence.

@nihonium-cfx nihonium-cfx added the needs manual verification PRs that need manual verification by testing the change locally label Jan 11, 2024
@blattersturm
Copy link
Contributor Author

blattersturm commented Jan 11, 2024

What's this 'needs manual verification' state? I've looked for process details (as said in the October 2023 Community Pulse, We also defined a new process for us to review community contributions to public repositories, as well as new contribution guidelines that will be added to our public repositories and documentation website soon.) that might define this so a contributor can know what exactly it means, but I couldn't find any details on this process that was supposed to be published 'soon'.

I've also not seen this on any previous PR, but I assume it's 'someone will have to review this, but not me' - why not assign a reviewer, then?

@nihonium-cfx
Copy link
Contributor

It was created to mark PRs that'd need to be manually verified (builds, runs, etc.) by the team and keep track of such PRs. In this case, it is to verify linux build is fine.

@nihonium-cfx nihonium-cfx self-requested a review January 31, 2024 09:59
@nihonium-cfx nihonium-cfx removed the needs manual verification PRs that need manual verification by testing the change locally label Jan 31, 2024
@nihonium-cfx nihonium-cfx added the ready-to-merge This PR is enqueued for merging label Jan 31, 2024
@nihonium-cfx nihonium-cfx merged commit 88a0a6c into citizenfx:master Feb 1, 2024
1 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR is enqueued for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants