yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.24k stars 1.54k forks source link

Feature request: multiplayer game with server API and more features #8817

Open CrsiX opened 1 year ago

CrsiX commented 1 year ago

Hi, we really like the game but the multiplayer functionality just sucks, sorry. (And the server is pretty ... hacky. Maybe a plain FTP server would have been a better choice).

Therefore, we would like to put our hands on a new multiplayer service. We essentially build our own server (in Rust) while also hooking the relevant client functionality. Additionally, we integrate feature requests such as the following:

However, we would like some help on integrating some of the stuff in the client app. For example, we build the server and the API and all of the network-related code, while someone else could jump in to tweak the user interface to make use of the newly available features (e.g. chatting or username/password instead of UUIDs).

Side question: how many people are currently using your public multiplayer service? A rough approximation (20 a day or 20k a day)?

yairm210 commented 1 year ago

That's a question for @touhidurrr, he built the server at https://github.com/touhidurrr/UncivServer.xyz Given the '3M requests per day' I assume we're talking more in the 20K range

I have no problem with overhauling the API (current API was basically a MVP for migrating away from Dropbox, hence the FTP-ness) But we need to let existing users retain their current games.

This means one of the following:

I would obviously prefer the former but @touhidurrr also has his own free time restrictions.

@GGGuenni has been leading the recent changes for passwords &c so he's more relevant to this than I am As I have stated many times in the past, this is super outside of the roadmap for the project - but if you have people willing to work on it from both sides, then why not

Starting with the basics - 'create multiplayer game and allow others to join'. We'll want to start with a really stripped-down POC:

That's already a lot of work, and this doesn't include issues like

touhidurrr commented 1 year ago

why not websocket? i think current fileserver is good enough, but if we want to implement all those features you are talking about,websocket might be better. also, i would be free after a week or so, at the time my mid term examination finishes. jobless now.

yairm210 commented 1 year ago

I'm not against, do whatever feels best to y'all

touhidurrr commented 1 year ago

Also due to some issues, i am actually rewriting UncivServer.xyz.

myOmikron commented 1 year ago

Hey guys, I'm with @CrsiX,

nice to hear that you're open to improvements in this regard :)

But we need to let existing users retain their current games.

I'd personally would go for a new implementation, let's call it v2 for now. It makes it easier to make design choices if you don't try to reuse the existing implementation. So I think it would be best to add another option to the multiplayer settings for this, like it is now for dropbox vs uncivserver.xyz

We're also open for any contributions, regardless if it is code or suggestions regarding API design @touhidurrr and @GGGuenni.

We'll want to start with a really stripped-down POC:

The suggested scope sounds reasonable for a first prototype


why not websocket? i think current fileserver is good enough, but if we want to implement all those features you are talking about,websocket might be better. also, i would be free after a week or so, at the time my mid term examination finishes. jobless now.

I think websockets would greatly improve the load on the server, as there's no "is there any new data?" like requests anymore.

I would use an API for the lobby mangement and go for websockets for exchanging the actual game data (and chatting in the later implementation).

touhidurrr commented 1 year ago

Ok, or let's split this into separate parts and implement them one by one: I would like to work on the lobby and chat feature first. Preferably in-game chat. The concept of in-game chat is pretty simple. You would join via WebSocket and any message you send will be broadcasted to other players. There would be no chat logs stored on the server, and if you go out of the game then you lose the chats. This is actually pretty common and examples are chat in google wrokspace products (meet, docs, sheets etc.), IRC chats etc. I think this can be done within a week or less.

CrsiX commented 1 year ago

As far as I understand you, @touhidurrr, you "just" want to improve the current multiplayer handling with a few new features (and a server re-write on your side). However, "our" current approach is completely overhauling the whole multiplayer handling. That's why I said earlier:

we build the server and the API and all of the network-related code

This includes both server and client. Therefore, the question: do you want to participate in designing and implementing the new architecture? Because splitting the work only makes sense if we pull together.

Our server implementation work already started here: https://github.com/hopfenspace/runciv

touhidurrr commented 1 year ago

However, "our" current approach is completely overhauling the whole multiplayer handling.

