Add Discord Game SDK support #1217

Buggem · 2024-12-10T06:21:50Z

Changes:

Adds a Discord RPC implementation

Finished!

I would love to hear your thoughts on this change, and any improvements or suggestions are more than welcome.

Legal Stuff:

By submitting this pull request, I confirm that...

My changes may be used in a future commercial release of VVVVVV
I will be credited in a CONTRIBUTORS file and the "GitHub Friends"
section of the credits for all of said releases, but will NOT be compensated
for these changes unless there is a prior written agreement

flibitijibibo · 2024-12-10T13:23:35Z

Looking it over I'm surprised you can't SDL_LoadObject and then load DiscordCreate... seems like they do everything right, it's versioned properly and the rest of the API is a function table to avoid needing more exports. Do they mention any specific reason that dlopening doesn't work?

Aside from that this looks okay; we might be able to avoid a lot of the build system goo if we can dynamically load like the other network backends.

Buggem · 2024-12-11T05:41:38Z

Looking it over I'm surprised you can't SDL_LoadObject and then load DiscordCreate... seems like they do everything right, it's versioned properly and the rest of the API is a function table to avoid needing more exports. Do they mention any specific reason that dlopening doesn't work?

Aside from that this looks okay; we might be able to avoid a lot of the build system goo if we can dynamically load like the other network backends.

I'm a real newbie to C/C++ and honestly do not understand how to dynamically load in the way that you do with the function tables. Also, adding libdiscord_game_sdk.so to the linker options does not actually merge it into the VVVVVV executable, it just fufills the headerfiles needs for the function calls that the SDK uses.

flibitijibibo · 2024-12-11T12:02:12Z

This is definitely worth checking out:

https://wiki.libsdl.org/SDL2/SDL_LoadObject

The idea is that instead of linking the SDK, we just use the headers to get the type declarations, and then store a pointer to the DiscordCreate function. We initialize the pointer by dynamically loading the SDK, then loading the function by its name. You can see this in the other network files as an example.

Buggem · 2024-12-12T05:55:34Z

This is definitely worth checking out:

https://wiki.libsdl.org/SDL2/SDL_LoadObject

The idea is that instead of linking the SDK, we just use the headers to get the type declarations, and then store a pointer to the DiscordCreate function. We initialize the pointer by dynamically loading the SDK, then loading the function by its name. You can see this in the other network files as an example.

Ahh that is interesting. So you are not talking about removing the headerfile completely, only keeping the structs. Nice, I can definitely do that.

…ake issues.

Buggem · 2024-12-12T07:01:56Z

@flibitijibibo I've done your requests and the PR is almost ready! I just need to fix some potential crashes!
E.g. one potential crash is closing or restarting Discord while VVVVVV is open will lose the context of app.core (in short, it causes a segfault), but that will be patched soon. I also need to remove the DISCORD_shutdown stub and replace it with ClearPointers, and other function calls. After a few patches, it will be ready for 2.5 :)

Buggem · 2024-12-12T07:31:21Z

Very strange finding...
With the warning this build added, when I removed it, triggered this out of the blue SDL2 error. Help?

                 from /usr/include/SDL2/SDL_config.h:58,
                 from /usr/include/SDL2/SDL_stdinc.h:31,
                 from /usr/include/SDL2/SDL_main.h:25,
                 from /usr/include/SDL2/SDL.h:32,
                 from /home/max/Downloads/VVVVVV/desktop_version/src/DiscordNetwork.c:17:
/usr/include/SDL2/SDL_platform.h:259:1: error: multiple storage classes in declaration specifiers
  259 | extern DECLSPEC const char * SDLCALL SDL_GetPlatform (void);
      | ^~~~~~
/usr/include/SDL2/SDL_platform.h:259:38: warning: ‘SDL_GetPlatform’ declared ‘static’ but never defined [-Wunused-function]
  259 | extern DECLSPEC const char * SDLCALL SDL_GetPlatform (void);
      |                                      ^~~~~~~~~~~~~~~
