-
Notifications
You must be signed in to change notification settings - Fork 325
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
[xlcore][feature] Dxvk settings rework #1205
Conversation
* Change the 4th arg in constructor to have a default value of null * xlcore calls this with only 3 args, so this allows compiling.
* Add dxvkVersion * Move dxvkAsync, dxvkHud into DxvkSettings * Set up dictionary for setting env vars. This will allow arbitrary number of vars to be set. * Reworked CompatibilityTools to support new features.
* uses DXVK_FRAME_RATE, so doesn't work with wineD3D * leave unset instead of 0, since that lets the user script it.
* Default is with no config * Custom will allow custom path to config file Currently uses ~/.config/MangoHud/MangoHud.conf by default. * Full shows almost everything. Very fun on a cpu with lots of cores and threads.
* Use ~/.xlcore/MangoHud.conf as first option for MangoHud Custom * If that doesnt' exist, use ~/.config/MangoHud/wine-ffxiv_dx11.conf * Finally, fall back to ~/.config/MangoHud/MangoHud.conf If it can't find any of those, it'll just present the default.
* DXVK Hud will only accept alphanumeric, comma, and = * MangoHud config file must exist
* If the file is not found, it will now just not set the file path * This will cause MangoHud to fall back to default locations which is unlikely to work in flatpak, but that's okay. * If the value passed is empty, it'll try to find a few default paths in .xlcore and .config/MangoHud
* set to empty config instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing all DXVK settings together in one class, this is a nice change.
Just a few thoughts on even improving DXVK handling further:
Setting DXVK_LOG_PATH
to the xlcore provided logs folder would be nice.
In the long run it would also be good to set DXVK_CONFIG_FILE
and DXVK_STATE_CACHE_PATH
to outside of the wine prefix, as that would make the shader state cache and DXVK configuration survive a prefix swap or deletion
@@ -68,10 +66,28 @@ public CompatibilityTools(WineSettings wineSettings, Dxvk.DxvkHudType hudType, b | |||
this.dxvkDirectory.Create(); | |||
} | |||
|
|||
if (!wineSettings.Prefix.Exists) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be duplicate code from line 80...
Actually there is no reason to do this at all since wine will create it's prefix folder on first startup anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it is... deleting that. You know how it goes with copy/paste
if (!wineSettings.Prefix.Exists) | ||
wineSettings.Prefix.Create(); | ||
} | ||
|
||
public CompatibilityTools(WineSettings wineSettings, Dxvk.DxvkHudType hudType, bool? gamemodeOn, bool? dxvkAsyncOn, DirectoryInfo toolsFolder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly wouldn't bother adding that, it's just unnecessary clutter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine. I was just putting it in so it would still build against unmodified xlcore. Deleting this as well.
@@ -40,7 +40,7 @@ public class Launcher | |||
|
|||
private const string FALLBACK_FRONTIER_URL_TEMPLATE = "https://launcher.finalfantasyxiv.com/v620/index.html?rc_lang={0}&time={1}"; | |||
|
|||
public Launcher(ISteam? steam, IUniqueIdCache uniqueIdCache, ISettings settings, string? frontierUrl) | |||
public Launcher(ISteam? steam, IUniqueIdCache uniqueIdCache, ISettings settings, string? frontierUrl = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems out of scope for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. It was just in so I could build against unmodified xlcore. Can definitely remove.
@@ -71,7 +71,7 @@ public Launcher(ISteam? steam, IUniqueIdCache uniqueIdCache, ISettings settings, | |||
this.client = new HttpClient(handler); | |||
} | |||
|
|||
public Launcher(byte[] steamTicket, IUniqueIdCache uniqueIdCache, ISettings settings, string? frontierUrl) | |||
public Launcher(byte[] steamTicket, IUniqueIdCache uniqueIdCache, ISettings settings, string? frontierUrl = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
Dxvk.DxvkVersion.v1_10_2 => "1.10.2", | ||
Dxvk.DxvkVersion.v1_10_3 => "1.10.3", | ||
Dxvk.DxvkVersion.v2_0 => "2.0", | ||
_ => VersionOutOfRange(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should be an ArgumentOutOfRangeException
, as it is not really supposed to happen and you'd have to change the default DXVK version in the future in like 3 different places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, found a couple of examples in the code and mimicked them. It compiles, and, as you said, this shouldn't ever actually happen to a normal user. Changed.
} | ||
} | ||
|
||
private string VersionOutOfRange() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unneeded with the above change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And deleted.
public Dxvk.DxvkVersion DxvkVersion { get; private set; } | ||
|
||
private const string ALLOWED_CHARS = "^[0-9a-zA-Z,=.]+$"; | ||
private const string ALLOWED_WORDS = "^(?:devinfo|fps|frametimes|submissions|drawcalls|pipelines|descriptors|memory|gpuload|version|api|cs|compiler|samplers|scale=(?:[0-9]){0,2}(?:.(?:[0-9])+)?)$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for scale being < 100 ?
DXVK will allow such values and will sanitize them by itself, in fact the only restriction it enforces is scale >= 0.01:
https://github.com/doitsujin/dxvk/blob/c0d843c5788448ed66d61246c6ad8453c41c87fd/src/dxvk/hud/dxvk_hud.cpp#L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know if there were any limits, it just seemed a reasonable high number. Scales over 10 are probably not useful unless you're on an 8K+ display, but it's easy enough to change {0,2} to *. So... changed.
[SettingsDescription("1.10.2", "Newer version of 1.10 branch of DXVK. Probably works.")] | ||
v1_10_2, | ||
|
||
[SettingsDescription("1.10.3", "Newer version of 1.10 branch of DXVK. Probably works.")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.10.3 for sure works with Dalamud + GShade; it's really only 2.0 that is creating trouble
Remove Launcher.cs commit; out of scope.
Changes per marzent's suggestions.
* Change 1.10.2 & .3 to say safe to use. * Change 2.0 to be less redundant.
Added the dxvk log to logs folder, and added a .dxvk folder for state cache. It'll even be possible to pass in the correct storage path, assuming we ever get around to allowing changing that. By default, just assumes ~/.xlcore, though. |
Alright, should be good now as of commit df1a3be |
And I'm not sure if sync/async need different caches, so I'm keeping them separate now. Hopefully that's everything. |
Theoretically they shouldn't, but there is some bugginess with async and DXVK 2.0 where async will miss saving entries sometimes, so this is probably cleaner |
this.DxvkHud = hud; | ||
string home = Environment.GetEnvironmentVariable("HOME"); | ||
corePath ??= Path.Combine(home,".xlcore"); | ||
string dxvkConfigPath = Path.Combine(corePath,".dxvk"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is cleaner to pass a DirectoryInfo into the cctor here as well, just in case the base xlcore storage path will change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just the default. You can pass in the correct path to the constructor; it just defaults to null and sets the default to ~/.xlcore in that case. XL Core's Program.cs creates a DxvkSettings object, and I've passed it in there. But I could change it to pass in a DirectoryInfo. It's probably more "correct". So now the storage.Root object can be passed in from XL Core, instead of the string storage.Root.FullName.
Pulled in marzent's commits. I am using the public static functions I declared in the XL.Core UI, so I added those back in. |
Got rid of a useless function that marzent previously removed and I foolishly re-added because I panicked when I saw a compile error about missing function That should be it for now. Everything seems to be working when I build PR #17, both natively and as a flatpak. |
Should we bump the default version to 1.10.3, and leave 1.10.1 and .2 in there for compatibility/testing purposes? |
1.10.3 is perfectly fine on any platform and plays nicer with some nvidia drivers iirc |
src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs
Outdated
Show resolved
Hide resolved
Made obsolete by #1366 |
Edit: Links updated for building with marzent's suggestions added.
This PR will add the necessary framework to XIVLauncher.Common.Unix to allow XL Core to add several new features around DXVK. This is only the first part of the patch, but it needs to be in place before XL Core can be updated.
I've been working on this for a while, trying to get it into a place where it would work in a flatpak environment without hanging or crashing. It's finally working now.
You can try it out for yourself by following the instructions here: XL Core Testing Flatpak, or build it by cloning my fork of XL Core and checking out the xlcore-fixes-plus-dxvk branch, and doing a
dotnet publish
The
master
branch includes all the XL Core updates I'm working on, and the 1.0.2+new-dxvk-settings branch, is there to prove that it can be built against XL Core with no changes other than updating the submodule.Here's some screenshots of how it looks (with XL Core patches).
Wine Tab:
Dxvk Tab:
Version Switcher:
Hud Switcher: