Initial Event Bus implementation & Event-based multiplayer updates #6826

Azzurite · 2022-05-15T22:30:33Z

This is still just another PR in preparation for the feature I actually want to add :D

In total, this is basically just a refactoring, there should be basically no observable changes, except where otherwise noted.

A central event bus is added. It is based on standard kotlin classes that extend Event. This imo makes it very nice to work with since you can add any data with the event you like, immediately with no boilerplate code. You also can see all events just by telling your IDE to list all implementing classes. In addition, it was requested that it not be asynchronous/threaded for better debuggability, so you are only supposed to use it on the main render thread. This thread was chosen because when you listen for events, you mostly want to do something with the UI immediately based on the event. Otherwise I hope it's pretty simple & self-explanatory.

(This "non-multithreaded requirement" also means I can't show off very nice coroutine features :D the whole EventBus file could be shrinked to 10 lines with added asynchronous support, at the expense of being able to step through from send->receive in the debugger)

The other part of this PR is a major refactor of the multiplayer logic to be centralized in their own classes in com.unciv.logic.multiplayer instead of being strewn around various Screens. And instead of using the "throw away everything and rebuild based on the newest data"-approach that has mostly been used so far, it instead now uses events from the event bus to listen for changes and only update the specific components that need updating.

(This is not really noticeable in the MultiplayerScreen because not much is happening there, but when you extend this approach to more performance-critical parts of the game, this will get us major performance benefits. We currently "if something changed, throw away things and rebuild based on the newest data" a lot of things when a WorldScreen.update happens - if all those things would not need a "has updated" check and only update the small parts that actually change, the whole game would be lightning-fast)

More possibly interesting comments as code comments down below:

Azzurite · 2022-05-15T22:41:22Z

android/build.gradle.kts

@@ -127,4 +129,5 @@ dependencies {
    //   run `./gradlew build --scan` to see details
    implementation("androidx.core:core-ktx:1.6.0")
    implementation("androidx.work:work-runtime-ktx:2.6.0")
+    coreLibraryDesugaring("com.android.tools:desugar_jdk_libs:1.1.5")


This increases the apk size by 3.431kB, but gives us almost all newer Java APIs even on old devices. I mainly included this for java.time (kotlin.time seems to basically only be for performance testing/debugging).

I can remove this if this size increase is not acceptable, it does increase the apk size by roughly 30% from around 11.5 to around 15 MB, but I would rather not :D me likey newey featurey

I remember arguing over desugaring once. When Unciv wanted to bump its minimum android sdk version. But it was shot down.

Azzurite · 2022-05-15T22:43:49Z

core/src/com/unciv/logic/multiplayer/OnlineMultiplayer.kt

+                delay(getRefreshInterval().toMillis())
+
+                // TODO will be used later
+                // requestUpdate()


This'll be used when my ingame multiplayer status display is done.

Azzurite · 2022-05-15T22:45:32Z

core/src/com/unciv/logic/multiplayer/OnlineMultiplayer.kt

+fun GameInfoPreview.isUsersTurn() = getCivilization(currentPlayer).playerId == UncivGame.Current.settings.userId
+fun GameInfo.isUsersTurn() = getCivilization(currentPlayer).playerId == UncivGame.Current.settings.userId


I was thinking of putting these into the actual classes, but they are solely multiplayer functions. I think it's good for them to be here.

Azzurite · 2022-05-15T22:46:49Z

core/src/com/unciv/logic/multiplayer/OnlineMultiplayerGame.kt

+
+
+/** @see getUpdateThrottleInterval */
+private const val DROPBOX_THROTTLE_INTERVAL = 8L


This means that when a user spams the "Refresh list" button, they will only get an update every 8 seconds of doing that.

I think this is more than acceptable with our rate limiting issues there.

Azzurite · 2022-05-15T22:52:08Z

core/src/com/unciv/ui/multiplayer/MultiplayerScreen.kt

-            }
+private class GameList(
+    onSelected: (String) -> Unit
+) : VerticalGroup() {


I know Table are being used mostly, and I now understand why. But in this case, a VerticalGroup makes much more sense, because it is able to remove/add and correctly layout actors all the time. With a table, removing/editing a cell is possible, but annoying, it doesn't happen automatically. Also it has the nice benefit of allowing us to do children.sort() to have the games always listed in alphabetical order.

Azzurite · 2022-05-15T22:52:36Z

core/src/com/unciv/ui/multiplayer/MultiplayerScreen.kt

+    val turnIndicator = createIndicator("OtherIcons/ExclamationMark")
+    val errorIndicator = createIndicator("StatIcons/Malcontent")
+    val refreshIndicator = createIndicator("EmojiIcons/Turn")
+    val statusIndicators = HorizontalGroup()


Same with the VerticalGroup comment above. Much better remove/add handling.

Azzurite · 2022-05-15T22:53:30Z

core/src/com/unciv/ui/multiplayer/MultiplayerScreen.kt


+    private fun createIndicator(imagePath: String): Actor {
+        val image = ImageGetter.getImage(imagePath)
+        image.setSizeForced(50f)


I can imagine this was one of the main problems when trying to use anything other than Table. This makes it nice and dandy though. (See later comment)

Azzurite · 2022-05-15T22:56:09Z

core/src/com/unciv/ui/utils/ExtensionFunctions.kt

+ * Either [Image] or [com.badlogic.gdx.scenes.scene2d.utils.Drawable] is badly written, as the [Image.drawable] always has its texture size as min size,
+ * and [Image] pref size is always equal to its [Image.drawable]'s min size.


This is the crux of the issue with Images & putting them in anything else than Tables. All the WidgetGroups like the horizontal/vertical one use the child actor's prefWidth/Height to set the size. Because of the design flaw outlined in this method comment, doing Image.setSize does absolutely nothing since the Drawables minWidth/Height is used as prefWidth/Height, and the actual width/height properties are completely ignored for prefWidth/Height... utter stupidity.

Table actually ignores the prefWidth/Height of any actors within cells if you do Cell.size(..), which is why that works so nicely. But there's no reason why the other WidgetGroups shouldn't be used, they do have their place & time.

Azzurite · 2022-05-15T22:57:49Z

core/src/com/unciv/ui/worldscreen/WorldScreen.kt

-                } catch (ex: FileStorageRateLimitReached) {
-                    postCrashHandlingRunnable {
-                        val cantUploadNewGamePopup = Popup(this@WorldScreen)
-                        cantUploadNewGamePopup.addGoodSizedLabel("Server limit reached! Please wait for [${ex.limitRemainingSeconds}] seconds").row()
-                        cantUploadNewGamePopup.addCloseButton()
-                        cantUploadNewGamePopup.open()
-                    }


In addition to reduced code duplication, this is also a bugfix: in the other catch (ex: Exception) branch is where isPlayersTurn and shouldUpdate are set to true, but not in this branch.

Azzurite · 2022-05-15T23:07:56Z

Also,

Unciv/core/src/com/unciv/ui/worldscreen/WorldScreen.kt

Line 356 in f6a4d5f

private suspend fun loadLatestMultiplayerState() {

is not adjusted on purpose, since that will also be changed with the ingame multiplayer status display.

Azzurite · 2022-05-16T00:17:04Z

core/src/com/unciv/logic/multiplayer/OnlineMultiplayer.kt

+            // Game is so old that a preview could not be found on dropbox lets try the real gameInfo instead
+            gamePreview = OnlineMultiplayerGameSaver().tryDownloadGame(gameId).asPreview()
+            fileHandle = GameSaver.saveGame(gamePreview, saveFileName)


This was added on 2021-12-13, can this be removed? How long until old saves on Dropbox get deleted?

One of the open problems around here: No auto cleanup on "Unciv's" Dropbox.

I manually remove a lot of them every time the dropbox fills up. Currently, all save games up to (but excluding) 2021-10-31 are removed from the dropbox

Ah yes there was some python script and idea to make it a github action...

itanasi · 2022-05-17T05:22:13Z

This would have been a nice way to track which cities need to re-evaluate in my PR #6650

Azzurite · 2022-05-17T13:40:39Z

This would have been a nice way to track which cities need to re-evaluate

And this is exactly why I want to add this. It's so incredibly useful.

…ates event-based instead of throwaway+rebuild

Azzurite · 2022-05-17T15:49:52Z

I rebased this branch on master - no conflicts

SomeTroglodyte · 2022-05-17T19:40:25Z

Six pages essentially a monologue so far... OK I'll read the beast.

open class Event

OK, this is key. In fact, I must find an excuse to disparage a 16-char oneliner.... ... .. . OK: Probability for Name conflicts with std libraries too high? Auto-Imports of Group, Array, Timer etc all run into problems that maybe the wrong one is chosen... Extra namespace "EventBus.Event" (need for qualifier can be import-tweaked away if needed - per-file)? "UncivEvent" (too long)?. "Efent" (:rofl:)? We'll probably just have to be careful. Oh, and please end all new files with a LF for prettier github rendering.

java.lang.ref.WeakReference

There's no kotlin equivalent? Even if it turns out to be just a typealias...

private fun <T: Event> receive

This does not actually receive, right? It's more a listen to or register handler or somesuch. Oh, private, the meat is elsewhere. never mind - yes, the dox of the actual meat make it clear. (Besides, isn't the 'recieve' typo obligatory by now?)

class MultiplayerGameUpdateErrored(...) : Event()

'Errored' - uh, okay, can't think of a better name either. And - why is Event an open class instead of an interface anyway? Would save Gigabytes of () all over the place...

At the UI stuff I started to skip stuff, trust will do.
I like the EventBus design and can't wait to see all our Stats updates riding it.
So...

Azzurite · 2022-05-18T05:21:32Z

Name conflicts

I never understood this argument ever. The type system will tell you if you used the wrong type, since as soon as you try to interface with existing code, it'll tell you Type mismatch and you'll figure it out from there.

There's no kotlin equivalent?

I pressed Ctrl+Space, and that was the only one showing up. So I guess not.

isn't the 'recieve' typo obligatory

You should know by now that for me, no typo is obligatory :D I even use "well" instead of "good" when being asked "How are you?"

why is Event an open class instead of an interface anyway?

Because of a very good reason: brainfart :D I first did the "standard" stuff with members like EventType and EventData but then deleted all that and made it class-based and didn't notice that the open class does now not make sense.

Azzurite · 2022-05-18T05:33:09Z

'Errored' - uh, okay, can't think of a better name either

Failed?

SomeTroglodyte · 2022-05-18T09:42:42Z

when being asked "How are you?"

Good ™️ for you. When asked that I always faint. 👽 👨‍⚕️

Name conflicts

We had precedences. Java math where using kotlin's consistently would be better, and one with just a tiny little more weight I forget? And for some Gdx classes I always think "does everybody on the project know not to pick the first offer"? Not important.

There's no kotlin equivalent?

That one had a link. Duckduck instead of Studio. May be out of context, didn't really dive in.

Azzurite · 2022-05-18T13:45:26Z

That link is to https://kotlinlang.org/docs/native-overview.html

SomeTroglodyte · 2022-05-18T16:30:56Z

Ah, out of context. Got it. 🤦‍♀️

Azzurite · 2022-05-18T17:47:16Z

After adding the possibility to filter events, I forgot to add that to the EventReceiver... so the filters got garbage collected. I fixed this with the most recent commit.

Azzurite · 2022-05-20T15:42:41Z

Found another issue - the setSizeForced approach does not work. I missed that all our drawables are pooled. So we can't set the size of one without affecting all the others. I'm working on a fix.

SomeTroglodyte · 2022-05-20T15:44:54Z

setSizeForced

Not really part of Event Bus, right? .... Split it off?

# Conflicts: # core/src/com/unciv/ui/multiplayer/MultiplayerScreen.kt

Azzurite · 2022-05-21T21:47:49Z

@SomeTroglodyte I resolved the conflict, it was just an unused method removal from the warnings cleanup.

Azzurite · 2022-05-21T21:51:42Z

@yairm210 did you want to look at this still? I implemented your request of the event bus staying mostly async-free, i.e. it's only ran from a single thread. Unfortunately if you already have code that is asynchronous you can't get around some debug issues where you can't quite step through everything, because you already are starting other threads.

Otherwise I don't think anyone else will be looking at this soon so I'd merge it :D

SomeTroglodyte · 2022-05-21T21:54:57Z

I see the merge was easy, yes. I reaffirm I think this will be a boon - we'll just have to get people to actually use it. Recalc stats, reassign population,...

Merge with just our two votes? Or request a review from someone specific first?

Azzurite · 2022-05-21T22:05:21Z

Yeah I think I'll merge, I want that other stuff that is based on this in here faster :) if @yairm210 has any concerns we can still make changes after :)

yairm210 · 2022-05-21T22:10:25Z

I think that once we start implementing things using this, we should add tests to validate previous behavior, and ensure that that behavior remains after refactor

GGGuenni · 2022-05-22T08:07:25Z

This has 11 Call requires API level 26 (current min is 21) errors in OnlineMultiplayer.kt. All related to java.time. Doesn't this result in unciv being unable to run on Lollipop, Marshmallow, and Nougat?

but gives us almost all newer Java APIs even on old devices. I mainly included this for java.time

Looks like I didn't read properly ¯\_(ツ)_/¯ ~~So is there a way to suppress these without suppressing them all manually?~~
Good old invalidate caches fixed it again.

touhidurrr · 2022-10-11T07:22:53Z

Just testing Github's email capabilities. Also what is this event bus for? Will it trigger an event for every change in GameInfo so that we can handle those events or something like that? I mean what does it cover???

…

On Sun, May 22, 2022, 04:10 Yair Morgenstern ***@***.***> wrote: I think that once we start implementing things using this, we should add tests to validate previous behavior, and ensure that that behavior remains after refactor — Reply to this email directly, view it on GitHub <#6826 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALDVLCX5W6MY77OGMYIOOC3VLFNN7ANCNFSM5V7XJETQ> . You are receiving this because you commented.Message ID: ***@***.***>

touhidurrr · 2022-10-11T09:03:05Z

Just testing Githubs email capabilities. Also what is this event bus for? Will it trigger an event for every change inside a game so that we can handle the even and something like that?

itanasi · 2022-10-11T14:49:41Z

Whatever you define as an event

Azzurite force-pushed the parallel-multiplayer branch from 078a5a6 to f6a4d5f Compare May 15, 2022 22:49

Azzurite commented May 15, 2022

View reviewed changes

Azzurite commented May 16, 2022

View reviewed changes

Azzurite mentioned this pull request May 17, 2022

Refactor: change GameSaver from singleton to single instance pattern & move autosave logic into GameSaver #6846

Merged

Azzurite changed the base branch from master to SomeTroglodyte-patch-1 May 17, 2022 15:48

Azzurite changed the base branch from SomeTroglodyte-patch-1 to master May 17, 2022 15:48

Azzurite added 2 commits May 17, 2022 17:49

Add EventBus

47f9e53

Refactor: Extract multiplayer logic into their own classes & make upd…

56323f3

…ates event-based instead of throwaway+rebuild

Azzurite force-pushed the parallel-multiplayer branch from f6a4d5f to 56323f3 Compare May 17, 2022 15:49

Azzurite marked this pull request as ready for review May 17, 2022 15:49

SomeTroglodyte approved these changes May 17, 2022

View reviewed changes

Cleanup

f61918c

Refactor: FAILED!

6c25576

Fix event filters being garbage collected

f75ff53

Azzurite marked this pull request as draft May 20, 2022 16:40

Merge remote-tracking branch 'origin/master' into parallel-multiplayer

46c69f0

# Conflicts: # core/src/com/unciv/ui/multiplayer/MultiplayerScreen.kt

Azzurite marked this pull request as ready for review May 21, 2022 21:44

Azzurite merged commit 244f947 into yairm210:master May 21, 2022

Azzurite deleted the parallel-multiplayer branch May 21, 2022 22:05

Azzurite mentioned this pull request May 22, 2022

Fix multiplayer turn checker potentially stopping turn checks for everyone that plays that game if a file was not found remotely #6901

Merged

		fun GameInfoPreview.isUsersTurn() = getCivilization(currentPlayer).playerId == UncivGame.Current.settings.userId
		fun GameInfo.isUsersTurn() = getCivilization(currentPlayer).playerId == UncivGame.Current.settings.userId



		/** @see getUpdateThrottleInterval */
		private const val DROPBOX_THROTTLE_INTERVAL = 8L

		* Either [Image] or [com.badlogic.gdx.scenes.scene2d.utils.Drawable] is badly written, as the [Image.drawable] always has its texture size as min size,
		* and [Image] pref size is always equal to its [Image.drawable]'s min size.

Initial Event Bus implementation & Event-based multiplayer updates #6826

Initial Event Bus implementation & Event-based multiplayer updates #6826

Conversation

Azzurite commented May 15, 2022 • edited Loading

Azzurite May 15, 2022 • edited Loading

Choose a reason for hiding this comment

touhidurrr May 20, 2022 • 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

Azzurite May 15, 2022 • edited Loading

Choose a reason for hiding this comment

Azzurite May 15, 2022 • edited Loading

Choose a reason for hiding this comment

Azzurite commented May 15, 2022 • 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

itanasi commented May 17, 2022

Azzurite commented May 17, 2022

Azzurite commented May 17, 2022

SomeTroglodyte commented May 17, 2022

Azzurite commented May 18, 2022 • edited Loading

Azzurite commented May 18, 2022

SomeTroglodyte commented May 18, 2022

Azzurite commented May 18, 2022

SomeTroglodyte commented May 18, 2022

Azzurite commented May 18, 2022 • edited Loading

Azzurite commented May 20, 2022

SomeTroglodyte commented May 20, 2022

Azzurite commented May 21, 2022

Azzurite commented May 21, 2022

SomeTroglodyte commented May 21, 2022

Azzurite commented May 21, 2022

yairm210 commented May 21, 2022

GGGuenni commented May 22, 2022 • edited Loading

touhidurrr commented Oct 11, 2022 via email

touhidurrr commented Oct 11, 2022 via email

itanasi commented Oct 11, 2022

Azzurite commented May 15, 2022 •

edited

Loading

Azzurite May 15, 2022 •

edited

Loading

touhidurrr May 20, 2022 •

edited

Loading

Azzurite May 15, 2022 •

edited

Loading

Azzurite May 15, 2022 •

edited

Loading

Azzurite commented May 15, 2022 •

edited

Loading

Azzurite commented May 18, 2022 •

edited

Loading

Azzurite commented May 18, 2022 •

edited

Loading

GGGuenni commented May 22, 2022 •

edited

Loading