sonic2kk / steamtinkerlaunch

Linux wrapper tool for use with the Steam client for custom launch options and 3rd party programs
GNU General Public License v3.0
2.04k stars 70 forks source link

ReShade INI being overwritten/deleted #1071

Closed zany130 closed 3 months ago

zany130 commented 3 months ago

System Information

Issue Description

setting CREATERESHINI to 1 creates a ReShade ini as expected but it will also overwrite existing ReShade ini's so it looks like the check for existing ini is not working.

Even stranger for some reason in Persona 3 Reloaded disabling CREATERESHINI causes the existing ReShade ini to be deleted.

my understanding is if CREATERESHINI is 0 it just skips the ini creation code. It shouldn't be deleting the ini... yet for some reason setting it to 0 causes this

Logs

2161700.log

sonic2kk commented 3 months ago

I can reproduce the removing INI behaviour, not sure why though. I don't think this is intended., as per the note on the OP for PR #1000

and also we should not[e] that this will not remove an existing ReShade IN

so it looks like the check for existing ini is not working.

Hm, I don't remember if we're supposed to do this, but it sounds sensible, so I'll take a look for this too.

sonic2kk commented 3 months ago

The removal code for the INI is handled in removeReShadeSpecialKInstallation.

This looks like a mistake on my part:

    if [ "$KEEPRESHADEINI" -eq 1 ]; then
        rmFileIfExists "$INSTDESTDIR/$RSINI"
    fi

My guess is that we're supposed to check -eq 0 based on the variable name (i.e. if KEEPRESHADEINI == false, remove INI).

sonic2kk commented 3 months ago

Yeah, changing the above to check -eq 0 fixes it. This is a bug introduced in #921 I think (https://github.com/sonic2kk/steamtinkerlaunch/pull/921/files#diff-77c6d4bb1bf7c1988ff5a068c856b6c24e113663f2e61cb3a2c98723c0fafc04R8931) which was not in a release, so I won't note this fix on the changelog.

I also think this behaviour is incorrect, not just logically, but because it doesn't match the behaviour in removeReShadeInstallation, which uses [ "$KEEPRESHADEINI" -eq 0 ];.

As for checking for an existing INI, I'll take a look at this too. Would this be a valid testing scenario?

  1. Start a game with no SteamTinkerLaunch config and no ReShade files installed from SteamTinkerLaunch.
  2. Place a "ReShade.ini" file in the same folder as the EXE, where SteamTinkerLaunch would create it.
  3. Start a game with a fresh SteamTinkerLaunch config and enable ReShade, with the default option to CREATERESHADEINI enabled.
  4. SteamTinkerLaunch will (probably incorrectly) overwrite the existing ReShade INI file.
  5. Close game, and manually edit the ReShade INI
  6. Restart game with SteamTinkerLaunch and the INI will still get overwritten
  7. Close game, manually edit the ReShade INI again :-)
  8. Start game without SteamTinkerLaunch and manually load ReShade (if it is not automatically picked up, setting a DLL override from the launch options should force the game to pick it up), INI will not get overwritten, which will confirm that SteamTinkerLaunch is overwriting the file and not ReShade itself on load

Likely, overwriting an existing ReShade INI is also a bug introduced as part of the major ReShade rework, so once investigated if a fix is needed I probably won't note this on the changelog either :-)

sonic2kk commented 3 months ago

Huh, it seems correcting the line to if [ "$KEEPRESHADEINI" -eq 1 ]; then fixes the ReShade INI overwrite issue. I tested by adding PerformanceMode=1 locally with the fix.

There is likely no need for a PR for this, so I'll just push the fix directly to master.

sonic2kk commented 3 months ago

This was automatically closed because I mentioned the issue, even though I deliberately used "may fix" ;-_-

Whenever you can, please test out that commit and see if it fixes your problem :-) If the problem really was this simple, well, that's embarrassing that I didn't catch it earlier...

sonic2kk commented 3 months ago

Should've created a ReShade label long ago, but never got around to it until now. Better late than never! :smile:

zany130 commented 3 months ago

Will test on my end when I get home. But from your earlier comments I think it was that easy 😂 had a feeling there was incorrect conditional lol

zany130 commented 3 months ago

was able to confirm that the overwrite part of the bug is fixed. Haven't tested an existing in being deleted if the option is disabled yet.... will test latter today

zany130 commented 3 months ago

Yup everthing looks good ini is no longer overridden or deleted sorry i took so long to get back to this

sonic2kk commented 3 months ago

No problem :-) If I had actually gotten around to playing more of P3R, I'd make a Persona-related "happy gaming" reply, but I haven't, so it's just "happy gaming" to you for now :-)