feat(discord-rpc): improve the system #4271

iHDeveloper · 2020-11-24T13:55:03Z

Contains

This improvement aims to fix bugs in the system.
And, it improves communication between the game and Discord RPC to provide full access to its features.

Source: Discord Developer Documentation

Currently, the system supports state that can be changed dynamically using DiscordRPCSubSystem#setState.
And, details is also supported but its value is fixed as Name: PlayerName. It can't be changed.

This PR is going to implement other features such as partySize, partyMax, etc...

Discussion Points

Turning the default to be dynamic to have different details from different gameplay such as Currently in JoshariasSurvival, etc...
Since the engine has a networking infrastructure I'm thinking that we can implement the secrets (join and spectate)
in a way to provide the modules the ability to receive events like DiscordAskToJoinEvent and DiscordSpectateEvent.
Instead of each module dealing with it in a raw way.

Comment about what do you think of the points above? 🤔

Bugs

Game detection is broken
The game assumes the player is in the lobby while in reality, the player is in-game.
Not thread-safe
The Discord RPC connection is being handled in a separate thread to avoid conflicts with the game.
The system isn't thread-safe which might result in unexpected behavior.

How to test

Fetch the PR commits
git fetch origin pull/4271/head:<localBranchName>
Checkout to the PR's branch
git checkout <localBranchName>
Run the game and make sure you have Discord Rich Presence turned on.
Join a game to trigger your game state from In Lobby to In Game

NOTE:
In terms of unit testing, I'm not sure how I would implement it for this system. 😅

Outstanding before merging

Fix: Game detection is broken (solved by feat(subsystems): add creating and loading reflections.cache for subsystems #4274)
Fix: Not thread-safe (47bae74)
Feat: Ability to change details (48d1502)
Feat: Implement party size
Feat: Implement secrets
Test the RPC
- Single Player
- Multiplayer

lgtm-com · 2020-12-01T16:43:16Z

This pull request introduces 1 alert when merging 84a4b5d into 1f7ea2a - view on LGTM.com

new alerts:

1 for Result of multiplication cast to wider type

...ms/DiscordRPC/src/main/java/org/terasology/engine/subsystem/discordrpc/DiscordRPCThread.java

pollend · 2021-01-02T23:10:03Z

@iHDeveloper what is the status on this and what is left to be implemented?

iHDeveloper · 2021-01-03T14:43:21Z

@iHDeveloper what is the status on this and what is left to be implemented?

Working on it. I'm debugging the party size commit. And, I'm not sure if the secrets should be implemented in this PR. Or, maybe later.

Cervator · 2021-01-03T21:42:39Z

I'm not sure if the secrets should be implemented in this PR

@iHDeveloper I would suggest keeping the PRs about as small as could be, for more frequent merges. Can always keep going in additional PRs :-)

Every checkbox in the bullets above could pretty much be its own PR, although if multiple finish around the same time anyway and overlap at all then grouping them may make sense.

iHDeveloper · 2021-01-07T01:42:57Z

This PR is ready to be reviewed and merged into the codebase. \o/ 😄

I discussed with @Cervator and decided not to implement the secrets feature in this PR.
And, I'm going to keep it to be implemented in the future when needed.

I tested this PR on both Singleplayer and Multiplayer (Hosting only) with the latest build from the develop branch.
I didn't have the ability to test this PR by joining a server. But, I'm going to assume that it works fine on the server.

skaldarnar

Code looks good overall, left a few comments. I haven't tested this, though.

skaldarnar · 2021-01-10T18:24:14Z

.idea/checkstyle-idea.xml

@@ -10,7 +10,7 @@
        <entry key="location-1" value="BUNDLED:(bundled):Google Checks" />
        <entry key="location-2" value="PROJECT_RELATIVE:$PROJECT_DIR$/config/metrics/checkstyle/checkstyle.xml:Terasology" />
        <entry key="property-2.samedir" value="config/metrics/checkstyle" />
-        <entry key="scan-before-checkin" value="false" />
+        <entry key="scan-before-checkin" value="true" />


What kind of scans does this include?

@keturn to the rescue? 🙃

Honestly, I didn't notice this file is added into the commit stage. 😅
But, I can assume it happened because I enabled the Checkstyle scan before VCS check-in (aka before commit).

Edit: Should I disable it?

