Skip to content
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

Multiplayer APIv2 #9497

Draft
wants to merge 157 commits into
base: master
Choose a base branch
from
Draft

Multiplayer APIv2 #9497

wants to merge 157 commits into from

Conversation

CrsiX
Copy link
Contributor

@CrsiX CrsiX commented Jun 1, 2023

This may be a larger PR and I'm open to suggestions. Please read on.

Highlights

  • in-game chats
  • lobbies & invites
  • friend system & friend chats
  • event notifications
  • lazy loading of data
  • it works™

What it does

This PR introduces a new library (ktor) and sub-package that speaks with our new multiplayer server, as well as a lot of new UI elements that make use of those new API features, bundled with some new events to integrate the WebSocket smoothly. It already comes with a new Android turn checker as well. For more details, see #8817.

We have carefully checked the new experience. It works pretty well, except for the list of bugs and other issues I created at our fork. Most importantly, I don't want to break backwards-compatibility with APIv0 and APIv1 (that's how I started to call the Dropbox and all the Dropbox-replacement servers, respectively). That means: at the game start (as well as on server change), the server is queried for the /isalive endpoint to determine if it's an "old-style" server. If not (response code 400), it is checked for /api/version. Only if the apiVersion can be matched, you will see the new features. Otherwise, everything should stay the way it currently is: players would not see any difference if they don't use a new APIv2-compatible server.

Note, however, that the servers are not compatible for a specific game. At the moment, you can't create a game in APIv1 and continue playing in APIv2 (or vice versa), and there are no plans to allow that, yet. That means: a party needs to decide on a server to play on beforehand.

API

I bundled all the details in com.unciv.logic.multiplayer.apiv2. It includes the main class APIv2, which holds a lot of other stuff besides the core functionality, which resides in APIv2Wrapper. Accessing functionality is possible like so: UncivGame.Current.onlineMultiplayer.api.auth.login to use the login endpoint, for example. There's also ApiV2FileStorageEmulator which enabled in-place usage of the new functionality whenever the current APIv0/APIv1 would be used (e.g. saving/loading games).

The "game preview" mechanism is not used at all. The server holds a counter dataID, which is incremented every time some player uploads a new game. Checking the value of that counter is enough to detect changed game states.

UX

There are a bunch of new screens, buttons, tables and other elements around. Some important screens:

  • A new LobbyBrowserScreen when you click on "multiplayer" in the main menu, which provides a list of your current games and all open lobbies of the current server, as well as buttons to create lobbies, manage friends, ...
  • A new MultiplayerGameScreen which you see when you click on the multiplayer button of the WorldScreen, providing the chat, player operations, access to the friends and recent games
  • A new LobbyScreen which replaced the NewGameScreen for our purposes here, it also holds a chat window

The user is usually notified by popups about incoming events (e.g. new chat message, lobby invitation, a friendship request, ...).

Missing things

  • Most importantly, any kind of persistence: Whenever the code needs some values, the server is queried for them (except if it's just the current operation, then the values are piggy-backed as long as needed). This means you probably see requests whenever some window is opened, because the data is loaded lazy. It doesn't bother or block the UI, though. At the moment, I didn't bother, because a) the server is fast and b) the data is very small, only a few hundred bytes at most usually.
  • Android turn check re-connect fixes, since it's pretty hard to keep the WebSocket alive and the data in sync. I've already spent hours tweaking that.
  • A few buttons and other elements will just popup with "not implemented yet". Of course, those things will be addressed in the future.
  • Joining a lobby by invitation & one-to-one in-game chats (all-player chat exists, though).

Take note

  • Changing the server or checking the connection will drop the instance of OnlineMultiplayer now, so that everything is cleaned up properly.
  • The NewGameScreen will use horizontally aligned picker buttons instead of vertically aligned as before (provides much more screen space).
  • There are no localized strings anywhere, because a) this is a draft and b) I wasn't sure how to do it properly.
  • Kotlin builds take seemingly longer now, or is my machine just slow? I thought about splitting all that apiv2 package stuff out, but that didn't work out easily.
  • I think I didn't change any existing data structures, so that everything should just continue working. The only exception is the data format of the uploads to APIv2 servers, which is "full" JSON, but since the PR 9427 is merged, this is no specialty anymore.