make[2]: *** [CMakeFiles/VVVVVV.dir/build.make:846: CMakeFiles/VVVVVV.dir/src/DiscordNetwork.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:100: CMakeFiles/VVVVVV.dir/all] Error 2
make: *** [Makefile:91: all] Error 2
cp: cannot stat './VVVVVV': No such file or directory

Buggem · 2024-12-12T07:34:10Z

Huh. Moving the sdl2 header up worked. Huh..... :/

Buggem · 2024-12-12T07:40:55Z

Ready for review 👍

Daaaav · 2024-12-12T15:56:16Z

I'm wondering how this should work with localization.

Ideally, every Discord user who sees the status messages sees them in their own chosen Discord language, not whatever language the player (the sender) happens to use. But unfortunately Discord does not support that at all - the game can only pass one string, and that string is shown worldwide. So I'm not sure that string should be in the player's language... For example, I may prefer to have my game and my Discord in Dutch, but pretty much all my online friends are from other countries and I talk to them in English.

We've always made the localization features such that players should never have to feel forced to switch their game to English because of some kind of disadvantage, or accidentally reveal their in-game language to others when they wanted to keep it anonymous where they come from.

So... I guess the lesser evil would be to force Rich Presence in English for now and call it a Discord L, and see if they add proper multi-language support later. Or we could maybe add a game option whether to translate Rich Presence status messages or not (we already have a lot of options, though...)

Daaaav

Just did some light code quality scanning, no in-depth review yet.

desktop_version/src/DiscordNetwork.c

.vscode/settings.json

Buggem · 2024-12-12T20:59:24Z

So... I guess the lesser evil would be to force Rich Presence in English for now and call it a Discord L, and see if they add proper multi-language support later. Or we could maybe add a game option whether to translate Rich Presence status messages or not (we already have a lot of options, though...)

Yes, I could do that very easily.

Co-authored-by: Dav999 <[email protected]>

Buggem · 2024-12-13T03:03:07Z

This PR is no longer ready. On Linux, if Discord is not installed...
(wrong screenshot for first edit sorry)

This would break Steam Deck compatibility.

Buggem · 2024-12-13T03:18:36Z

Oops! Minor mistake, only happened because I had an old build of my pull request installed onto steam. It's totally fine.

Buggem · 2024-12-13T03:20:50Z

Oops (again) when running the build without discord, BOOM! A segfault. Patch incoming, lemme debug with GDB first.

Buggem · 2024-12-13T03:31:37Z

Well, I patched it. I think it is finally ready for a merge. Merge when you want to, for now feel free to review the code. I will play my version of VVVVVV and look for potential segfaults for now. Bye!

…rd would see it in the player's language.

Buggem · 2024-12-14T21:47:43Z

@Daaaav I have completely finished your requests. (I forgot to remove the localisation but now that's done)

Daaaav

Did a slightly more thorough review of the code - and of course we're still gonna need to test this.

desktop_version/src/DiscordNetwork.c

Daaaav · 2024-12-14T22:58:18Z

desktop_version/src/DiscordNetwork.c

+    discordCrashes = 0;
+}
+
+void DISCORD_update(const char *level, const char *name)


I'd probably rename level and name to something like area and roomname, to make them clearer. Also, code style thing: we put the * with the type, not the variable name. (I see it's that way in the existing Steam code but we've been consistent in all new code)

@Daaaav GOGNetwork does not have the type* naming convention for functions I didn't implement, so I added that too. Should I just keep to my Discord implementation? Or would this fix be acceptable as part of this PR?

I'd say it'd be acceptable to move spaces in the GOG/Steam code so that you don't have to add new code that either introduces type * convention or is inconsistent with the existing code. For more unrelated parts (functions you didn't implement for Discord but which were in the same GOG/Steam files you were already making consistency fixes in... Eh. Might as well.

desktop_version/src/Network.c

third_party/libdiscord_game_sdk.so

- remove more useless headerfiles - rename [level, name] to [area, roomname] - remove 10 repeats fallback - do not update_activity if it is the same value as last DISCORD_update - fix bug with DiscordCreate failure handling - rename (and invert) discordNotDetected to discordDetected