Oh, I think I misunderstood the context of the issue. Good work!

the question: do you want to participate in designing and implementing the new architecture?

I don't know rust, so no. However, I would like to participate on the design aspect of it. I already have many design concepts in my mind for many features like Lobby, Chat, etc. But not as a REST API but as a WebSocket server. Obviously, some endpoints can be REST but overall it is better to avoid REST as a whole to make the mp experience feel more seamless. As polling is not really a pleasant experience for anyone. I guess it is time to write them down and take opinions and start working on them. I guess we can share our design choices and discuss among us to build the best Multiplayer server ever. Also, the focus should not be just rust but a good standard that can improve the Multiplayer experience as a whole.

touhidurrr commented 1 year ago

Side question: how many people are currently using your public multiplayer service? A rough approximation (20 a day or 20k a day)?

I think this is the data you are looking for: image total_requests_2023-03-05T19_03_02.562Z.csv unique_visitors_2023-03-05T19_03_02.564Z.csv

Also see: https://docs.google.com/spreadsheets/d/e/2PACX-1vSPk2pf0_SGVm08pXLmrJjStlVh33ItHweVOYFdqqP9Ghtec1rvTuz9GmfP-zQIWfiB2TGtJU7kn6Lu/pubchart?oid=2039282213&format=image

CrsiX commented 1 year ago

Thanks for the insight. Around 100k users is a good value for this game, but no stressful amount for a good server as well.

Do you want to join us at the discussion page? :)

touhidurrr commented 1 year ago

Thanks for the insight. Around 100k users is a good value for this game, but no stressful amount for a good server as well.

This is monthly statistics, but yes, that is not considered as high volume traffic.

Do you want to join us at the discussion page? :)

Yes. I read your rust code and there is some things that I would like to talk about. However, I would prefer if for Unciv Multiplayer v2, we discuss a list of features that we would like to implement, open a new issue with a checklist in this repo and then start working on that. Also, @yairm210 from you post https://github.com/yairm210/Unciv/issues/8817#issuecomment-1455022553, I am not sure if you want to maintain UncivServer.jar and it would be good if you could clarify that, but regardless I think it would be good if we open a new branch to and start writing proper docs UncivServer API and possibly implementation for UncivServer.jar (given that many players from time to time want to start a server locally) and the client.

yairm210 commented 1 year ago

Unciv.jar can die as far as I'm concerned. It was just a temporary hack to demonstrate the API so people who wanted to stop using Dropbox could.

yairm210 commented 1 year ago

Like with everything, I'm in favor of burning all the old stuff to the ground - provided that we have a replacement that's been tested and that we can phase out the old gradually. Which is why it's important for us to maintain compatibility to the current server, at least until such a time that the current server uses the new format, or - alternatively - all of our users successfully migrate to the new one.

touhidurrr commented 1 year ago

Which is why it's important for us to maintain compatibility to the current server

I think this is a good time to share this poll: image

CrsiX commented 1 year ago

Which is why it's important for us to maintain compatibility to the current server, at least until such a time that the current server uses the new format, or - alternatively - all of our users successfully migrate to the new one.

I think the following mechanism solves the problem with relatively low effort (at the time the new server is stable and ready):

But what about the server address? It just continues to work. The aforementioned mechanism is a kind of version auto-detection. However, to make things perfectly clear, I would like to introduce a new endpoint: GET /api/version. This endpoint determines the API version as simple JSON body with a single integer. E.g., for all current servers, it yields {"version": 1}. For our implementation, it returns {"version": 2} instead.

Note that currently we plan to have a completely different user handling. Therefore, this approach requires significant client-side rework (and extension). I'm already looking into it...

yairm210 commented 1 year ago

That already exists in the /isalive check, it should already return the version for password-authenticated servers Implemented by @GGGuenni We could have it as an entirely different endpoint, YMMV

GGGuenni commented 1 year ago

Yea I was planing to introduce Websockets with the next server feature: Lobbys I don't know how much our ideas are clashing though but we can probably find a solution we can agree on I will say though that chat is nothing I want to work on, so I won't be helping to implement that

