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

fix: Multiple mod versions #758

Open
wants to merge 60 commits into
base: main
Choose a base branch
from

Conversation

Alystrasz
Copy link
Contributor

@Alystrasz Alystrasz commented Jul 28, 2024

Associated mods PR: R2Northstar/NorthstarMods#824

Problem

Whether through manual installation or through mod auto-downloading, several versions of the same mod can be installed in Northstar, which currently does not handle this:

main_status.webm

This video was recorded using a profile with two Parkour mod versions, v0.1.2 and v0.2.1: note how the client does not distinguish between the two versions (version displayed in the UI is the same for both mods), and is not capable of (de)activating any of them.

Description

New enabledmods.json format

Since the current enabledmods.json format does not allow for multiple mod versions, it has been reworked.
With this PR, if "old" file format is detected, enabledmods.json will automatically be converted to "new" format.

Old format
{
    "Northstar.Client": true,
    "Northstar.Coop": true,
    "Northstar.CustomServers": true,
    "Northstar.Custom": true,
    "MRVN Shake It": true,
    "Titan Payload": false,
    "Parkour": true
}
New format
{
    "Northstar.Client": {
        "1.19.0": true
    },
    "Northstar.Coop": {
        "0.0.0": true
    },
    "Northstar.CustomServers": {
        "1.19.0": true
    },
    "Northstar.Custom": {
        "1.19.0": false
    },
    "Parkour": {
        "0.1.2": false,
        "0.2.1": true
    },
    "MRVN Shake It": {
        "0.0.1": true
    },
    "Titan Payload": {
        "1.2.0": false
    }
}

(new enabledmods.json format also means mod managers will have to adapt)

Squirrel-exposed functions

  • Functions were reworked to take mod version into account:
    • NSIsModEnabled(string modName) is now NSIsModEnabled(string modName, string modVersion);
    • NSSetModEnabled(string modName, bool enabled) is now NSSetModEnabled(string modName, string modVersion, bool enabled);
    • NSIsModRemote(string modName) is now NSIsModRemote(string modName, string modVersion);
    • NSGetModVersionByModName(string modName) is now NSGetModVersions(string modName);
  • A lot of functions loop over the list of local mods to retrieve some mod attribute (mod description, download link etc); these functions are called one after the other in the VM, therefore making useless loops over the list of mods:
    • To fix that, we add a NSGetModsInformation method, that returns a list of mods with all associated information at once (and not only names like NSGetModNames currently does it);
    • As the information they provided is now accessible through NSGetModsInformation, the following functions were removed: NSGetModDescriptionByModName, NSGetModDownloadLinkByModName, NSGetModLoadPriority, NSIsModRequiredOnClient, NSGetModConvarsByModName

Result

multiple_versions_handling.webm

This video was recorded using a profile with two Parkour mod versions, v0.1.2 and v0.2.1: the client now differentiate the two versions (they both appear correctly on the UI), and is capable (de)activating any of them.


Closes #670.
Closes #757.

Testing (manifesto format)

As this PR modifies your enabledmods.json file, you might want to copy it somewhere safe before testing.

Test scenarios
  • Old format refers to the following configuration format: mod name => boolean value
  • New format refers to the following configuration format: mod name => version string => boolean value

Launch game:

with no enabledmods.json file at all
  • enabledmods.json should be created using new format
with an invalid enabledmods.json file (not following JSON format)
  • invalid enabledmods.json should be renamed into enabledmods.json.old
  • enabledmods.json should be created using new format
with enabledmods.json using old format
  • enabledmods.json file should be renamed into enabledmods.json.old
  • enabledmods.json should be recreated using new format
with enabledmods.json using new format
  • game should start and load mods according to enabledmods.json configuration (you can check that in the Mods menu)

TODOs
  • If old manifesto format is detected, rename file and recreate file using new format
    • Mutualise file creation code with UnloadMods method
    • Check whether I can remove GenerateModsConfigurationFile method (since mod entries are already created ~L667)
    • Rename original file if it exists
    • Convert old format to new format
  • Use new manifesto format to load mods config into the client
  • Remove all code related to old format
  • Remove new script package and update old functions instead (e.g. NSSetModEnabled(name,bool to NSSetModEnabled(name,ver,bool)
    • Mirror changes in mods

@Alystrasz Alystrasz marked this pull request as ready for review September 3, 2024 14:35
@Alystrasz
Copy link
Contributor Author

The two PRs are now ready for review, I'll gladly take any feedback.

@ASpoonPlaysGames ASpoonPlaysGames added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Sep 5, 2024
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Github is absolutely butchering this diff but I think code looks ok? Really hard to tell which code is actually new or not though.

@ASpoonPlaysGames ASpoonPlaysGames added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed needs code review Changes from PR still need to be reviewed in code labels Sep 7, 2024
@Alystrasz Alystrasz added the MAD Related to mod-auto-download label Sep 7, 2024
@Alystrasz
Copy link
Contributor Author

Note: merging #826 first should ease reviewing this PR.

@Sandwhiched
Copy link

Is any additional testing required? I should be available tomorrow at around 6:00 PM CDT

@Alystrasz
Copy link
Contributor Author

Is any additional testing required? I should be available tomorrow at around 6:00 PM CDT

Testing is necessary at all, yep!
Kind reminder that you need to install both launcher and mods PRs before testing; testing instructions are listed on each PR webpage.
(don't hesitate to ping me if you need more info!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge MAD Related to mod-auto-download needs testing Changes from the PR still need to be tested
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Handle multiple mod versions Disable MAD mods on leaving server
4 participants