Yes please - while that's possibly a good practice it is probably better if we leave it up to individual contributors for now, not all of whom may even have the Checkstyle plugin installed (I don't remember if that's a default one) :-)

Probably would be a different project to actually get the Checkstyle config and docs to the point where we could consider doing this for real. We'd want to make sure we have good docs on what a false positive is, for instance, or that might confuse contributors and lead to unrelated "fixes" in PRs we then have to ask to have undone 😅

Then we should find a way to ignore these change by default (put .idea into the git-ignored list?). I think if a user changes this setting, it will automatically reflect in a change in this checked-in config file.... =/

This looks like a topic for another venue. For the purposes of this PR, if it's not obviously the right setting, go ahead and revert the change to this file.

Alright I will revert the change and try to avoid adding those unrelated files by accident 😅

skaldarnar · 2021-01-10T18:27:04Z

...DiscordRPC/src/main/java/org/terasology/engine/subsystem/discordrpc/DiscordRPCSubSystem.java

        }
-        logger.info("Discord RPC >> Disconnected!");
+        thread.start();


Since we log the start of initializing as INFO, should we also load the success when the thread was started (analog to the case where the integration is disabled).

The purpose of the thread is to be alive all the time until the game shuts down. But if the user disabled the RPC the thread will be in the sleeping mode until something changes and notifies it. When the thread gets notified it tries to connect to the IPC server. If it fails to connect 5 times at maximum then the thread will go sleep again.

skaldarnar · 2021-01-10T18:29:18Z

...ms/DiscordRPC/src/main/java/org/terasology/engine/subsystem/discordrpc/DiscordRPCThread.java