I went with the server features approach to give server owners (and developers) enough time to adopt changes and to be able to deprecate individual modules instead of replacing everything at once as soon as the old "dropbox way" of doing things isn't used anymore The feature set endpoint could be moved to it's own endpoint but I don't really see a reason to do so since we would need to make two API calls instead of one

CrsiX commented 1 year ago

I went with the server features approach to give server owners (and developers) enough time to adopt changes and to be able to deprecate individual modules instead of replacing everything at once as soon as the old "dropbox way" of doing things isn't used anymore

An understandable approach. But the number of server developers and server operators is pretty limited, AFAIK. Is it really useful to have long periods of upgrade?


You can find our currently implemented API here: https://runciv.hopfenspace.org/docs/ Note that it changes rapidly, since we're implementing with a fast-forward approach. You can try those features, if you want.

I'm going to implement the PoC mentioned above, so that we can further elaborate the next steps, because it requires a lot of UI work, too.

SomeTroglodyte commented 1 year ago

Unciv.jar can die

You mean the UncivServer.jar included in the main Project - I concur. And that "beer iron" code looks - from a very superficial look - like a significant amount of investment and expertise.

touhidurrr commented 1 year ago

Unciv Multiplayer v2 Protocol (proposal):

Standards: Multiplayer v2 will be a WebSocket only protocol. The previous rest api's might still be supported for backward compatibility. However, no further development of any REST API will be considered a good standard.

Message format: All messages would be valid jsons and have to have a cmd property. all other properties will be camel case. shorter but readable properties are advised to be used. responses should have the same cmd value.

/isalive

Commands & Responses: --Ping ping > {"cmd":"ping"} < {"cmd":"ping"} must ping every 10s or a client would be deemed intactive.

--Popup servers can send popup commands to display a popup to clientside. < {"cmd":"popup","text":"text"}

--Errors same as popup, but client should display this as error < {"cmd":"error","data":error"}

--Accounts register > {"cmd":"register","user":"username","pass":"password","email":"email"} [ email is optional, some servers can choose to support it]

[--to be continued]

touhidurrr commented 1 year ago

@yairm210 @GGGuenni @myOmikron @CrsiX is something like this good enough? in websocket, everyone seems to make their own protocol. but json seems to be commonly used.

CrsiX commented 1 year ago

Multiplayer v2 will be a WebSocket only protocol. The previous rest api's might still be supported for backward compatibility. However, no further development of any REST API will be considered a good standard.

I oppose this proposal for two reasons.

--Ping ping

WebSocket has built-in Frame types. Two of them are Ping and Pong. Therefore, we're not defining our own Ping mechanism, but rather use the one provided by the supporting WS library.

--Popup servers can send popup commands to display a popup to clientside.

This is too generic in my opinion and it makes internationalization harder. I would therefore prefer common codes for all possible messages, so that a) the implementations can use enums for them instead of strings and b) those enums can be translated to all different languages. It would be better to indicate the type of pop-up (e.g. ClientDisconnected, ServerFailure, ...).


We've also started to work on a WebSocket protocol. It's built on JSON sent via WebSocket Text frames and structured like the following:

{
    "type": "string_indicating_the_message_type",
    "content": {
        // various content based on the message type, which allows for easier parsing by libraries
    }
}

There are already a few defined types: InvalidMessage, FinishedTurn, UpdateGameData, ClientDisconnected, ClientReconnected The structure of the content field is solely determined by one of the types.

touhidurrr commented 1 year ago

@CrsiX I the main reason for avoiding REST APIs is to avoid polling. Is there any reason of making things complicated by keeping REST API alongside WebSocket? I see no point in that. OpenAPI is just another standard and a way of documenting REST APIs, the argument here is whether there is any reason to keep REST APIs in the first place. WebSocket is needed to ditch the current polling mechanism that Unciv uses. And Since we are trying to redefine the Multiplayer experience in Unciv, most of the people in this chat have the sentiment of ditching v1 and completely rebuilding v2 in this chat. I see no reason of keeping legacy REST APIs when WebSocket is being implemented.

To ensure a seamless Multiplayer Experience, everything is better to be built on WebSocket from scratch.

touhidurrr commented 1 year ago

