Nexus-Mods / Vortex

Vortex Development
GNU General Public License v3.0
861 stars 127 forks source link

Review: Terraria #15794

Closed Senjay-id closed 1 week ago

Senjay-id commented 2 months ago

Nexus Username

yajneS

Extension URL

https://www.nexusmods.com/site/mods/857

Game URL

https://www.nexusmods.com/terraria

Existing Extension URL

No response

New features

1.0.1 brings new gameart that follows vortex extension guidelines and installer pattern for the most popular mods including tmod mods and save mods

Information

Packaging

Testing

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

When reviewed and passed, please complete the following tasks:

IDCs commented 1 month ago

Hi there @Senjay-id - just started reviewing your extension; there are a few issues that need to be rectified in order for the extension to get approved:

  1. The extension cannot activate properly for new users who do not have the mods path in the user documents folder.

    function findGame() {
    return util.GameStoreHelper.findByAppId([STEAMAPP_ID, GOGAPP_ID])
        .then(game => game.gamePath); // <----- this will resolve to the location of the .sh file (wherever GoG/Steam stores it) and will be used as the discovery.path.
    }
    
    // please note that the discovery path that Vortex stores in its state, is the location of the extension's required files.
    function prepareForModding(discovery) {
    //// discovery.path will point to the location of tModLoader and not the mods path in the user's documents folder!
    return fs.ensureDirWritableAsync(path.join(discovery.path, 'Mods')); // <---- This will never create the correct mods folder 
    }
  2. Our userbase consists of mostly Windows users, yet the executable is set to a .sh file - please enhance the extension to run the correct script depending on the user's OS (if I'm not mistaken tModLoader comes with both .bat and .sh files)

Senjay-id commented 1 month ago

@IDCs Uploaded a new version.

Fixed the first issue by swapping the first return argument function with modPath

Fixed the second issue by using the .bat file present in the root tmod folder, I don't have other OS to check before implementing a function to check the user's os so i'll leave them as a bat for now

Removed the GOG discovery because i just realized that's the id for Terraria, not TModLoader and TML doesn't have a gog version, only Steam.

IDCs commented 1 week ago

Thank you for your contribution