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.

Sign up for GitHub

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

Jump to bottom

Add Discord Rich Presence support #6983

Merged

delroth merged 8 commits into dolphin-emu:master from yourWaifu:add-discord-rpc-support

Jun 24, 2018

Contributor

yourWaifu commented May 27, 2018

I thought Discord's Rich Presence would be a nice feature to add to Dolphin.

A few things though.

I didn't know where to place the code for this, so I placed both DiscordPresence.h and DiscordPresence.cpp in Source/Core/Core. If this is an issue, it shouldn't be too difficult to move the two files somewhere else.
I also only have a computer running on Windows, so I havn't tested this on macOS or Linux nor do I know how to write code for them. I was wondering if I should place #ifdefs around the code because of this, but I didn't want to in the case that they did work on macOS.

yourWaifu changed the title ~~Add discord rpc support~~ Add Discord Rich Presence support

Member

RisingFog commented May 27, 2018

@dolphin-emu-bot rebuild

Member

lioncash commented May 27, 2018

Sorry but as this is, I can't agree with merging this feature to the codebase. This doesn't belong in the core library of Dolphin. This should be entirely driven by the frontends or put in UICommon.

I also do not think this is really worth adding yet-another-dependency or yet-another-external to Dolphin for (IMO).

Contributor Author

yourWaifu commented May 27, 2018

I can understand that. I'll figure how to move it over to UICommon or you can close it and I'll not argue with you about it.

Member

lioncash commented May 27, 2018

I'm sure other devs will have opinions on it, so UICommon would likely be better for now

Member

lioncash commented May 27, 2018 •

edited

Loading

Posting this for @delroth, another maintainer, who currently can't access GitHub, but does have access to IRC:

[03:02] <@delroth> +1 to the feature from me -- I've seen people requesting it and other emulators implement it. In general I think discord has won the majority for gaming related rtc, I don't mind us giving a preferential treatment to their software
[03:02] <@delroth> But haven't looked at the implementation at all
[03:05] <@delroth> Hmmm can't login to gh because I can't receive SMS here. :/
[03:05] <@delroth> +1 to UICommon
[03:07] <@delroth> One idea I was looking at was using the discord "join party" feature for netplay
[03:07] <@delroth> Basically using discord as a netplay lobby

and the join party thing was something I actually didn't think about, which does sound nice to have, so I've changed my opinion on the matter.

JosJuice reviewed

View reviewed changes

Source/Core/Core/DiscordPresence.cpp Outdated

+              void Init()
+              {
+                DiscordEventHandlers handlers = {0};
+                Discord_Initialize("450033159212630028", &handlers, 1, NULL);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

lioncash reviewed

View reviewed changes

Member

lioncash left a comment

Did a basic review on minor things that can be changed

Source/Core/Core/DiscordPresence.cpp Outdated

+              {
+              void Init()
+              {
+                DiscordEventHandlers handlers = {0};

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/DiscordPresence.cpp Outdated

+                discord_presence.largeImageKey = "dolphin_logo";
+                discord_presence.largeImageText = "Dolphin is an emulator for the GameCube and the Wii.";
+                discord_presence.details = details.empty() ? "Not in-game" : details.c_str();
+                discord_presence.startTimestamp = time(0);

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/DiscordPresence.cpp Outdated

+              void UpdateDiscordPresence(const std::string& details)
+              {
+                // update Discord RPC
+                DiscordRichPresence discord_presence = {0};

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/DiscordPresence.h Outdated

+              // Licensed under GPLv2+
+              // Refer to the license.txt file included.
+              // Discord Presence

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/DiscordPresence.h Outdated

		@@ -0,0 +1,17 @@
		// Copyright 2008 Dolphin Emulator Project

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/DiscordPresence.cpp Outdated

		@@ -0,0 +1,33 @@
		// Copyright 2008 Dolphin Emulator Project

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/DiscordPresence.h Outdated

+              // Adds support for Discord Rich Presence in to Dolphin
+              #pragma once
+              #include <string>

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/DiscordPresence.cpp Outdated

+                Discord_Shutdown();
+              }
+              }

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/DiscordPresence.h Outdated

+              void Init();
+              void UpdateDiscordPresence(const std::string& details);
+              void Shutdown();
+              }

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/DiscordPresence.cpp Outdated

+              // Licensed under GPLv2+
+              // Refer to the license.txt file included.
+              #include "DiscordPresence.h"

This comment was marked as off-topic.

Sign in to view

Member

delroth commented May 27, 2018 via email •

edited

Loading

Hey, thanks for tagging me in here, I can reply by email now! :) From my side: * +1 to the feature. * I'll provide the client id we'll use in the final version to make sure the project keeps proper ownership. For testing just continue using yours for now. * Definitely should be out of Core into either Common or UICommon, probably the latter. * We should put the source code in Externals, not binaries. * On Linux / MacOS there should be a compile time flag to not include this integration. Can probably be enabled by default though. * Not sure if there should be an option in Dolphin to disable this. There is an option in discord to disable "now playing", and I can't think of why someone would want to show dolphin but not the game they're playing. Maybe JMC47 wants to hide his Sexy Poker addiction... Haven't looked at the implementation yet, so high level comments only. Thanks for the contribution!