This is too generic in my opinion and it makes internationalization harder. I would therefore prefer common codes for all possible messages, so that a) the implementations can use enums for them instead of strings and b) those enums can be translated to all different languages. It would be better to indicate the type of pop-up (e.g. ClientDisconnected, ServerFailure, ...).

I agree with that, but there should be ways to send popup messages from the server. Maybe we can implement types for that.

touhidurrr commented 1 year ago

"type": "string_indicating_the_message_type",

Also kindly use camelCase, they are more readable.

CrsiX commented 1 year ago

I agree with that, but there should be ways to send popup messages from the server. Maybe we can implement types for that.

Could you name me a use case for that feature?

myOmikron commented 1 year ago

I see no reason of keeping legacy REST APIs when WebSocket is being implemented.

To ensure a seamless Multiplayer Experience, everything is better to be built on WebSocket from scratch.

I kindly disagree with this statement.

Websockets are great for working with events that don't use the request / response pattern, e.g. pushing your turn results as one event that is sent to the server. Or pushing this updated state to all the other clients.

If you want to force a req/res pattern onto websockets, there are a few downsites doing so:

These are concerns that you don't have, when doing HTTP requests, as they are already solved by the nature of HTTP requests themself.

@CrsiX I the main reason for avoiding REST APIs is to avoid polling. Is there any reason of making things complicated by keeping REST API alongside WebSocket? I see no point in that.

So I'm totally onboard with you regarding everything that is related to any kind of polling. But things like a register endpoint should be handled the easiest way - with plain old http requests.

Normally, I don't like the way how microsoft designs its systems, but this article also makes some additional valid points.


Also kindly use camelCase, they are more readable.

I also disagree with this statement, but as I don't have a strong preference here, fine 4 me.

touhidurrr commented 1 year ago

I agree with that, but there should be ways to send popup messages from the server. Maybe we can implement types for that.

Could you name me a use case for that feature?

This would allow a greater server - user relationship. Things like server events would be possible.

touhidurrr commented 1 year ago

But things like a register endpoint should be handled the easiest way - with plain old http requests.

@myOmikron I wonder how are you thinking of the auth mechanism right now? Since auth has been of greater importance now, the following feature has been agreed upon: If a user is registered/has a password, his turn cannot be done by anyone else.

Since we are sending/receiving game updates via WebSocket (my plans + seems to be in your code also), the server would need to recognize the client anyways.

That's why I thought auth should be done via WebSocket.

Are you planning to send Authorization header via /api/v2/ws and upgrade that connection to ws and implement auth that way?

Normally in other implementations, I saw auth is normally done via WebSocket also. Example: Discord.

CrsiX commented 1 year ago

... his turn cannot be done by anyone else.

Strictly speaking, this is currently false, since a client uploads a whole game state, and there's no mechanism ("anti-cheat") that prevents a client from playing the turn of someone else or modifying the state in any way, too. Am I right about that? But of course, ideally this is the case.

That's why I thought auth should be done via WebSocket.

Well, authentication is only necessary during WebSocket creation, because afterwards the established connection is used as authenticated communication channel. We're using a simple session cookie at the moment. The flow is:

  1. POST /api/v2/auth/login to get the session cookie (in response header Set-Cookie) If this fails, you could try again by registering at POST /api/v2/accounts/register first, then repeat login
  2. GET /api/v2/accounts/me to get the user's identifier (used later, but not required for the WebSocket workflow), authenticated via the Cookie header
  3. GET /api/v2/ws creates a new WebSocket connection (101 Switching Protocols), authenticated via the Cookie header
CrsiX commented 1 year ago

This would allow a greater server - user relationship. Things like server events would be possible.

Of course. But the server events should be well defined. I don't like free strings in this case. I can think of various things you might have in mind as well.

Any of those get their own message type. And then, they can easily be translated into different languages, to name one advantage. Of course, the new incoming text message can't be translated, but the information that there's a new message can!

sockybob commented 1 year ago

Is the server that you're working on could hypothetically support simultaneous turns aka real-time multiplayer?

myOmikron commented 1 year ago

@sockybob What exactly do you mean by "simultaneous turns"?

sockybob commented 1 year ago

