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

Abstract out game-specific logic #3223

Merged
merged 2 commits into from
Jan 10, 2021

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Dec 5, 2020

Motivation

The currently pledged release date for KSP 2 is sometime in 2022. Shortly after that, we would like to support it in CKAN, see #2863. But currently the code is shot through with KSP1-specific logic that is unlikely to work in both games, and we have no framework for adding support for other games.

graph

Changes

Now we take a first step towards cleanly encapsulating game-specific logic such that we will be able to swap it out for other games in the future. This got a bit big, sorry about that.

Code summary:

  • A new IGame interface encapsulates all the logic we need to support a game in the abstract.
  • All KSP1-specific logic (that I could find) is migrated into a new KerbalSpaceProgram class that implements this interface
  • If we want to add a new game, we would write a new class for it that implements IGame
  • Most occurrences of "KSP" or "Ksp" in class and function names are changed to "Game" or "GameInstance" or "CKAN"
  • Each GameInstance (f.k.a. KSP) holds a reference to the IGame object associated with the game of which it represents an instance
  • GameInstanceManager (f.k.a. KSPManager) holds a list of known games, which are used to probe folders and determine which game to use
  • Various places that require game-specific logic now take an IGame parameter or retrieve it from GameInstance.game if they already have a game instance
  • A new JsonPropertyNamesChangedConverter class allows us to change property names in JSON files while still migrating forward the old data

Visible changes:

  • Most mentions of "KSP" in the UIs now say "Game" or "Game instance"
  • Where the UI is actually referring to the specific game (like in the title bar), it still says "KSP", but it comes from IGame.ShortName instead of being hard coded, so future games will be shown the same way
  • The lists of game instances now have a "Game" column which at the moment can only ever say "KSP" but can support more games in the future
  • Everything else should still work the same

Side fix:

  • ckan repo default uri previously ignored its uri parameter and just set the default repo to the default URL (and then lied about it). As of this PR, it sets the default repo to the URL from the parameter, as expected.

Limitations:

  • Wherever Netkan needs an IGame, it just instantiates a new KerbalSpaceProgram(), since it doesn't have a game instance to check. We will need to add something more sophisticated here in the future, maybe a --game parameter defaulting to KSP. This will have to be figured out in a future PR.
  • Similarly, Tests also use new KerbalSpaceProgram() freely to bootstrap old code into the new reality. If and when we add more games, writing more Tests would make sense.
  • Similarly, the fake-instance code also assumes new KerbalSpaceProgram(). If and when we add another game, we would want to prompt the user to choose a game here.

Future ideas:

  • I'm assuming a new game would get its own separate NetKAN repo and CKAN-meta repo, and we'd duplicate the NetKAN-Infra services per each game

@HebaruSan HebaruSan added Enhancement GUI Issues affecting the interactive GUI Cmdline Issues affecting the command line Core (ckan.dll) Issues affecting the core part of CKAN ConsoleUI Issues affecting the interactive console UI labels Dec 5, 2020
@techman83
Copy link
Member

This is beyond my C# to review properly, but I wholeheartedly support the concept! Making it work across different games was always something on the future road map.

The indexing infra doesn't necessarily require duplication, but we can certainly map out how we might achieve supporting multiple games.

@HebaruSan
Copy link
Member Author

@techman83 yeah. I've been considering that maybe .netkans could have a new optional propety indicating which game they're for:

    "game": "KSP",

This could be used to drive various game-specific validation checks, as well as where the Indexer commits them (CKAN-meta vs CKAN-meta-ksp2). Does that strike you as a good idea or bad?

@techman83
Copy link
Member

@techman83 yeah. I've been considering that maybe .netkans could have a new optional propety indicating which game they're for:

    "game": "KSP",

This could be used to drive various game-specific validation checks, as well as where the Indexer commits them (CKAN-meta vs CKAN-meta-ksp2). Does that strike you as a good idea or bad?

My only concern and something we don't have to come up with for this, would be schemas. A NetKAN is a subset of the CKAN schema. Would we have a schema per game, ensure our schema also is game independent, things like that?

Actually after writing that, there is probably a lot that can be re-used. As a separate exercise we could go through the schema and figure out what is/isn't ksp specific.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slowly digging through the changes. Fortunately most of them are just class renames, but still takes some time to filter out all of them.
Two notes to start with...

Core/Registry/Registry.cs Outdated Show resolved Hide resolved
Core/Games/KerbalSpaceProgram.cs Outdated Show resolved Hide resolved
@HebaruSan HebaruSan force-pushed the feature/multi-game branch 2 times, most recently from 657a6ff to 3123ee6 Compare December 15, 2020 05:10
@DasSkelett
Copy link
Member

I just pushed a few localization fixes, also tried my luck with Chinese, with the assumption that "game" is translated to "游戏". 🤞

GUI/Main/MainRepo.cs Outdated Show resolved Hide resolved
@DasSkelett
Copy link
Member

What's the reason for removing the build map cache from the config.json/Windows Registry?

@HebaruSan
Copy link
Member Author

HebaruSan commented Dec 23, 2020

Does it belong there? Seems like we'd be setting ourselves up to store an unreasonable amount of data from many games, none of which is truly "configuration" in any conceivable sense.

@DasSkelett
Copy link
Member

I agree that it doesn't belong into config.json, though it might have fit somewhere else.
But I took a quick look around, and it doesn't look like anything big breaks if a user has no working internet connection and the embedded build map is multiple versions behind. So we're probably fine removing it.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting close. Two more small little things.
I've tested this branch quite a bit, and it is really solid so far.

Netkan/Validators/PluginCompatibilityValidator.cs Outdated Show resolved Hide resolved
Core/GameInstanceManager.cs Outdated Show resolved Hide resolved
@DasSkelett
Copy link
Member

Pushed a small commit with minor clean-up, i.e. typo fixes and removing some unused imports, and made a field in KerbalSpaceProgram.cs readonly.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I've forced enough rebases on you. The changes work excellently, and this is a huge jump forward for multi-game support in the future. It should simplify any metadata and infra changes that will be needed.
Thank your very much!

@HebaruSan HebaruSan merged commit e144144 into KSP-CKAN:master Jan 10, 2021
@HebaruSan HebaruSan deleted the feature/multi-game branch January 10, 2021 19:50
@DasSkelett DasSkelett added the i18n Issues regarding the internationalization of CKAN and PRs that need translating label Jan 18, 2021
@HebaruSan HebaruSan mentioned this pull request Mar 5, 2021
@HebaruSan HebaruSan mentioned this pull request Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cmdline Issues affecting the command line ConsoleUI Issues affecting the interactive console UI Core (ckan.dll) Issues affecting the core part of CKAN Enhancement GUI Issues affecting the interactive GUI i18n Issues regarding the internationalization of CKAN and PRs that need translating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants