-
-
Couldn't load subscription status.
- Fork 496
Add initial Discord Game SDK Integration #1031
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
Conversation
Client/core/CDiscordManager.cpp
Outdated
| std::lock_guard<std::mutex> guardian(m_threadSafety); | ||
| m_storedActivity.SetDetails("Retrieving server name..."); | ||
| m_storedActivity.GetAssets().SetSmallText(SString("Connected to %s:%i", *ipaddr, port)); | ||
| m_storedActivity.GetAssets().SetSmallImage("a-server"); // TODO: Maybe contact with MTA:SA servers and check if this ip is a premium one containing a small image to set using this function |
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.
Maybe from https://mtasa.com/toplist/ ?
Discord have a limit of 150 images that you can upload.
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.
Thank you very much for the pull request! Overall, it looks very good.
I commented on a few minor issues, but haven't fully reviewed it yet.
| CLuaArguments Arguments; | ||
| Arguments.PushBoolean(true); | ||
| Arguments.PushString(joinSecret); | ||
| Player.CallEvent("onPlayerDiscordJoin", Arguments); |
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.
I'd maybe generalize this a bit to something like onPlayerMessengerJoin to be not too tightly coupled to Discord.
The signature may be something like: string messengerType, bool justConnected, string secret.
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.
Perhaps the third argument could be a table that can change depending on the "messenger"? Is "messenger" correct terminology here? Not sure.
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.
onPlayerExternalJoin maybe?
Client/core/CDiscordManager.cpp
Outdated
|
|
||
| CDiscordManager::~CDiscordManager() | ||
| { | ||
| m_bSuicide = true; |
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.
This look very hacky. Not sure how to do this more nicely though
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.
The only other way is signals I guess
|
So as you guys stopped reviewing this, I'll adjust the necessary modifications to the PR. Edit: There you have it. |
|
Seems to work well, with a few minor catches! Doesn't seem to compile on Windows without modification (i.e. copying a lib file from https://dl-game-sdk.discordapp.net/latest/discord_game_sdk.zip into Please can you fix this? Things that worked great
Something odd The discord activity message does not reappear after you enable rich presence, it only appears when you join a server. (Does not say "In Main Menu" when you disconnect from a server either!) |
Well, it will be enabled in 15 seconds or less, it will use the "reconnecting" feature for reappearing which will be done in every 15 sec. (Thought it's better to prevent user spamming enable/disable)
Are you sure? It works for me.
Why not. Also things that you may have missed:
|
Also allow it through .gitignore
…a-blue into discordintegration
Okay!
I meant after you turned the feature off and on again. So this doesn't matter anymore.
Yep, can confirm the first one works. Haven't tried backwards compat. |
|
156cdf9 should have done the job. Can you test it for once more? |
Also disallow to use party max than the max players value
Also do not download discord again when it is already up to date
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.
This looks ready to merge.
The Lua API will need changing but I'm happy for us to say something like this on the wiki:
This is function is experimental and you should first check that the function exists before using it. The function can be removed at any time.
if onPlayerDiscordJoin then onPlayerDiscordJoin(...) end
I'll give it a little bit of time for one of @multitheftauto/blue-collaborators to send a secondary approval, because it's many lines of code.
If someone merges this, please squash merge, a couple of large files have been deleted and shouldn't end up in master.
| if os.isfile(archive_path) and os.md5_file(archive_path) == DISCORD_MD5 then | ||
| print("Discord Game SDK is up to date.") | ||
| download_discord = false | ||
| return |
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.
It was like this before, but I found that if you delete the other files but keep the .zip, it won't re-add the other files.
I think CEF has the same issue, so I don't mind if both do.
|
repeating for loudness If someone merges this, please squash merge, a couple of large files have been deleted and shouldn't end up in
|
|
I am merging this in 1 week if nobody else reviews this |
|
@qaisjp The PR looks good. Let's go for the merge! |
As discussed in #721 I've implemented it.
There is a new library added to the mta path, called discord_game_sdk.dll
I think there should be client verification for this, I'm not sure.
Also it's best to request Discord for the official MTA:SA game so we can use spectate feature too.
My friend @Deihim007 suggested this in the first place.
Ali Gerami.