cc @SomeTroglodyte @touhidurrr

If you checkout and build, I suggest the following patch (it provides better debugging experience when testing on multiple devices):

diff --git a/android/AndroidManifest.xml b/android/AndroidManifest.xml
index e0ef891c2..5cc931a0a 100644
--- a/android/AndroidManifest.xml
+++ b/android/AndroidManifest.xml
@@ -21,7 +21,7 @@
         android:allowBackup="true"
         android:icon="@mipmap/uncivicon"
         android:roundIcon="@mipmap/uncivicon_round"
-        android:label="@string/app_name"
+        android:label="Unciv V2"
         android:isGame="true"
         android:largeHeap="true"
         android:appCategory="game"
diff --git a/android/build.gradle.kts b/android/build.gradle.kts
index cfd212cfd..f35a9c2c9 100644
--- a/android/build.gradle.kts
+++ b/android/build.gradle.kts
@@ -26,7 +26,7 @@ android {
         resources.excludes += "DebugProbesKt.bin"
     }
     defaultConfig {
-        applicationId = "com.unciv.app"
+        applicationId = "com.unciv.app.apiv2"
         minSdk = 21
         targetSdk = 32 // See #5044
         versionCode = BuildConfig.appCodeNumber
diff --git a/android/src/com/unciv/app/AndroidLogBackend.kt b/android/src/com/unciv/app/AndroidLogBackend.kt
index ed32784cc..0efba78c2 100644
--- a/android/src/com/unciv/app/AndroidLogBackend.kt
+++ b/android/src/com/unciv/app/AndroidLogBackend.kt
@@ -18,7 +18,7 @@ class AndroidLogBackend : LogBackend {
     }
 
     override fun isRelease(): Boolean {
-        return !BuildConfig.DEBUG
+        return false // !BuildConfig.DEBUG
     }
 
     override fun getSystemInfo(): String {
diff --git a/core/src/com/unciv/Constants.kt b/core/src/com/unciv/Constants.kt
index 977c70e80..fe9a2f6c2 100644
--- a/core/src/com/unciv/Constants.kt
+++ b/core/src/com/unciv/Constants.kt
@@ -87,6 +87,7 @@ object Constants {
 
     const val dropboxMultiplayerServer = "Dropbox"
     const val uncivXyzServer = "https://uncivserver.xyz"
+    const val runcivServer = "https://runciv.hopfenspace.org"
 
     const val defaultTileset = "HexaRealm"
     const val defaultUnitset = "AbsoluteUnits"
diff --git a/core/src/com/unciv/models/metadata/GameSettings.kt b/core/src/com/unciv/models/metadata/GameSettings.kt
index eafb8e4f8..3be4b6f59 100644
--- a/core/src/com/unciv/models/metadata/GameSettings.kt
+++ b/core/src/com/unciv/models/metadata/GameSettings.kt
@@ -225,7 +225,7 @@ class GameSettingsMultiplayer {
     var passwords = mutableMapOf<String, String>()
     @Suppress("unused")  // @GGuenni knows what he intended with this field
     var userName: String = ""
-    var server = Constants.uncivXyzServer
+    var server = Constants.runcivServer
     var friendList: MutableList<FriendList.Friend> = mutableListOf()
     var turnCheckerEnabled = true
     var turnCheckerPersistentNotificationEnabled = true
diff --git a/core/src/com/unciv/ui/crashhandling/CrashScreen.kt b/core/src/com/unciv/ui/crashhandling/CrashScreen.kt
index 58cdf49ba..6aeedf1cc 100644
--- a/core/src/com/unciv/ui/crashhandling/CrashScreen.kt
+++ b/core/src/com/unciv/ui/crashhandling/CrashScreen.kt
@@ -181,7 +181,7 @@ class CrashScreen(val exception: Throwable): BaseScreen() {
         )
             .onClick {
                 if (copied) {
-                    Gdx.net.openURI("https://github.com/yairm210/Unciv/issues")
+                    Gdx.net.openURI("https://github.com/hopfenspace/Unciv/issues")
                 } else {
                     ToastPopup(
                         "Please copy the error report first.",
diff --git a/desktop/src/com/unciv/app/desktop/DesktopLogBackend.kt b/desktop/src/com/unciv/app/desktop/DesktopLogBackend.kt
index 122c167ef..6cbbe09e2 100644
--- a/desktop/src/com/unciv/app/desktop/DesktopLogBackend.kt
+++ b/desktop/src/com/unciv/app/desktop/DesktopLogBackend.kt
@@ -11,7 +11,7 @@ class DesktopLogBackend : DefaultLogBackend() {
             && System.getProperty("kotlinx.coroutines.debug") == null
 
     override fun isRelease(): Boolean {
-        return release
+        return false
     }
 
     override fun getSystemInfo(): String {

CrsiX added 30 commits May 4, 2023 23:32
This is mandatory since for API v2, the server will create and
validate the game UUIDs. Generating them on the client is therefore
no viable solution.
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@CrsiX
Copy link
Contributor Author

CrsiX commented Jun 25, 2023

I'm curious. The runOnGLThread uses the Dispatcher.GL with Gdx.app.postRunnable. I'm using the runOnGLThread for most modifications to the UI (and where I don't, I have crashes sometimes -- remember that LobbyPlayerList, a huge mess). As far as the documentation is concerned, the GL context is single-threaded.

  1. Is this still the case in Unciv? (I don't see anything telling otherwise, but just to be sure.)
  2. Can I always assume that runOnGLThread will be synchronized, i.e. it won't require mutexing to avoid concurrent access to variables, if all accesses are done on the GL thread? (As long as it's not disposed.)

I can't finally answer those two questions just by reading docs & code. Thanks :)

@SomeTroglodyte
Copy link
Collaborator

As far as I know, in the end, the actual "executor" of such a block would be - for desktop - in this source line - you'll see a synchronized un-stack, but that's all. It's inside the application class'es loop() method, which as far as I know simply ticks with typically 60fps, calls draw, actions.act and so on... So that would make a "Yes, Yes"??? I haven't read the other backends though.

@CrsiX
Copy link
Contributor Author

CrsiX commented Jun 25, 2023

If we put our trust into StackOverflow answers here, then it's also single-threaded on Android. I will just try implementing it that way and see if it works or not.

@SomeTroglodyte
Copy link
Collaborator

Never. Trust. Anybody. Hmmm... Noo, too weak as a joke. Anyway, here's confirmation - they're called from the actual frame render routine.

Interesting, however, that they suppress any exceptions there and only log them, while desktop does no such thing..

@yairm210
Copy link
Owner

yairm210 commented Jul 8, 2023

GL is single threaded always because that's how G
OpenGL works

Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Mar 14, 2024
@SeventhM SeventhM mentioned this pull request Mar 16, 2024
2 tasks
@SeventhM
Copy link
Collaborator

If you say you're still working on this, I'll trust you and leave this to not stale (unless Yairm) objects to the label here as well). If noting else, someone could grab most of the work and probably implement it

@CrsiX
Copy link
Contributor Author

CrsiX commented Mar 20, 2024

It's fine if this PR gets closed by the bot if you ask me. The work I have done is still there; and since it was unreviewable due to size back then, I would need to break it down into smaller components that can be integrated one by one -- this is the tricky part. And since I haven't touched it in half a year, it will be a mess for me just as for anyone else (but if anyone actually wants to touch it, feel free) :D

@github-actions github-actions bot removed the Stale label Mar 20, 2024
Copy link

github-actions bot commented May 4, 2024

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Jun 21, 2024
@touhidurrr
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

Lol. Whats the purpose of will not stale tag then?

@github-actions github-actions bot removed the Stale label Jun 22, 2024
@SeventhM
Copy link
Collaborator

Huh... I bet the will not stale tag is only applying to issues and not PRs

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

Successfully merging this pull request may close these issues.

6 participants