Contributor

JMC47 commented May 27, 2018

I would never hide my sexy poker addiction.

lioncash reviewed

View reviewed changes

Source/Core/Core/ConfigManager.cpp Outdated

                 if (game_id == "00000000")
                 {
                   m_title_description.clear();
+                  Discord::UpdateDiscordPresence(m_title_description);

This comment was marked as off-topic.

Sign in to view

Contributor Author

yourWaifu commented May 27, 2018 •

edited

Loading

Here are some screenshots.

leoetlino reviewed

View reviewed changes

Source/Core/UICommon/DiscordPresence.cpp

+              // Licensed under GPLv2+
+              // Refer to the license.txt file included.
+              #include "UICommon/DiscordPresence.h"

This comment was marked as off-topic.

Sign in to view

leoetlino reviewed

View reviewed changes

Source/Core/UICommon/DiscordPresence.cpp Outdated

+              {
+                DiscordEventHandlers handlers = {};
+                // The number is the client ID for Dolphin, it's used for images and the appication name
+                Discord_Initialize("450033159212630028", &handlers, 1, NULL);

This comment was marked as off-topic.

Sign in to view

leoetlino reviewed

View reviewed changes

Source/Core/UICommon/DiscordPresence.h Outdated

+              // Licensed under GPLv2+
+              // Refer to the license.txt file included.
+              // Adds support for Discord Rich Presence in to Dolphin

This comment was marked as off-topic.

Sign in to view

leoetlino reviewed

View reviewed changes

Source/Core/UICommon/DiscordPresence.h Outdated


		#pragma once

		#include <string>

This comment was marked as off-topic.

Sign in to view

lioncash reviewed

View reviewed changes

Source/Core/UICommon/DiscordPresence.cpp Outdated

+                discord_presence.largeImageKey = "dolphin_logo";
+                discord_presence.largeImageText = "Dolphin is an emulator for the GameCube and the Wii.";
+                discord_presence.details = title.empty() ? "Not in-game" : title.c_str();
+                discord_presence.startTimestamp = time(nullptr);

This comment was marked as off-topic.

Sign in to view

spycrab reviewed

View reviewed changes

Contributor

spycrab left a comment

Shouldn't we provide a CMake option for enabling / disabling this?

merryhime reviewed

View reviewed changes

Source/Core/UICommon/UICommon.cpp Outdated

@@ @@ -91,6 +93,7 @@ void Shutdown() @@
                 VideoBackendBase::ClearList();
                 LogManager::Shutdown();
                 SConfig::Shutdown();
+                Discord::Shutdown();

This comment was marked as off-topic.

Sign in to view

shuffle2 reviewed

View reviewed changes

Source/Core/UICommon/UICommon.vcxproj Outdated

@@ @@ -35,6 +35,16 @@ @@
                   <Import Project="..\..\VSProps\PCHUse.props" />
                 </ImportGroup>
                 <PropertyGroup Label="UserMacros" />
+                <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">

This comment was marked as off-topic.

Sign in to view

Contributor

shuffle2 commented May 28, 2018 •

edited

Loading

Generally, putting binaries into git / linking against precompiled libs sucks.

fantesykikachu commented May 28, 2018

why is this being done with a precompiled binary, corrent me if i am wrong but the source code for this is here: https://github.com/discordapp/discord-rpc
and not only does the source support windows, linux and mac but also uses cmake for easy integration.

Member

delroth commented May 28, 2018

Yeah, I mentioned it in my previous comment (which somehow I can't figure out how to make readable... Markdown not working for some reason). It's very much a prerequisite for us merging this.

Orphis reviewed

View reviewed changes

Member

Orphis left a comment

This is certainly missing all CMake integration as well to build on other platforms.

Source/Core/UICommon/CMakeLists.txt Outdated

@@ @@ -2,6 +2,7 @@ add_library(uicommon @@
                 AutoUpdate.cpp
                 CommandLineParse.cpp
                 Disassembler.cpp
+              	DiscordPresence.cpp

This comment was marked as off-topic.

Sign in to view

Member

BhaaLseN commented May 28, 2018

💭 Maybe use a submodule for this? Unless we prefer the code copied over.

spycrab reviewed

View reviewed changes

Contributor

spycrab left a comment

The option to enable / disable discord integration needs to be available for CMake as well.

Member

JosJuice commented May 28, 2018

💭 Maybe use a submodule for this? Unless we prefer the code copied over.

I don't think we should have a submodule for this. We already have the source code for many libraries in Externals, and this one isn't an especially large one.

Member

BhaaLseN commented May 28, 2018

Was just a random thought. I see pros and cons to both approaches; but it probably doesn't matter much for the...16 files (+/- some CMake) to be included right away.

Member

BhaaLseN commented May 28, 2018

Also, another random thought from IRC:
What about we introduce a thin indirection to get the calls more localized (ie. in a file named GameStatus, GameMonitor or something similar) vs. being sprinkled all around the initialization/shutdown/game-change logic? That way they'd be closer together and a tiny little bit easier to extend (if someone else wanted to add support for $other-software-here, like Twitch or whatever).
Plus, the (at least on IRC) requested #ifdef guarding would be limited to this single file; again in very close proximity to each other making them easier to discover.
The (small-ish) con is that we'd have yet another indirection that might never be used if nobody else ever implements other reporting/monitoring code.
(and in case it wasn't clear, this is up for discussion; not a "please make it so" request)

Contributor Author

yourWaifu commented May 28, 2018 •

edited

Loading

I'm not sure if this is a problem for you guys, but when moving the Discord RPC library source code to external, I noticed that it uses RapidJSON (Source). I also noticed that Dolphin also has PicoJSON (Source).

Member

delroth commented Jun 12, 2018

@yourWaifu Done. Also in the future I wonder if we should find some nice pictures for the top 20-50 games :)

Contributor Author

yourWaifu commented Jun 14, 2018

@delroth It still doesn't look right. This is what it looks like

and this is what it should look like

Also here's the logo I used. if you have a higher quality one, please use that one for people with higher scaling settings.

Member

delroth commented Jun 14, 2018 via email

What about now?

On Thu, Jun 14, 2018 at 4:06 AM Sleepy Flower Girl ***@***.***> wrote: @delroth <https://github.com/delroth> It still doesn't look right. This is what it looks like [image: image] <https://user-images.githubusercontent.com/16181109/41387440-dc68d082-6f54-11e8-9fbb-09b074a1e22e.png> and this is what it should look like [image: image] <https://user-images.githubusercontent.com/16181109/41387530-5de6da32-6f55-11e8-963d-3830e76f0ac1.png> [image: image] <https://user-images.githubusercontent.com/16181109/41387386-a4209a98-6f54-11e8-916e-9b8e1454175e.png> Also here's the logo I used. if you have a higher quality one, please use that one for people with higher scaling settings. [image: logo] <https://user-images.githubusercontent.com/16181109/41387577-949801dc-6f55-11e8-8cfb-76fc146ed72a.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6983 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMYLl_w79GAOo8Ql6D_d-EXSdl4uoekks5t8cUfgaJpZM4UPK0n> .

yourWaifu force-pushed the add-discord-rpc-support branch from eb6d580 to cd2d5c4 Compare

June 14, 2018 18:23

Contributor Author

yourWaifu commented Jun 14, 2018

@delroth It works now

yourWaifu force-pushed the add-discord-rpc-support branch 4 times, most recently from 790ca07 to 29039ab Compare

June 17, 2018 23:28

Contributor Author

yourWaifu commented Jun 17, 2018

I just tested it working on Linux and Windows (All screenshots above were done in Windows)

@connorshea I think the PR is almost ready, I think that I just need to figure out how to successfully pass all the checks without breaking anything. I don't have access to a computer running macOS so I would totally like you to test it. Open Dolphin while the Discord client is open, and it should show up on your profile. Also test to make sure that settings for this works and can be saved after closing.

connorshea commented Jun 18, 2018 •

edited

Loading

@yourWaifu took a bit to get things set up and compiled, but it works on macOS! :D

Here's what it looks like when I'm actually in a game:

connorshea commented Jun 18, 2018

Also just tested toggling the setting off and it persists when rebooting the emulator. 👍

Contributor

lukearnould commented Jun 18, 2018

@delroth Would there be any way of utilizing games' save icons or images of their discs (from gametdb.com) for Discord game icons? They would be low-resolution but for the many less popular games it would be better than nothing.

Contributor Author

yourWaifu commented Jun 18, 2018 •

edited

Loading

@8times9 Discord placed a limit of 150 for the number of assets that you can upload to Discord. Plus they want you to do via their developer dashboard instead of an API as they don't document any other way to do so. Maybe in the future, they'll add it and then we may add icons for less popular games. I like the idea though.

Contributor Author

yourWaifu commented Jun 20, 2018

I'm not sure if you guys are ok with this so I'm asking about it here. I ran into the linker issue

undefined reference to `MAIN_USE_DISCORD_PRESENCE'

that for some reason only happens on Linux and not Windows. I've done some testing a found that moving MAIN_USE_DISCORD_PRESENCE from UICommon and into Core fixes this issue. I did come up with a different soluation, ditching IsSettingSaveable since it's a temp solution and instead use a vector or list of savable settings, but I felt that would be outside the scope of this pull request.

lioncash reviewed

View reviewed changes

Source/Core/UICommon/DiscordPresence.cpp Outdated


		#include "Core/ConfigManager.h"

		const Config::ConfigInfo<bool> MAIN_USE_DISCORD_PRESENCE{

This comment was marked as off-topic.

Sign in to view

Source/Core/UICommon/DiscordPresence.h Outdated

+              void SetDiscordPresenceEnabled(bool enabled);
+              }  // namespace Discord
+              extern const Config::ConfigInfo<bool> MAIN_USE_DISCORD_PRESENCE;

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/ConfigLoaders/IsSettingSaveable.cpp Outdated

+              #ifdef USE_DISCORD_PRESENCE
+                    // Main.General
+                    // for some reason clang-format didn't like the ifdef below the comment

This comment was marked as off-topic.

Sign in to view

Source/Core/DolphinQt2/Settings.cpp Outdated

@@ @@ -19,6 +19,10 @@ @@
               #include "DolphinQt2/Settings.h"
               #include "InputCommon/InputConfig.h"
+              #ifdef USE_DISCORD_PRESENCE

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Source/Core/Core/ConfigLoaders/IsSettingSaveable.cpp Outdated

@@ @@ -9,6 +9,9 @@ @@
               #include "Common/Config/Config.h"
               #include "Core/Config/GraphicsSettings.h"
+              #ifdef USE_DISCORD_PRESENCE
+              #include "UICommon/DiscordPresence.h"

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

yourWaifu force-pushed the add-discord-rpc-support branch from 29039ab to 0ba9f5b Compare

June 20, 2018 02:26


          Discord Rich Presence CMake integration

63f0345

I have no idea if this works or not. Hopefully the build bot will tell me.

yourWaifu force-pushed the add-discord-rpc-support branch from 0ba9f5b to 63f0345 Compare

June 20, 2018 02:43

Contributor Author

yourWaifu commented Jun 24, 2018 •

edited

Loading

OK, I think this PR is ready to be merged. I think the WIP tag can be removed now. Unless there's something else that I need to do that I'm not aware of.

delroth removed the WIP / do not merge label

Member

delroth commented Jun 24, 2018

There are a few things I'm not completely happy with (the commits ordering, the fact that we don't build from Externals on Windows and instead rely on prebuilts, etc.) but overall LGTM. This PR is getting too long to nitpick, so I'll merge it as is and hopefully people can just send followup fixes if they have objections.

delroth merged commit 2cfdf89 into dolphin-emu:master

yourWaifu deleted the add-discord-rpc-support branch

June 26, 2018 22:50

sboukortt commented Jun 27, 2018

Now, make install installs include/discord_{register,rpc}.h and lib/libdiscord-rpc.a (presumably because of these lines). I assume it is unintended?

Contributor Author

yourWaifu commented Jul 1, 2018

@sboukortt Yes it was unintended and to be honest, I forgot about make install. However, I tested it on my linux machine and didn't run into any issues. I honestly don't see any problem here, and I'm not sure if you disagree or not.

Member

Orphis commented Jul 2, 2018

@yourWaifu It's easy to fix, change add_subdirectory(discord-rpc) to add_subdirectory(discord-rpc EXCLUDE_FROM_ALL)

yourWaifu mentioned this pull request

Add Discord Join Net Play functionally #7249

Merged

4 tasks

baka0815 mentioned this pull request

Add discord button Return-To-The-Roots/s25client#965

Open

r-burns mentioned this pull request

Exclude Discord RPC lib from installation #9305

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

merryhime merryhime left review comments

JosJuice JosJuice left review comments

leoetlino leoetlino left review comments

spycrab spycrab left review comments

lioncash lioncash left review comments

Orphis Orphis left review comments

shuffle2 shuffle2 left review comments

Labels

None yet

22 participants