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 · 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.

Buggem · 2025-01-10T23:48:47Z

@flibitijibibo @Daaaav Can we please get this pushed soon? I don’t want this PR to go stale like it has for the past 9 days. I just want at least some activity (playtesting, etc)

flibitijibibo · 2025-01-11T00:13:02Z

I've been stuck on SDL3 for a while (RC1 coming soon!) so I've not been able to check this out - upstream doesn't move very fast so even if this takes a while to review it shouldn't have to get rebased or anything like that since I can't really merge other stuff either.

mothbeanie · 2025-01-11T00:40:34Z

@flibitijibibo @Daaaav Can we please get this pushed soon? I don’t want this PR to go stale like it has for the past 9 days. I just want at least some activity (playtesting, etc)

Not to be rude, but you need to chill out... This is an entirely nonessential feature which has required a lot of intervention from maintainers and those more familiar with the codebase to fix bugs and and follow guidelines. 9 days is not a long time at all - being this impatient is really immature

flibitijibibo

The new Network backend is pretty good, but how its features are integrated could be less invasive.

Separately we should implement Steam rich presence, which thankfully is a one-liner: https://partner.steamgames.com/doc/features/enhancedrichpresence

A good reason to do this is to prepare localized strings:

https://partner.steamgames.com/doc/api/ISteamFriends#richpresencelocalization