@sockybob What exactly do you mean by "simultaneous turns"?

Like where all players are doing their turn at the same time instead of waiting for each other

CrsiX commented 1 year ago

Like where all players are doing their turn at the same time instead of waiting for each other

Just to get confirm I get you right: there's a planning phase where all non-AI players plan the moves of their turn, followed by a action phase in which the game executes the moves in some defined order. What happens if there's a collision (say, player A and player B moved on the same field in the same turn)? Has the second player the chance to change his plans? I'm not sure how original Civ V handles this, therefore my question.

To answer it: we could build a server that handles this, it would require patch files (the plan what should be done) instead of whole game files. However, the client game doesn't support it AFAIK, so that's not a question for me. But I would be open to help implement the relevant networking. Heavily refactoring the client is out of scope for me at the moment.

sockybob commented 1 year ago

In Civ 5 every player action is synced immediately. If two players move on the same tile, the one that moved first wins (it can also be affected by ping etc). My concern was if the new server will be able to support something like that, in case there will be plans to implement simultaneous turns on client side in the future. It is very different from the current unciv multiplayer implementation which is syncing the whole game state once player finishes their turn vs sending many small updates with low latency.

CrsiX commented 1 year ago

In Civ 5 every player action is synced immediately.

Thanks for the clarification. Since it's a so completely different game mechanic, I would say no at the moment. However, it might be a good idea for the future. However, it would require handling all game logic on the server as well, which is currently completely out of scope. Keep it in mind and maybe open an issue in the future?

touhidurrr commented 1 year ago

Strictly speaking, this is currently false, since a client uploads a whole game state, and there's no mechanism ("anti-cheat") that prevents a client from playing the turn of someone else or modifying the state in any way, too. Am I right about that? But of course, ideally this is the case.

Sorry, but I think you didn't look into the pull requests that I game you in your repo's discussion. This had already been implemented with Basic auth implementation.

POST /api/v2/auth/login to get the session cookie (in response header Set-Cookie) If this fails, you could try again by registering at POST /api/v2/accounts/register first, then repeat login GET /api/v2/accounts/me to get the user's identifier (used later, but not required for the WebSocket workflow), authenticated via the Cookie header GET /api/v2/ws creates a new WebSocket connection (101 Switching Protocols), authenticated via the Cookie header

Is there any need to make it so complicated by implementing cookies? There is several problems with this upgrade approach. One shouldn't necessarily have to register to connect to websocket. unregistered players should also be able to connect with their existing user id. So, the approach should be, connect to WebSocket and auth if you want.

I think @yairm210 and @GGGuenni can tell better about this issue cause they will be the ones to implement these features on clientside. So, what do you guys think. Its there any need to keep REST APIs or we completely switch to WebSockets for everything.

touhidurrr commented 1 year ago

Any of those get their own message type. And then, they can easily be translated into different languages, to name one advantage. Of course, the new incoming text message can't be translated, but the information that there's a new message can!

Yeah, that is a good idea. I just wanted a way to send normal popups also. It can be called a ServerBroadcast or whatever but should be there.

touhidurrr commented 1 year ago

Also @CrsiX @myOmikron, I would want you guys to at least maintain a text file about what are you currently doing on WebSocket. So, that simultaneous implementation is possible. I was also in the process of rewriting UncivServer.xyz but couldn't get enough time because of exams, which has been completed today.

CrsiX commented 1 year ago

One shouldn't necessarily have to register to connect to websocket. unregistered players should also be able to connect with their existing user id. So, the approach should be, connect to WebSocket and auth if you want.

I kindly disagree. If you're not yet registered, then just register yourself. Then, authenticate at the login endpoint and you can participate in the game via WebSocket. "auth if you want" is dangerous. Auth is always added.

On first multiplayer usage, the user is required to set a username and password. But if the user doesn't want to set a password, we can just generate a strong one on the client. If the user wants to use multiple devices later, he needs to set a password and share it. Different user flows are totally possible here, too. I'm open for that.

So, to name just two advantages of accounts (= auth): you can ban malicious users and you can find friends easier (by username, not by ugly UUID). Without auth, the whole API would not work.


