New game SDK - Githubissues

BomBardyGamer commented 1 year ago

Here's the PR for the new game SDK changes that I've made. I'll highlight some important changes below to make it easier to review.

Initialisation and entry points for game servers

The entire way stuff is initialised has been completely redone.

Previously, the GameSdkModule would be statically initialised by a module from the game server, which was okay, but very messy, especially since we definitely didn't need static initialisation, and this way of having the game server create its own module and register it as part of the system was unnecessary boilerplate that had to be present in every game server.

In the new version, the game SDK initialisation is no longer part of the module system, and is done separately, which means the game servers only have to create the server and provide the game SDK config, which they would normally have to do with the old system anyway. This is much cleaner, and means less boilerplate on the game servers.

Game removal

Game removal is now very different.

The way it used to work is that when the game was finished, it would call in to the game manager directly, which was a bit messy, but wasn't that bad when things were done with static initialisation. However, with the new system, it would mean passing the game manager in to every game instance, or doing some messy static initialisation, which was an option I did not want to take.
Providing the game manager to the games would mean every game would need to have an extra parameter that they would need to accept in and pass to the super constructor, which is extra boilerplate for the game servers again.
Having the game manager be a singleton seems like an okay option, but the game manager relies on the config, so it would mean a lateinit singleton that is initialised somewhere, which is very messy, and was not an option I even considered. It is only now, on review, that I see this option as a possibility.

In the new version, what I decided to do instead was use events. Using events means that not only do I not need to pass in the game manager to the games, but the games do not need to have any awareness of the game manager's existence at all, which means that the job that the game SDK does is completely transparent to the game servers. There are some concerns to be made over performance, but it is very unlikely that this will cause a significant performance degradation.

In addition to this, the previously duplicated logic for finishing, which was for the game servers to send all the players back to the lobby and kick them if they were not sent back, is now something the game manager handles. The games now have a cleanUp method as part of their API, to allow the games to clean up anything game-specific that the game servers have, after the players have been removed.

This way of doing things will also mean that things like requeueing games could be done entirely within the game manager, with the game only needing to indicate to the game manager that players have requested a requeue, after which the game manager can move the players to a fresh, new game, instead of kicking them. I know @ZakShearman said about making this seamless, but I think that this would likely be the better way, as recreating the game means less duplicated logic in the game, and also ensures that the game will be in the same starting state that players would expect when they requeue.

Game lifecycle

The onPlayerLogin method has been renamed to onJoin, taking a Player instead of the player login event. The solution here required that every game provide an instance for the game SDK to set the spawning instance to, but as every game does this anyway, and every game will most definitely require some sort of instance for players to be able to play the game, I don't see this as a bad thing.

There is also now an onLeave method, which is called when the disconnect event is sent. This allows the game to handle player disconnections, without knowing exactly how they work, which allows us to simulate this behaviour if desired for testing purposes. It also means that every game doesn't need to individually listen to the disconnect event.

Event nodes

The previous way we created event nodes was flawed. The issue before was that the game manager would create and provide the event node. This was an issue because all the games have an instance, and so all the games would just end up creating and using their own event node anyway, and only using the one provided from the game manager as a transparent parent, with no actual event listening on it.

This has been replaced, so that getEventNode is now abstract, and games are now expected to create and provide their own event node, which the game manager will register with the games event node, which is a parent that will allow game nodes to be easily identified. This process has been made significantly easier with the addition of the GameEventPredicates utility, which provides a predicate that does what most of the nodes did before, which is check instance events to ensure they are the same instance, and check player events to ensure they are for a player in the game instance the node is registered to, which means for most games, event node creation is now a one liner.

Stuff that still needs reviewing

GameCountUpdatedEvent

The GameCountUpdatedEvent was added to allow AgonesGameHandler to be disconnected from the game manager. However, having an event and firing it for the entire event stack just for this very small, very specific functionality seems like a bad idea. I don't really want the should allocate logic to be inside the game manager, as I'd like it to be agnostic as to whether it's running in a testing or production environment. Perhaps I could create a listener interface and pass that in to the game manager.

ZakShearman commented 1 year ago

It appears right now that games might not be cleaned up properly so it doesn't get marked as READY. Might just be a blocksumo issue.