-
Notifications
You must be signed in to change notification settings - Fork 72
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
ReShade: Implement Custom ReShade DLL Name #881
Conversation
Following discussion in #873, we'll probably just go with backing up any existing DLLs with the same name that we're trying to use by default if this is a first-time ReShade install. Any further changes to this behaviour can be adjusted based on user feedback. Since the user can choose any name now, this makes sense imo in case they accidentally overwrite a DLL. As for when we change the name of a DLL and still have a stray one, I think this is fine. It might make the internal files a bit messy but the user can clean it up if they really care about that (they'd probably have to do this with mods and such anyway too - when tinkering, expect to get your hands dirty!) So the main things left to do with this PR are:
I want to get these changes in soon, as this is useful imo and also fixes OpenGL with ReShade which is a cool win. |
Pushed a change to try to detect when a DLL with the same name as our chosen name exists in the game folder, but is NOT present in This change is for now untested. |
This allows users to easily simulate the old behaviour of copying both DLLs.
Added |
For some reason, it looks like the dropdown always defaults to |
D'oh, I used the wrong arguments for |
6fbe8e5 adds a check for DLLs to ensure they end with There is also some logic to ensure these updated names are written to the game config file. For example if the user writes out That commit also fixed the silly issue with choosing the wrong DLL name. |
There seem to be cases where the DLL names in the game config can sometimes get duplicated. For example, I noticed some issues with how we were handling the |
Pushed a commit that refactors If the DLL already exists on the path and if it IS in the ReShade.txt file, then we assume it's an existing file we're already tracking and ignore it. This should NOT impact ReShade Update because that's in a separate block, and if The So as well as testing for DLL conflicts, I also tested the DLL naming changes I made. If the DLL doesn't end with a file extenion, I tested a few cases to ensure that the Basically all that's left for this is testing actual shaders, and then updating the documentation+langfiles. |
I wonder if it's worth doing any kind of notifier work for this... Unsure for now. It's probably too noisy, so this could always be added later if we really want. |
A nice QoL enhancement might be to check for duplicate ReShade DLL names, like if the user actually enters It would be possible to do this with something like |
lgtm tested, and everything works as expected for the most part. Just some minor things.
notice the lack of line break between
|
Thanks a bunch for testing! Some great observations here that I'll try to address. With some luck this should be ready for the weekend 🥳
Tackling this may be tricky. I chose to let this case be on the basis that a user should be responsible for which DLL name they choose. Going into the game files and removing the offending DLL is the approach I chose to go with. I am not sure how we could know when we should remove the DLL, in other words how we can tell that a DLL is definitely a previously added ReShade DLL. I think there may be some kind of A possible fix would be to store and compare the previous ReShade DLL name or names, but there could be a case like this:
I fear being too "hands-on" with removing DLLs from the game folder could end up being problematic. Even renaming them could have the same effect: A user still has to go and intervene in the game files. I would rather leave the unnecessary DLLs in place. The case you've brought up is absolutely valid and even something I encountered but I don't know a good way to deal with it. Warning the user on the wiki about this and documenting how to resolve this kind of issue (i.e. game crash, Direct3D error messages) may be the way to go, essentially to say "tinker responsibly" 😉 It is annoying but to my understanding it is possible to have this happen on Windows if the user chose an invalid rendering API name (such as if they chose D3D11 and it started causing a game to crash). I recognise though that for STL we use the DLL names and not the API names, and as you pointed out in #873 it may not be immediately clear to the user which DLL to choose. I'm hoping the tooltip which explains it (albeit a little verbosely) will help alleviate this, and we'll also document this on the wiki, so hopefully users will be able to find out which DLL name to choose for the corresponding API. And if they don't know the API they'd have to look it up on Windows, too. As an aside, I went with the DLL names over the API names for simplicity (mapping the names would've been a bit of a pain...) and since the user can enter DLL names, I think it makes slightly more sense to just take DLL names. I did consider as well having the option to enter both the API name and DLL names, and perhaps trying to map multiple values (i.e. accepting DX9, D3D9, Direct3D 9, DirectX 9, etc) but I realised this could conflict with the custom DLL names (i.e. if the user for some reason wanted
Absolutely. I came to a similar conclusion below but wrote it all out before I actually saw this point. Great idea, I will add this. It might have a slight performance cost, especially if the user is adding a lot of ReShade DLLs. This is extremely unlikely imo, I'd say maybe a handful of DLLs at most is the most likely scenario. It should be feasible to do and will ensure the contents of Justification for Writing to ReShade.txt every launchI wrote this before reading the above point but I'm keeping it in this reply mostly so I can refer to it and remind myself as well of what needs to be done and why 😅 There is a different problem though. This To fix this, we'll need to re-create the I'll implement this tonight if I have some time.
Wow! That is a really good catch. This is a bug and potentially a fairly significant one actually (almost definitely exclusive to this PR though), there should be a newline. This should hopefully be fixed as part of better managing which DLLs are listed in the ReShade.txt file.
Interesting, the code is supposed to check the ReShade version from the given ReShade DLL. It would be interesting if you could check the log in a scenario when an up-to-date ReShade DLL is present but still gets overwritten. I remember you mentioning this in your OP for #873, but I thought it was fixed as I didn't notice it during testing for this PR. I will also take another look though later tonight. |
So it turns out removing and writing out to the ReShade.txt on each boot is a bit trickier than I thought. Since we rely on this file to tell what the tracked ReShade DLLs are, we couldn't do it before the DLL installation. So I decided to test out removing and re-writing to the file after we move the DLLs. Essentially once we move the DLLs we update the ReShade.txt at the end instead of in |
Okay, that approach seems to be working well. Basically:
So now as of the latest commit, we will only track ReShade DLLs that are entered into the ReShade DLL name box, and d3d47, so that the above scenario worked. If not for that, any superfluous DLLs that the user had ever previously entered, would be marked with This was a great catch on your part, it was a little tricky for me to think through how to resolve it without breaking the checks for existing DLLs, but I think this works. Please let me know if you find any fault in this logic, because in my head I still have a nagging feeling that this may not be solid so a Second Opinion would help put my mind at ease a little :-) You've helped out a lot with the ReShade stuff and I'm really grateful you're always willing to help test out. But I wanted to say that I'm not going to come back and point the finger at you if you don't catch a certain edge-case. If there's a flaw with the logic that you missed or anything else in this PR that's fine, I'm just looking for a bit of testing to give me a bit more confidence that this actually works, from someone who is a bit more experienced with using ReShade than myself. If there are problems discovered later on you can always open an issue, and otherwise I am sure someone will complain about it eventually :-) I could not reproduce the issue of the We write out to the I will investigate the ReShade updating issue you mentioned, as well as doing some (hopefully) minor code refactoring, then I think this PR should be good to merge now that the ReShade DLL tracking issue is resolved. I believe that is the last outstanding piece of work assuming the updating DLLs works as expected, i.e. only updating when the ReShade DLL version does not match our selected version. There may be a conflict in when we check for updates here with the new ReShade conflict logic in this PR, so I will be looking around that area of the code first as a potential culprit. This is still looking on track to be merged soon, assuming I have not made any silly mistakes with resolving conflicts+writing out our tracked DLLs to the ReShade.txt appropriately. |
I was testing the ReShade update and it seems to be working for me. It seems to only update when there is a different ReShade DLL version. I tested the following:
Throughout all of this as well, the ReShade.txt file was updated as expected, so I think we're good on that front as well. Unsure if I perhaps fixed something somewhere along the way to resolve the problem but let me know if that scenario I tested is wrong and I need to try something else 🙂 |
I refactored the architecture check to be a bit cleaner, since with the new custom DLL name logic we use a loop to copy the DLLs. Instead of repeating this code, we can set which architecture of ReShade DLLs to copy from the source folder to the destination folder (and the architecture of the d3d47 DLL). This is just a bit cleaner and doesn't appear to have broken anything. The code should be good now I think, pending some further tests from yourself if you have time. If you don't or if you're happy enough with the state of the PR I am ready to merge once I get the wiki updates sorted :-) |
Exactly. Yeah I also thought maybe it would be a little difficult to restore
it sounds good to me I will test it and let you know, but I agree every time we install reshade DLLs we want to recreate the
no worries sorry I haven't really had the time to implement this my self but I use this feature often so I will be testing it now and whenever I game to 😄. Like I said in #873 these where issues that where on my mind and I intended to resolve but never got around to 😅 Though the PR as is now grew way more than what I was planing (Good Job though 👍 )
I got that issue by leaving the reshade dll box at So really this should be fixed by always re-creating
Well, what I was testing was just leaving all reshade options enabled so enable, install, override, and update. (Now that I think about it, how was I sure it was the update function that kept replacing the .dll It could of just as easily been the override will test) I noticed that the One question I have is I noticed there is an option for |
Aha! Now this is a good point. I looked into this when I started work on this PR and in the beginning I had made reference to opengl32/opengl64, as I assume these were the DLLs - It would make sense, right? opengl32 for 32bit, opengl64 for 64bit. Windows uses the same
I'll try to reproduce it with all the options on again when I have some time, I was having trouble re-creating it but I forget now what other options I had enabled :-) |
ReShade wiki has been updated: https://github.com/sonic2kk/steamtinkerlaunch/wiki/ReShade So aside from further tracking down the issue relating to the ReShade DLLs being updated when they don't need to be, this PR is ready to merge :-) |
Had to resolve a conflict with the file naming, the version will need to be bumped anyway before this is merged 😄 |
I think I will merge this PR as-is, if there are still issues with up-to-date ReShade DLLs being overwritten when they shouldn't be we can look at that. I thought perhaps the ReShade Version Override logic might be the culprit here but I couldn't reproduce the problem, and in Also, working in the ReShade code a bit more I can see more how it works and I have an even bigger appreciation for the work done to set up the ReShade Version Override code. It's a pretty cool feature I've come to appreciate from a utility perspective more as well as a code perspective. I'll merge this PR as-is for now to avoid it getting stale. Thanks for all the feedback on ReShade in STL, any improvements I can make I am happy to :-) |
Thanks, @sonic2kk from my testing other than reshade constantly getting updated I see no other issues. was able to play tales last night for a few hours and played around with some shaders as normal |
Mostly resolves #873, as the core issue there is that ReShade is selecting the wrong DLL. There was an issue raised with the UX of enabling ReShade, but that is not resolved in this PR. It will be tackled separately.
Background
Currently, STL will copy over the ReShade DLL for the relevant game architecture (such as ReShade32/64 for 32bit and 64bit respectively). It copies the same DLL but names it
dxgi.dll
andd3d9.dll
. Only one of these will ever be read by a game, but using this DLL covers us for Direct3D 9 (d3d9.dll
) and Direct3D 10/11/12 games (dxgi.dll
). However, there can be cases when this is problematic, such as in #873 where a game will crash if it tries to readd3d9.dll
.A proposed solution for that is to only copy one DLL, defaulting to
dxgi.dll
, and allowing the user to select/enter their own name for the ReShade DLL. This is useful for cases like that mentioned in the above issue, and also allows for avoiding conflicts where a modloader may already be using the ReShade DLL name. A custom name could avoid conflicts.Implementation
This PR implements a combobox entry textbox for selecting a ReShade DLL name, or entering a custom one. This name is then used for the 32bit/64bit ReShade DLLs in the game folder. Users can by default choose
dxgi.dll
,d3d9.dll
,d3d11.dll
, andopengl32.dll
. They can also enter a custom name. The textbox also allows for inputting comma-separated DLL names, so they can optionally copy over multiple DLLs if they so choose: for example,dxgi.dll,d3d9.dll
(which will mimic existing behaviour).When the ReShade DLL name is blank, we default to
dxgi.dll
.On Windows, ReShade will default to
dxgi.dll
, and tell the user they have to check which DLL is right for whichever API the game is using. PCGamingWiki has a helpful list: https://www.pcgamingwiki.com/wiki/ReShade#Compatibility_list - We should document this on our wiki, too.This PR also implements support for using ReShade with OpenGL games, as it now allows the user to copy the OpenGL DLL and will also automatically set the OpenGL DLL override, allowing games to work. This was tested with "DOOM 3: BFG Edition". Some other games (such as Hotline Miami running under Proton with the
-gl
option) could be tested.Testing
The following games have been tested up to the point of showing the ReShade menu:
Existing ReShade installs have not been tested, and I have not actually tested any shaders. In general, this PR would benefit from testing by those who actively use ReShade
Compatibility/Remaining Issues
There are a couple of concerns I have with compatibility which have not yet been resolved. I also do not have a clear idea on how to handle these cases just yet either:
d3d11.dll
on a game that doesn't like it, such as Let's zig zag), then the game will be left in a bad state and the user will have to remove the DLL themselves. This would also apply on Windows.For the two above reasons, I think the best solution is to simply ignore bad ReShade DLLs. Even with existing behaviour, if a bad DLL was chosen, the user would have to manually remove it. Since now we only copy one DLL and let the user choose the name, it may be okay to expect them to choose the name sensibly and to manage ReShade+mods together, to know not to inadvertently overwrite a DLL.
There is also a case where a DLL we're trying to move into the game folder already exists. In these cases it was proposed to have a dropdown, with the preferred behaviour being to rename the DLLs, but I am a bit torn on this. For simplicity I would like to leave it up to the user to manage how they name their DLLs and resolve conflicts, but I am also curious as to what ReShade does on Windows. If ReShade overwrites, then we should too.
Documentation
We should update the ReShade wiki once this is merged to note some of the changes here:
TODO:
opengl32.dll
already exists and we try to name a ReShade DLLopengl32.dll
when that was not placed by us (missing fromReShade.txt
in game folder), existingopengl32.dll
should becomeopengl32.dll.bak
)..dll
extension to filenames without extension