Is there any need to make it so complicated by implementing cookies? There is several problems with this upgrade approach.

What do you mean by upgrade approach? But well, since this is a game, I'm open to change that. The client does auto-login anyways.


I think @yairm210 and @GGGuenni can tell better about this issue cause they will be the ones to implement these features on clientside. So, what do you guys think. Its there any need to keep REST APIs or we completely switch to WebSockets for everything.

As stated by @myOmikron, the API will use both HTTP and WebSockets. We will handle implementing the network stack (he on the Rust server, me on the client side), so that the others don't have to do that.

touhidurrr commented 1 year ago

Also @CrsiX @myOmikron, I would want you guys to at least maintain a text file about what are you currently doing on WebSocket. So, that simultaneous implementation is possible. I was also in the process of rewriting UncivServer.xyz but couldn't get enough time because of exams, which has been completed today.

And what about this @CrsiX @myOmikron? I think you guys should add api / websocket docs at the same time of writing apis / websockets. that way it would be easier to give feedback per feature and also other can keep implementing it one by one.

Also a workflow of how the features would work would be great also.

CrsiX commented 1 year ago

I think you guys should add api / websocket docs at the same time of writing apis / websockets.

Our REST API is 100% documented. The link posted above is our hosted service, which we always keep updated: https://runciv.hopfenspace.org/docs/ For a diff of our progress, I specifically created our discussion. Note that may somehow run out of sync with the implementation, so when in doubt, git log tells the truth.

The WebSocket API isn't documented using something like OpenAPI, since it doesn't exist, as I already mentioned. Since @myOmikron was faster implementing the server, look at the server code for the structure of the messages here. Short summary: it's JSON sent via Text frames, with the structure as described here. The currently available types are found in the server and client code as well:

    InvalidMessage("invalidMessage"),
    FinishedTurn("finishedTurn"),
    UpdateGameData("updateGameData"),
    ClientDisconnected("clientDisconnected"),
    ClientReconnected("clientReconnected"),
    IncomingChatMessage("incomingChatMessage");

Also a workflow of how the features would work would be great also.

I've tried to lay out the authentication flow using session cookies above. Of course, this could change when we drop session cookies and use plain HTTP basic auth instead. We haven't decided upon that matter yet, haven't we?

myOmikron commented 1 year ago

And what about this @CrsiX @myOmikron? I think you guys should add api / websocket docs at the same time of writing apis / websockets. that way it would be easier to give feedback per feature and also other can keep implementing it one by one.

Also a workflow of how the features would work would be great also.

Like @CrsiX mentioned, https://runciv.hopfenspace.org/docs/ is the place to find the REST API. I'll improve the description of the endpoints frequently.

For the websocket, I'll start writing docs :)

CrsiX commented 1 year ago

Do you have policies regarding additional dependencies? I've added the ktor-client library and some of their helpers (e.g. logging, serialization, encoding, websockets) to implement the networking functionality, since the currently used library didn't provide required functionality. It doesn't significantly increase the size of the APK (around 1.5 MiB extra for a debug build). Some of those dependencies might be moved to a "dev dependency" section if it exists.

touhidurrr commented 1 year ago

Do you have policies regarding additional dependencies?

Unciv's general policy has been so far is to keep the apk size small. If the production apk size doesn't increase much then it is considered ok.

CrsiX commented 1 year ago

In the long-term, I would like to change some setting formats. At the moment, the GameSettings.json contains a multiplayer section, GameSettingsMultiplayer, which stores various multiplayer-related settings. The section copied below (source) is one part of it.

class GameSettingsMultiplayer {
    var userId = ""
    var passwords = mutableMapOf<String, String>()
    var userName: String = ""
    var server = Constants.uncivXyzServer
    var friendList: MutableList<FriendList.Friend> = mutableListOf()