+
+public final class DiscordRPCThread implements IPCListener, Runnable {
+    private static final Logger logger = LoggerFactory.getLogger(DiscordRPCThread.class);
+    private static final long DISCORD_APP_CLIENT_ID = 515274721080639504L;


Is this a random number or something assigned to us in some way?

This random represents the client ID of the Discord application. Discord client needs this ID to fetch information and assets about the application to display it to the rich presence.

Yeah I think that might be associated with our registered app on Discord, which I originally set up. Nowadays the app is owned by a team with several of us having access 👍

Is that sensitive information (like an access token) or more of a global identifier?

Just an identifier 👍 There's also a secret but I don't think it is needed for anything in this case.

Incidentally you should have access to it via https://discord.com/developers/teams/781638052501520417 although it looks like you might have gotten an invite and not accepted it yet (Nira is in)

Yep as Cervator said it's an identifier so it's not sensitive information. And, about the secret, it's mostly used in OAuth2 applications which I think we don't need soon.

I agree with what Cervator said. And about the secret, it's mostly used in OAuth2 applications which I don't think we will need soon.

skaldarnar · 2021-01-10T18:31:23Z

...ms/DiscordRPC/src/main/java/org/terasology/engine/subsystem/discordrpc/DiscordRPCThread.java

+    @Override
+    public void run() {
+        while (true) {
+            logger.info("Waiting for auto-connecting...");


How much will this actually log on the INFO level? I'm cautious when I see logger.info in combination with loops or frequently called methods as this might spam the log.

The purpose of the loop is to keep the thread alive. It's rare for this auto-connect log to appear.

It will appear by one of these conditions below:

Once on the start

Every time the player disables the rich presence from the player settings (if the player doesn't have the Discord client)

When the thread tries to find the Discord client but still couldn't connect to it.

Likewise a bit concerned here, it doesn't seem overly far fetched that a connectivity issue could appear then suddenly the log is being filled by junk due to a decidedly optional extra that you could play without perfectly fine otherwise :-)

In other words: if the Discord integration breaks or is in a bad state it shouldn't be too loud about it - ideally just log once per "incident" however we define that

So what happens if the discord servers are down, or the player just does not have an active internet connection?

The Discord client hosts a local IPC server for the applications to connect it.
When the application connects it exchanges information with the server to identify the application.
And then the application can send RPC information every x amount of time to update it.
The Discord client decides which application RPC will be shown depends on different factors.

If something happens in the Discord servers the only affected side is going to be the Discord client.
The Discord Client runs the IPC server when the client starts. And it doesn't stop until the client is being shutdown.
And until that happens it's going to stay listening to application RPCs whether the Discord server is down or not.

It's user-specific file. It shouldn't be shared since each user has its own settings.

Change main menu state from "In Lobby" to "In Main Menu"

The rewrite was necessary to be able to achieve the system to be thread-safe. It gave the opportunity to organise how the subsystem works. This also ensures the subsystem is easy to: - Implement new features! - Fix bugs without breaking stuff - Separated the thread into its own class - Subsystem manages the communication with the thread - Re-organized the system - Fix: disable discord-ipc library logger

Used to set the gameplay name (e.g. Josharias Survival, Metal Renegades, etc...) It will show on discord in this format "Game: <name>" - Improve the buffer change trigger and handler - Ability to change details in the buffer - Apply the new format for the rich presence -- Details -> "Game: <name>" -- Party State -> "Playing Solo" / "Playing Online" / "Hosting" - Fix: Triggering `Thread#setEnabled` with keeping the connection alive - Fix: No safe shutdown to the RPC thread

- fix: can't hide start timestamp

iHDeveloper · 2021-02-18T16:20:24Z

Pull with rebase to resolve conflict with develop branch
Remove the change from .idea/checkstyle-idea.xml

PR ready to be merged 😄

skaldarnar · 2021-03-05T21:48:37Z

😕 Wanted to briefly test this on my Linux machine, ending up with an exception...

22:47:14.806 [main] INFO  o.t.e.s.d.DiscordRPCSubSystem - Initializing...
22:47:14.808 [DISCORD-RPC-THREAD] INFO  o.t.e.s.discordrpc.DiscordRPCThread - Waiting for auto-connecting...
22:47:14.810 [DISCORD-RPC-THREAD] INFO  o.t.e.s.discordrpc.DiscordRPCThread - Waiting for enabling...
22:47:14.810 [DISCORD-RPC-THREAD] INFO  o.t.e.s.discordrpc.DiscordRPCThread - Waiting for connection...
22:47:14.810 [DISCORD-RPC-THREAD] INFO  o.t.e.s.discordrpc.DiscordRPCThread - Connecting to Discord RPC...
Exception in thread "DISCORD-RPC-THREAD" java.lang.RuntimeException: org.newsclub.net.unix.AFUNIXSocketException: No such file or directory (socket: /run/user/1000/discord-ipc-0)
        at com.jagrosh.discordipc.entities.pipe.Pipe.createPipe(Pipe.java:167)
        at com.jagrosh.discordipc.entities.pipe.Pipe.openPipe(Pipe.java:67)
        at com.jagrosh.discordipc.IPCClient.connect(IPCClient.java:116)
        at org.terasology.engine.subsystem.discordrpc.DiscordRPCThread.run(DiscordRPCThread.java:170)
        at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: org.newsclub.net.unix.AFUNIXSocketException: No such file or directory (socket: /run/user/1000/discord-ipc-0)
        at org.newsclub.net.unix.NativeUnixSocket.connect(Native Method)
        at org.newsclub.net.unix.AFUNIXSocketImpl.connect(AFUNIXSocketImpl.java:136)
        at org.newsclub.net.unix.AFUNIXSocket.connect(AFUNIXSocket.java:107)
        at org.newsclub.net.unix.AFUNIXSocket.connect(AFUNIXSocket.java:98)
        at com.jagrosh.discordipc.entities.pipe.UnixPipe.<init>(UnixPipe.java:46)
        at com.jagrosh.discordipc.entities.pipe.Pipe.createPipe(Pipe.java:163)
        ... 4 more

Discord integration works in principle, at least Spotify manages to display my activity nicely...

Edit: this might be related to having Discord installed via snap or flatpak. See jagrosh/DiscordIPC#16 and flathub/com.discordapp.Discord#29.

Seems like the actual socket file on my system is /run/user/1000/app/com.discordapp.Discord/discord-ipc-0 .

Edit 2: Followed the steps in https://github.com/flathub/com.discordapp.Discord/wiki/Rich-Precense-(discord-rpc) and got it to work with flatpak.

iHDeveloper force-pushed the discord-rpc/improvement branch from 7cbe17b to 4214950 Compare November 25, 2020 11:59

iHDeveloper changed the title ~~Sub-System Improvement: Discord-RPC~~ feat(discord-rpc): improve the system Nov 25, 2020

iHDeveloper mentioned this pull request Nov 26, 2020

feat(subsystems): add creating and loading reflections.cache for subsystems #4274

Merged

iHDeveloper force-pushed the discord-rpc/improvement branch from 4214950 to df87bc7 Compare November 26, 2020 20:47

jdrueckert self-requested a review November 26, 2020 21:52

iHDeveloper force-pushed the discord-rpc/improvement branch 2 times, most recently from acb8b98 to 84a4b5d Compare December 1, 2020 16:32

iHDeveloper force-pushed the discord-rpc/improvement branch 2 times, most recently from dbd3324 to 47bae74 Compare December 1, 2020 16:54

DarkWeird reviewed Dec 1, 2020

View reviewed changes

...ms/DiscordRPC/src/main/java/org/terasology/engine/subsystem/discordrpc/DiscordRPCThread.java Show resolved Hide resolved

iHDeveloper force-pushed the discord-rpc/improvement branch 3 times, most recently from e02c616 to e3b8063 Compare January 7, 2021 01:19

iHDeveloper marked this pull request as ready for review January 7, 2021 01:42

skaldarnar reviewed Jan 10, 2021

View reviewed changes

skaldarnar requested a review from pollend January 10, 2021 18:32

skaldarnar added this to the v4.3.0 milestone Feb 18, 2021

iHDeveloper added 5 commits February 18, 2021 19:10

chore(idea): ignore discord plugin settings

d8e959d

It's user-specific file. It shouldn't be shared since each user has its own settings.

feat(discord): change main menu state

f8077d8

Change main menu state from "In Lobby" to "In Main Menu"

feat(discord): Implement party size

dc1c158

- fix: can't hide start timestamp

iHDeveloper force-pushed the discord-rpc/improvement branch from e3b8063 to dc1c158 Compare February 18, 2021 16:10

Merge branch 'develop' into discord-rpc/improvement

54784a0

skaldarnar added the Type: Improvement Request for or addition/enhancement of a feature label Mar 5, 2021

Merge branch 'develop' into discord-rpc/improvement

7fd8605

skaldarnar approved these changes Mar 5, 2021

View reviewed changes

skaldarnar merged commit f454f38 into MovingBlocks:develop Mar 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(discord-rpc): improve the system #4271

feat(discord-rpc): improve the system #4271

iHDeveloper commented Nov 24, 2020 •

edited

Loading

lgtm-com bot commented Dec 1, 2020

pollend commented Jan 2, 2021 •

edited

Loading

iHDeveloper commented Jan 3, 2021

Cervator commented Jan 3, 2021

iHDeveloper commented Jan 7, 2021

skaldarnar left a comment

skaldarnar Jan 10, 2021

iHDeveloper Jan 17, 2021 •

edited

Loading

Cervator Jan 17, 2021

skaldarnar Jan 19, 2021

keturn Jan 19, 2021

iHDeveloper Jan 27, 2021

iHDeveloper Jan 27, 2021

skaldarnar Jan 10, 2021

iHDeveloper Jan 27, 2021

skaldarnar Jan 10, 2021

iHDeveloper Jan 17, 2021

Cervator Jan 17, 2021

skaldarnar Jan 19, 2021

Cervator Jan 20, 2021

iHDeveloper Jan 27, 2021

skaldarnar Jan 10, 2021

iHDeveloper Jan 17, 2021

Cervator Jan 17, 2021

skaldarnar Jan 19, 2021

iHDeveloper Jan 27, 2021

iHDeveloper commented Feb 18, 2021 •

edited

Loading

skaldarnar commented Mar 5, 2021 •

edited

Loading

feat(discord-rpc): improve the system #4271

feat(discord-rpc): improve the system #4271

Conversation

iHDeveloper commented Nov 24, 2020 • edited Loading

Contains

Discussion Points

Bugs

How to test

Outstanding before merging

lgtm-com bot commented Dec 1, 2020

pollend commented Jan 2, 2021 • edited Loading

iHDeveloper commented Jan 3, 2021

Cervator commented Jan 3, 2021

iHDeveloper commented Jan 7, 2021

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iHDeveloper Jan 17, 2021 • 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

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

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

Choose a reason for hiding this comment

iHDeveloper commented Feb 18, 2021 • edited Loading

skaldarnar commented Mar 5, 2021 • edited Loading

iHDeveloper commented Nov 24, 2020 •

edited

Loading

pollend commented Jan 2, 2021 •

edited

Loading

iHDeveloper Jan 17, 2021 •

edited

Loading

iHDeveloper commented Feb 18, 2021 •

edited

Loading

skaldarnar commented Mar 5, 2021 •

edited

Loading