If this is too much to do in one patch then we could split this up into two parts, where one part is rich presence and the second part is Discord integration. (Discord could be done first FWIW, it just wouldn't be fancy integration at first.)

flibitijibibo · 2025-01-11T00:31:43Z

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!


We'll need to lock this in before we can merge, so whoever can find this number should patch this line ASAP

desktop_version/src/DiscordNetwork.c

flibitijibibo · 2025-01-11T00:34:02Z

desktop_version/src/DiscordNetwork.c

+    }
+    if (!DISCORD_REQUIRE(app.core->run_callbacks(app.core)))
+    {
+        // Something  or other is wrong, but do we care?


Probably not, but Discord support might...? Not sure what their policy is on reporting internal errors

desktop_version/src/Network.c

desktop_version/src/main.cpp

Daaaav · 2025-01-11T01:23:42Z

before we can merge

(By the way, we haven't playtested yet, so please also wait with merging until we've had the chance to do that.)

(We have actually attempted to start playtesting but got stopped by some UB on Windows. So we're still working on it)

Buggem · 2025-01-13T09:47:57Z

@flibitijibibo I would be happy to expand this PR to include the Steam network RPC (you may have noticed my comment in the code lol), as the way you’ve got it set up makes it really easy. And also make it less invasive (I’m thinking run NETWORK_setRPC only when RPC is actually updated, running NETWORK_update each frame separately) However as of now I am on holidays and won’t get back to my computers for the next week or so.

Feel free to suggest changes while I’m out, all of them will be greatly thanked and considered.

Buggem · 2025-01-25T01:41:54Z

(We have actually attempted to start playtesting but got stopped by some UB on Windows. So we're still working on it)

I'm back and WHAT?? Undefined behavior? It's working for me...

Buggem · 2025-01-25T01:44:49Z

@flibitijibibo

"About that Client ID I owe ya..."

Ask Terry to do it or maybe you can do it yourself. I got a response but they said "there is a ID for VVVVVV, but we can only provide to people on the dev team". You have access to the Steamworks profile for VVVVVV, right (I saw you posting announcements) so maybe you can do it.

Feel free to do it once you've finished the SDL3 pr, we can keep the placeholder for the time being

Buggem · 2025-01-25T01:50:17Z

@mothbeanie My past PR's to other very niche repos have been merged in less than a day without pinging people... so... ¯_(ツ)_/¯

Daaaav · 2025-01-25T02:06:46Z

I'm back and WHAT?? Undefined behavior? It's working for me...

Yes, something about incorrect calling convention that only affects Windows.

My past PR's to other very niche repos have been merged in less than a day without pinging people... so... ¯_(ツ)_/¯

I have a feeling those past PRs might've been very small changes that were easy to instantly see they were correct?

For this, we need to set aside parts of our free time to test thoroughly. And we have more things to do. This isn't critical.

Thanks Flibit for the idea! :D

Co-authored-by: Ethan Lee <[email protected]>

Buggem · 2025-01-25T03:47:32Z

For this, we need to set aside parts of our free time to test thoroughly. And we have more things to do. This isn't critical.

Yep I understand.

flibitijibibo

It's definitely getting better - here's another quick review.

flibitijibibo · 2025-01-25T04:10:33Z

desktop_version/src/Network.c

    #if NUM_BACKENDS > 0
    int32_t i;
    for (i = 0; i < NUM_BACKENDS; i += 1)
    if (backends[i].IsInit)
    {
-        backends[i].Update();
+        if ( backends[i].Update() )


We don't need to worry about Update status too much - as long as we do a good job with SetRPC this can stay as-is.

desktop_version/src/main.cpp

Co-authored-by: Ethan Lee <[email protected]>

Buggem · 2025-01-26T01:20:14Z

Hmm something you commited made it not work... I'm looking through the code right now but it all looks good... debugging.

Oh yeah and by "make it not work" I mean level changes dont quite register as they should

Buggem · 2025-01-26T01:37:36Z

OH YES I THINK I GOT IT! It's the POINTERS!!!!!!!!!!!!!!!

oh shit fixing it is insane difficulty

Buggem · 2025-01-26T02:19:33Z

Fixed the pointers... but the fix SUCKS!

It just makes it slower when your original changes were to make it faster. Or maybe it doesnt make it slower idk

leo60228 · 2025-02-05T18:19:06Z

I've implemented Steam rich presence here: leo60228@418ff07

This expects localization strings added in Steamworks along these lines:

"lang"
{
	"english"
	{
		"tokens"
		{
			"#Default"	"%area%"
			"#NamedRoom"	"%area%: %name%"
		}
	}
}

Ideally, this would support localized room/area names, but this would significantly complicate both implementation and deployment.

leo60228

This mostly looks good, though I found a number of minor nitpicks.

leo60228 · 2025-02-05T18:21:11Z

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!


The automatically assigned application ID is 491427544134975498, but this doesn't currently work for rich presence. I'm not sure if the Discord Store release had a separate application ID.

leo60228 · 2025-02-05T18:22:01Z

desktop_version/src/DiscordNetwork.c

+
+int32_t DISCORD_init(void)
+{
+#if defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__HAIKU__) || defined(__DragonFly__)


This should probably be rewritten to check for OSes that Discord does support, rather than ones it doesn't.

leo60228 · 2025-02-05T18:23:35Z

desktop_version/src/DiscordNetwork.c

+
+        app.activityMan->update_activity(app.activityMan, &activity, NULL, NULL);
+    }
+


Newlines in this file seem generally inconsistent. This should probably be cleaned up.

leo60228 · 2025-02-05T18:26:37Z

desktop_version/src/main.cpp

+    }
+    else
+    {
+         char nextArea[RPC_SAFE_BUFFER] = "", nextRoom[RPC_SAFE_BUFFER] = "";


This line is indented incorrectly.

leo60228 · 2025-02-05T18:27:10Z

desktop_version/src/main.cpp

+        // Dirty fix for custom levels getting the area from Dimension VVVVVV
+        if (map.custommode)
+        {
+            SDL_strlcpy(nextArea, game.customleveltitle.c_str(), RPC_SAFE_BUFFER-1);
+        }
+        else
+        {
+            SDL_strlcpy(nextArea, map.currentarea(game.roomx, game.roomy), RPC_SAFE_BUFFER-1);
+        }
+        SDL_strlcpy(nextRoom, map.roomname, RPC_SAFE_BUFFER-1);


This should only run in GAMEMODE.

leo60228 · 2025-02-05T18:27:24Z

desktop_version/src/main.cpp

    // Update network per frame.
    NETWORK_update();

+


This line seems to have been added by accident.

leo60228 · 2025-02-05T18:27:32Z

desktop_version/src/main.cpp

+        //printf("room %s == %s, area %s == %s\n", rpcRoomname, nextRoom, rpcArea, nextArea);
+        if ((SDL_strcmp(rpcArea, nextArea) != 0) || (SDL_strcmp(rpcRoomname, nextRoom) != 0))
+        {
+            // FIXME BEFORE PUSHING!!: These pointers aren't safe to use and result in RPC not updating when it needs to.


Does this still apply?

no i fixed it, i just forgot to remove it

Add Discord Network support.

Loading
Loading status checks…

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.

Loading
Loading status checks…

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

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

Buggem added 2 commits January 1, 2025 10:24

Merge branch 'TerryCavanagh:master' into discordrpc

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

731f2ce

Add more names for different areas of the game IG

Loading
Loading status checks…

6e5fed0

flibitijibibo requested changes Jan 11, 2025

View reviewed changes

Make the RPC less invasive, only change when we NEED to.

bf9dd78

Thanks Flibit for the idea! :D

Buggem force-pushed the discordrpc branch from 99bcba4 to bf9dd78 Compare January 25, 2025 03:34

Strip unused stdio.h headerfile [redo]

fba8f7d

Co-authored-by: Ethan Lee <[email protected]>

flibitijibibo requested changes Jan 25, 2025

View reviewed changes

Commit flibit's changes **needs polish**

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

28a38bd

Co-authored-by: Ethan Lee <[email protected]>

Buggem added 2 commits January 26, 2025 12:52

Polish Flibit's changes to work. (PLS FIX POINTERS D:)

ec132ac

Patch out pointer control (new method sucks but ok I guess)

62ecc83

small commit: type* varname convention

4a12fae

leo60228 suggested changes Feb 5, 2025

View reviewed changes

Merge branch 'TerryCavanagh:master' into discordrpc

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

6bc5649

		#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!


		app.activityMan->update_activity(app.activityMan, &activity, NULL, NULL);
		}

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 Jan 1, 2025

Daaaav commented Jan 1, 2025

Buggem commented Jan 1, 2025 • edited Loading

Buggem commented Jan 10, 2025

flibitijibibo commented Jan 11, 2025

mothbeanie commented Jan 11, 2025

flibitijibibo 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 commented Jan 11, 2025

Buggem commented Jan 13, 2025

Buggem commented Jan 25, 2025

Buggem commented Jan 25, 2025 • edited Loading

Buggem commented Jan 25, 2025

Daaaav commented Jan 25, 2025

Buggem commented Jan 25, 2025

flibitijibibo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Buggem commented Jan 26, 2025 • edited Loading

Buggem commented Jan 26, 2025 • edited Loading

Buggem commented Jan 26, 2025 • edited Loading

leo60228 commented Feb 5, 2025

leo60228 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

leo60228 Feb 5, 2025 • 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

Buggem commented Dec 10, 2024 •

edited

Loading

Buggem commented Dec 13, 2024 •

edited

Loading

Buggem commented Jan 1, 2025 •

edited

Loading

Buggem commented Jan 25, 2025 •

edited

Loading

Buggem commented Jan 26, 2025 •

edited

Loading

Buggem commented Jan 26, 2025 •

edited

Loading

Buggem commented Jan 26, 2025 •

edited

Loading

leo60228 Feb 5, 2025 •

edited

Loading