    // The other values will not be touched
    var turnCheckerEnabled = true
    var turnCheckerPersistentNotificationEnabled = true

I would like to change them to make them compatible to multiple servers with multiple API implementations. That means: to deprecate the fields like userId, userName and passwords and combine them all together. For every server, the configuration should be saved separately. Therefore, what I have in mind is a list of settings of the following format:

data class MultiplayerServerSettings {
    var userId: String  // mandatory; allows support of multiple accounts for different servers
    var username: String?  // might not be supported by a server
    var password: String?  // might not be supported by a server
    var friendList: MutableList<FriendList.Friend> = mutableListOf()  // friends and lists are server-specific functionality, e.g. in APIv2 they are managed by the server instead
}

class GameSettingsMultiplayer {
    // DEPRECATED var userId = ""
    // DEPRECATED var passwords = mutableMapOf<String, String>()
    // DEPRECATED var userName: String = ""
    var server = Constants.uncivXyzServer  // this is the *currently selected* server
    // DEPRECATED var friendList: MutableList<FriendList.Friend> = mutableListOf()

    // New
    var servers: MutableMap<String, MultiplayerServerSettings> = mutableMapOf()  // this is a mapping of baseUrl to details MultiplayerServerSettings, e.g. 'https://uncivserver.xyz': { ... }

    // The other values will not be touched
    var turnCheckerEnabled = true
    var turnCheckerPersistentNotificationEnabled = true

This is also easily serializable using the existing JSON infrastructure.

Of course, this requires some additional migration steps.

I didn't use a list of objects and include the server base URL in that objects, since I guess having multiple accounts for the same server at the same time is no use case. If a user wants to play as foo@server as well as bar@server from the same device, it requires the extra step of changing the settings (and logging in again).

I might open another issue on, I guess. Do you have any further thoughts on that matter? @yairm210 @SomeTroglodyte as well :)

SomeTroglodyte commented 1 year ago

You need only deprecate one field? Simply several instances of GameSettingsMultiplayer as is? But I'd rather have a disable for multiplayer. As is, our docs do not really cover the fact that Unciv will disclose your IP to a third party with no chance to opt out before it happens, or that the only way to opt out is to enter localhost or something invalid into the server field.

CrsiX commented 1 year ago

Simply several instances of GameSettingsMultiplayer as is?

Hmm, that would be another option. However, then the GameSettings would need a list of those GameSettingMultiplayer (or a map with the server base URL being the key), so that would be another type of migration.

As is, our docs do not really cover the fact that Unciv will disclose your IP to a third party with no chance to opt out before it happens, or that the only way to opt out is to enter localhost or something invalid into the server field.

Then something like

var multiplayer: MutableMap<String, GameSettingsMultiplayer>
var enableMultiplayer: Boolean

instead (reference)?


At the moment, I'm building the PoC for APIv2, so I can integrate something like a pop-up on first use "do you want to use $serverUrl, this might reveal your IP address"? Of course, we need to strictly enforce that no connection is made before the popup is accepted, then. And what about the mods and their listings, they also require network connectivity and might disclose your IP address to the server(s); does this also require such a pop-up approval? Might it be a valid path to just ask on the very first game use "do you want any network connectivity"? But on the other hand, if a user doesn't want multiplayer connectivity for a game, he might just not grant the networking permission for that app (at least on Android), provided the necessary knowledge to do so. But that aren't questions for this issue :)

SomeTroglodyte commented 1 year ago

I was just throwing ideas into the ring. That MutableMap<String, GameSettingsMultiplayer> was what I meant, though when the field name is identical it'll be hard (but not impossibe) to migrate older settings files. Much easier inventing a new name. Also, Map<String should serialize without problems, but I'd check what is actually written before deciding that collection variant is best. And a need for an extra flag may not exist if we assign that meaning to the state "map is empty"...

As for other 'net traffic, even me the paranoid thinks mods are not an acute problem, though a little thought on clarification won't hurt. Should be obvious to the user they come from outside, so a click on "mods" is pretty much enough consent. The Discord thing, too, is not really documented in-depth, but that's borderline OK too - after all, it's not Unciv phoning home in this case, it just prods a local Discord desktop client to do so if one is running - and by installing Discord you pretty much consented to a loss of privacy of inscrutable extent - I mean what other Software has the chuzpah to install a protocol handler, ugh that's not much better than a rootkit........ Pardon me.

But - my intent butting in is more than fulfilled, brains are working, have fun bye till next time :+1: :smile: