Review: FATE - Traitor Soul

LuxenDM commented 2 months ago

Nexus Username

LuxenDM

New features

FATE has 4 distinct games in the series, all using nearly similar engines. Mods that support one game almost always are compatible with the others. This duplicate extension is intended to allow users who own multiple games to manage the games independently of one another in vortex. If this should be handled in another manner, please let me know. If needed, you can find the FATE:TS steam page here

Information

[X] I confirm the above is accurate

Packaging

[X] This extension is packaged correctly

Testing

[X] This game extension has been tested
Review Tasks

If a task fails, contact the author to request changes before continuing.

[ ] Double-check for existing extension
[ ] Is the extension packaged correctly?
[ ] Does it install into Vortex?
[ ] Does it correctly discover the game?
[ ] Does it successfully install a mod?
[ ] Does it successfully install a collection?
[ ] Does the game run correctly with the mods installed?

When reviewed and passed, please complete the following tasks:

[ ] Run the GitHub Actions to add to manifest
[ ] Contact author
[ ] Ask Community to enable the Vortex button for the game
[ ] Update the #vortex-announcements channel on Discord

IDCs commented 1 month ago

Hi there @LuxenDM !

Just started reviewing your fate extensions and noticed a couple of issues.

First of all I can see you're trying to raise a notification to inform the user of the FPX utility in the context.once block. Please note that the code you wrote there will never execute based on the gameMode check alone which is incorrect.

// gameMode should be compared against the gameId you provided in the registerGame call.
if (gameMode == 'fate') {
  // This will never execute
  ...
}

Please also note that context.once is not the correct location for file checks and renderer specific operations like raising notifications, dialogs, etc. You should move that functionality to the setup function you provided in your registerGame call - in this case it appears to be prepareGameDir

Finally, FPX seems to be a hard requirement - why not just write a download function for it and place it inside the setup function?

Here is an example of how we download and install SMAPI in the Stardew Valley extension.

export async function downloadSMAPI(api: types.IExtensionApi, update?: boolean) {
  api.dismissNotification('smapi-missing');
  api.sendNotification({
    id: 'smapi-installing',
    message: update ? 'Updating SMAPI' : 'Installing SMAPI',
    type: 'activity',
    noDismiss: true,
    allowSuppress: false,
  });

  if (api.ext?.ensureLoggedIn !== undefined) {
    await api.ext.ensureLoggedIn();
  }

  try {
    const modFiles = await api.ext.nexusGetModFiles(GAME_ID, SMAPI_MOD_ID);

    const fileTime = (input: any) => Number.parseInt(input.uploaded_time, 10);

    const file = modFiles
      .filter(file => file.category_id === 1)
      .sort((lhs, rhs) => fileTime(lhs) - fileTime(rhs))[0];

    if (file === undefined) {
      throw new util.ProcessCanceled('No SMAPI main file found');
    }

    const dlInfo = {
      game: GAME_ID,
      name: 'SMAPI',
    };

    const nxmUrl = `nxm://${GAME_ID}/mods/${SMAPI_MOD_ID}/files/${file.file_id}`;
    const dlId = await util.toPromise<string>(cb =>
      api.events.emit('start-download', [nxmUrl], dlInfo, undefined, cb, undefined, { allowInstall: false }));
    const modId = await util.toPromise<string>(cb =>
      api.events.emit('start-install-download', dlId, { allowAutoEnable: false }, cb));
    const profileId = selectors.lastActiveProfileForGame(api.getState(), GAME_ID);
    await actions.setModsEnabled(api, profileId, [modId], true, {
      allowAutoDeploy: false,
      installed: true,
    });

    await deploySMAPI(api);
  } catch (err) {
    api.showErrorNotification('Failed to download/install SMAPI', err);
    util.opn(SMAPI_URL).catch(() => null);
  } finally {
    api.dismissNotification('smapi-installing');
  }
}

To recap:

Use the correct gameId when comparing against the activeGameMode
Move the notification functionality to the setup function OR even better, download the FPX requirement automatically.

Also: Are you sure https://www.nexusmods.com/site/mods/801 is attempting to provide support for the same games? if so - you should get in contact with the original author and see if you can submit your modifications/enhancements to the existing game extension directly.

LuxenDM commented 1 month ago

Understood and thanks for help with the changes; i'll work on integrating them better.

For your last part - uhhh, I am the original author? And its not the same game, but four unique games that share the same engine to the point they are almost 100% mod compatible. I guess I didn't clarify that enough. All four extensions (the first original, and the three variants here) would allow people to mod the games uniquely (because Vortex doesn't support multiple local installs of a game using a single extension)

IDCs commented 1 month ago

Hey @LuxenDM,

Right, didn't notice that you're the original author of 801; that makes things easier - given that the game modding pattern is pretty much identical, you can merge all of the games in one extension. Basically every registerGame call represents a game. You could have an array of objects containing the individual data for each of the games (obviously re-use shared logic where applicable) and iterate through the array to call registerGame for each of them; that way you avoid unnecessary code duplication which saves you some time if for any reason you need to make changes to the code.

Technically you could implement this in 801 and avoid code review entirely.

Nexus-Mods / Vortex