Buggem · 2024-12-15T08:48:46Z

@Daaaav Completed your requests again :)

Daaaav · 2024-12-17T04:36:01Z

desktop_version/src/DiscordNetwork.c

+    if(activity.state == roomname || activity.assets.large_text == area)
+    {
+        DISCORD_REQUIRE(app.core->run_callbacks(app.core));
+        return;
+    }
+    SDL_strlcpy(activity.state, roomname, sizeof(activity.state));
+    SDL_strlcpy(activity.assets.large_image, "vvvvvv", sizeof(activity.assets.large_image));
+    SDL_strlcpy(activity.assets.large_text, area, sizeof(activity.assets.large_text));
+
+    app.activityMan->update_activity(app.activityMan, &activity, NULL, NULL);
+
+    DISCORD_REQUIRE(app.core->run_callbacks(app.core));


I think how the update function should work is something like this:

if (app.core->run_callbacks(app.core) != DiscordResult_Ok) { // No Discord return; } // It was Ok, so we can now do things with Discord if (activity is outdated) { update activity; }

Also, with C strings, if you want to check that two strings are equal, you need to do SDL_strcmp(a, b) == 0 instead of a == b. activity.state == roomname and activity.assets.large_text == area will check that the two strings are stored in exactly the same bit of memory, rather than the strings having the same text.

@Daaaav I did consider this, but you are meant to run the run_callbacks function after the activity update (source as per Discord C example)

I don't think it's necessarily that you need to run run_callbacks after the activity update; the example code just wanted to demonstrate updating the activity only one time and then run run_callbacks in a main loop, as a simplified example. And even if run_callbacks is the thing that "confirms" an activity update... well, then it's only going to be delayed by one frame.

The main advantage of switching them around is that we don't have to bother constantly updating the activity and telling Discord about it, if we know Discord isn't ready to take it anyway.

Alright, in my opinion 1 frame delay is way too much (lol average gamer) but I can switch it around if you really want, I'll leave it as a single commit incase anyone wants to rollback.

Wait... I don't actually think we should do that. We need to handle for app.core == NULL not running a callback, as if app.core was to be NULL and we ran a function on it (what functions pointers are defined inside the specific app.core instance) then we would have a segfault. So instead of replacing error handling with that I'll just add it on.

The commit fucked something up - it no longer works. Reverting.

I put some debug logs in for different scenarios:

[ERROR] Discord API failed to initialise!

[ERROR] DiscordResult_Ok
[ERROR] Discord API failed to initialise!
even weirder... probably an easy fix

Take your time to get it fully working - you don't have to post a message every step along the way 😛

Fixed it, just incorrect handling of DISCORD_REQUIRE, I fixed that but introduced another variable. I have to do it unless of course you want discordDetected to be true by default, which would be weird.

Daaaav

Not really exhaustive (I did a scan and called it a day):

Little unsure about adding the whole DiscordResult_Strings array... Especially if it's not actively used... I guess we do uglier things, and it seems to be the only way to not have to say "Discord error 15" if we do want debug messages...
We tend to avoid commented-out code statements, like the //vlog_error()s. Maybe most of them would be good as vlog_debug() calls - vlog_debug is hidden by default so it's perfect for messages that might be a little too verbose. Console spam still isn't nice though lol, so no messages when run_callbacks can't find a running Discord instance every frame
Code style nits, part of which took me a bit to notice and part of which are new: we always have a space after if (so if (true) instead of if(true)), and braces on a new line (no if (true) {). We also never put comments on the same line as a { - in that case the comment gets its own line.

desktop_version/src/DiscordNetwork.c

Daaaav · 2024-12-19T03:39:15Z

desktop_version/src/DiscordNetwork.c

+#undef FOREACH_FUNC
+
+bool DISCORD_REQUIRE(int x) {
+    //vlog_error(DiscordResult_Strings[x]);


If Discord adds new error codes, this could segfault (if uncommented).

Yes... is there any way we could workaround this?

We have INBOUNDS_ARR(x, DiscordResult_Strings) which will return true if index x is in bounds.

Since DISCORD_REQUIRE is (at least in the current state) used for run_callbacks, I feel like maybe it should only print once if an error occurs. We have another macro for that: WHINE_ONCE/WHINE_ONCE_ARGS.

In all... This should work (definitely in the x != DiscordResult_Ok branch below):

const char* message = "Unknown"; if (INBOUNDS_ARR(x, DiscordResult_Strings)) { message = DiscordResult_Strings[x]; } WHINE_ONCE_ARGS(("Discord error: %s (%d)", message, x));

Edit: you will need to #include "UtilityClass.h"

@Daaaav I don't think we should bother with this as spitting out inaccurate error codes is just as worse as a segfault.

It's not really relevant anymore since the error printing code is now removed, but no.

One is a best-effort "Unknown (54)" in console output that almost no player will ever read (and it's not lying about what error it is), the other is the game crashing completely when it should've been playable.

desktop_version/src/DiscordNetwork.c

@Daaaav

@Daaaav - removed DiscordResult error logging feature - uncommented debug features - replace some vlog_error calls with vlog_debug to indicate only with verbosity - fix string equal or not equal handling (use SDL_strcmp) - remove tab @Buggem (some stuff I noticed) - handle Discord API shutdown correctly app.core->destroy - if libHandle exists to libHandle != null

Buggem · 2024-12-21T03:15:39Z

@Daaaav finished your req again. @flibitijibibo do you want to tag this?

Daaaav · 2024-12-21T03:48:19Z

Note that we haven't tested the code yet (I might do that within the next week but no promises). I also still see a few code style things (like if() instead of if () or {s not on their own line) but we're getting pretty close I think!

flibitijibibo · 2024-12-21T04:11:58Z

Yeah I'll try to read through everything next week - took a quick look and it's definitely coming together nicely!

flibitijibibo · 2024-12-21T04:12:21Z

desktop_version/src/DiscordNetwork.c

+#define DISCORD_CLIENT_ID 1315544357532729447
+
+// TO TERRY/FLIBIT: You can create your own Discord instance at the Discord Developer Portal. This ID belongs to me, so just be aware that if my account was to get hacked, VVVVVV RPC would too. Use your own!


@TerryCavanagh, does this already exist...?

For reference, the Discord docs do say this:

If you're integrating our SDK into an already-released game, there's a good chance that we may already have an application in our database for your game! Reach out to our Dev Support to learn more

I wonder whether the reasons to use the ID they already have are good enough reasons to bother lol (maybe so it smoothly takes over the "Playing VVVVVV", "last played 2 days ago", etc that was already showing without rich presence?)

@Daaaav @flibitijibibo I have sent a message to Discord Support to see if this is true for VVVVVV. Hopefully I get a response before the 15th anniversary of VVVVVV. (I'm hoping this will be released then)

I'm sure an application already exists for VVVVVV, as it existed on the Discord Game Store back when that existed.

Buggem · 2024-12-22T01:15:53Z

Note that we haven't tested the code yet (I might do that within the next week but no promises). I also still see a few code style things (like if() instead of if () or {s not on their own line) but we're getting pretty close I think!

Well I tried to fix that, I'll use a find and replace tool to help I guess.

Buggem · 2024-12-31T23:22:37Z

@Daaaav @flibitijibibo Happy new year! VVVVVV's 25th anniversary is coming soon and I am REALLY hoping to get it pushed by then. I also have some fixes coming in soon as well so get ready for that! Cheers and to a good 2025, Max Parry.

Buggem · 2025-01-01T00:30:27Z

One of the nice things about this PR is that if anyone wants to implement another type of RPC (eg Steam RPC) they already got a baseplate set up for them :D

Daaaav · 2025-01-01T00:40:59Z

Happy new year! VVVVVV's 25th anniversary is coming soon and I am REALLY hoping to get it pushed by then.

Happy new year!

To be clear, to my knowledge it's not planned for 2.5 to be released within the coming month. Unless you meant pushed to the repo and not pushed to store shelves, that one is realistic...

Buggem · 2025-01-01T01:35:35Z

To be clear, to my knowledge it's not planned for 2.5 to be released within the coming month. Unless you meant pushed to the repo and not pushed to store shelves, that one is realistic...

Alright let's just push to the repo. I would have though something would happen with VVVVVV's 15th anniversary but if not, it's totally fine.

Add Discord Network support.

eb21260

Buggem marked this pull request as draft December 10, 2024 06:22

Dynamically load Discord Game SDK with SDL_LoadObject() to avoid CM…

0a1244b

…ake issues.

Patch potential crash scenarios and remove warnings.

7975b27

Buggem marked this pull request as ready for review December 12, 2024 07:40

Daaaav suggested changes Dec 12, 2024

View reviewed changes

Buggem and others added 3 commits December 13, 2024 08:03

Replace strncpy with SDL_strncpy

76d5738

Co-authored-by: Dav999 <[email protected]>

Remove unneeded headerfiles, .vscode folder, and move comment

76d4368

Make Discord not required to exist - thanks @Daaaav

ce111cf

Avoid segfault if Discord is not detected, remove useless headerfiles

7d3e7db

Remove localisation features from the RPC, as people on English Disco…

17fea40

…rd would see it in the player's language.

Daaaav suggested changes Dec 14, 2024

View reviewed changes

Buggem added 3 commits December 15, 2024 19:03

Remove placeholder

459437c

Fix bad implementation of inversion of discordDetected

5cbf03d

finish off naming convention = type*

132c504

Daaaav reviewed Dec 17, 2024

View reviewed changes

Finish @Daaaav's requests :D

29708db

Daaaav suggested changes Dec 19, 2024

View reviewed changes

Buggem added 2 commits December 21, 2024 14:09

consistancy

bef03b6

maybe remove console spam (seperate commit to consider reverts)

d35fb29

flibitijibibo reviewed Dec 21, 2024

View reviewed changes

Finish formatting

9c05ffe

flibitijibibo added the targets 2.5 label Dec 22, 2024

Buggem added 2 commits January 1, 2025 10:24

Merge branch 'TerryCavanagh:master' into discordrpc

731f2ce

Add more names for different areas of the game IG

6e5fed0

		#define DISCORD_CLIENT_ID 1315544357532729447

		// TO TERRY/FLIBIT: You can create your own Discord instance at the Discord Developer Portal. This ID belongs to me, so just be aware that if my account was to get hacked, VVVVVV RPC would too. Use your own!

Add Discord Game SDK support #1217

Are you sure you want to change the base?

Add Discord Game SDK support #1217

Conversation

Buggem commented Dec 10, 2024 • edited Loading

Changes:

Legal Stuff:

flibitijibibo commented Dec 10, 2024

Buggem commented Dec 11, 2024

flibitijibibo commented Dec 11, 2024

Buggem commented Dec 12, 2024

Buggem commented Dec 12, 2024

Buggem commented Dec 12, 2024

Buggem commented Dec 12, 2024

Buggem commented Dec 12, 2024

Daaaav commented Dec 12, 2024

Daaaav left a comment

Choose a reason for hiding this comment

Buggem commented Dec 12, 2024

Buggem commented Dec 13, 2024 • edited Loading

Buggem commented Dec 13, 2024

Buggem commented Dec 13, 2024

Buggem commented Dec 13, 2024

Buggem commented Dec 14, 2024

Daaaav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Buggem commented Dec 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Buggem Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daaaav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daaaav Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Buggem commented Dec 21, 2024

Daaaav commented Dec 21, 2024

flibitijibibo commented Dec 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Buggem commented Dec 22, 2024

Buggem commented Dec 31, 2024

Buggem commented Jan 1, 2025

Daaaav commented Jan 1, 2025

Buggem commented Jan 1, 2025 • edited Loading

Buggem commented Dec 10, 2024 •

edited

Loading

Buggem commented Dec 13, 2024 •

edited

Loading

Buggem Dec 18, 2024 •

edited

Loading

Daaaav Dec 21, 2024 •

edited

Loading

Buggem commented Jan 1, 2025 •

edited

Loading