Compatibility rework #1348

rankynbass · 2023-06-24T22:47:16Z

This replaces the Dxvk Settings Rework - #1205

Here's the compatibilitytools rework I mentioned a few weeks ago. This moves most of the bits that will change over to the XL.Core side, such as specific versions of wine or dxvk, where they will be stored, and what environment variables need to be set for each one.

I also moved the MacVideoFix over to XL.Core. I figure individual implementations of fixes can be done on the XL.Core side, and then passed to GameFixApply object.

Depends on

And here's a link to a flatpak build if you want to test:
https://github.com/rankynbass/XIVLauncher.Core/releases/edit/compatrework-v1

Install with flatpak install xivlauncher-rework.flatpak, run with flatpak run dev.goats.testing.xivlauncher.

This reverts commit c37927c. It's better to keep all this on the XL side.

marzent · 2023-06-27T06:14:18Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

    public bool IsToolReady { get; private set; }

-    public WineSettings Settings { get; private set; }
+    public WineRunner WineSettings { get; private set; }


Why rename WineSettings to WineRunner, when it still only holds settings and everything is actually run here

Good point. Can certainly change this back to WineSettings.

It made more sense when I started the whole thing, honestly. The names just kind of stuck around. I can certainly change it back. When I started this whole thing, Dxvk and Wine were both in the same object, but it became too complicated and janky, so I split them. I think Runner was chosen simply because that's the folder lutris stuffs wine installs into.

marzent · 2023-06-27T06:14:44Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

-                                    Settings.CustomBinPath;
-    private string Wine64Path => Path.Combine(WineBinPath, "wine64");
-    private string WineServerPath => Path.Combine(WineBinPath, "wineserver");
+    public DxvkRunner DxvkSettings { get; private set; }


DxvkRunner is also a Misnomer IMHO

marzent · 2023-06-27T06:15:45Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

-    private readonly Dxvk.DxvkHudType hudType;
-    private readonly bool gamemodeOn;
-    private readonly string dxvkAsyncOn;
+    public string WineDLLOverrides;


Why does this need to be a public field?

marzent · 2023-06-27T06:20:15Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

+        DxvkSettings = dxvkSettings;
+        EnvVars = environment;
+        Prefix = prefix;
+        WineDLLOverrides = (string.IsNullOrEmpty(wineoverrides)) ? "msquic=,mscoree=n,b;d3d9,d3d11,d3d10core,dxgi=n,b" : wineoverrides;


Why are we setting wineoverrides externally and then have this internal fallback here, and the fallback doesn't even take into account dxvkSettings...
IMO they should only be set in 1 place and ideally here

marzent · 2023-06-27T06:21:26Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

-            if (!this.dxvkDirectory.Exists)
-                this.dxvkDirectory.Create();
-        }
+    public CompatibilityTools(WineRunner wineSettings, DxvkRunner dxvkSettings, Dictionary<string, string> environment, string wineoverrides, DirectoryInfo prefix, DirectoryInfo toolsFolder, FileInfo logfile)


environment should only contain extra user specified env vars that override the standard behaviour

marzent · 2023-06-27T07:19:55Z

src/XIVLauncher.Common.Unix/Compatibility/WineRunner.cs

+
+namespace XIVLauncher.Common.Unix.Compatibility;
+
+public class WineRunner


Not sure if this needs to be called runner, when all it does is hold wine settings/configuration

marzent · 2023-06-27T07:31:04Z

src/XIVLauncher.Common.Unix/Compatibility/WineRunner.cs

+
+    public Dictionary<string, string> Environment { get; }
+
+    public WineRunner(string runCmd, string runArgs, string folder, string url, string rootFolder, Dictionary<string, string> env = null, bool is64only = false)


is64only is a misnomer here, in general all wine builds with 64 bit support will have a dedicated wine64 executable which is the one that should be invoked for xiv and Dalamud, as when you use the wrong 32 bit one, wine will notice and call wine64 for you (except for some IL-only executables).

is64only should always be true, if anything, but is not even needed to be specified at all... The only exception to not calling wine64 is using the new experimental wine8 wow64 backend, which will create a single 64 bit only wine executable, that internally thunks all system calls when passing it a 32 bit PE executable (which ironically means when you have a true 64 bit only wine 8 is64only should be false here).

This is easy enough to handle transparently handle though, always pick wine64 unless it doesn't exist, then pick wine.

marzent · 2023-06-27T07:35:23Z

src/XIVLauncher.Common.Unix/UnixDalamudRunner.cs

+        Console.WriteLine("[DALAMUD] " + output);
+
+        // Skip "ERROR: Could Not Get Primary Adapter Handle" and proceed to next line
+        if (output.Equals("ERROR: Could Not Get Primary Adapter Handle"))


Why is this necessary?
Wine should always output its internal logging to stderr, but this doesn't even look like wine.

marzent · 2023-06-27T07:36:14Z

src/XIVLauncher.Common.Unix/UnixDalamudRunner.cs

@@ -74,43 +74,57 @@ public UnixDalamudRunner(CompatibilityTools compatibility, DirectoryInfo dotnetR
        launchArguments.Add(gameArgs);

        var dalamudProcess = compatibility.RunInPrefix(string.Join(" ", launchArguments), environment: environment, redirectOutput: true, writeLog: true);
-        var output = dalamudProcess.StandardOutput.ReadLine();
+        var output = dalamudProcess.StandardOutput.ReadLine() ?? "";


output is checked against null later, no need to null-coalesce it twice.
Also output being null is meaningful information, since it indicates that the process exited without writing a single line to stdout

marzent · 2023-06-27T07:41:59Z

src/XIVLauncher.Common.Unix/UnixDalamudRunner.cs

        try
        {
            var dalamudConsoleOutput = JsonConvert.DeserializeObject<DalamudConsoleOutput>(output);
-            var unixPid = compatibility.GetUnixProcessId(dalamudConsoleOutput.Pid);


All of UnixDalamudRunner can be left as is with the adjustments to GetUnixProcessId from before

If GetUnixProcessId is properly implemented all of this can be really left as as (save the JSON reading adjustment and spelling mistakes) to keep it simple

rankynbass · 2023-06-27T14:07:24Z

I'll certainly check into all of this when I get a chance later today. Thanks for taking a good look at this.

* Changed names of classes to WineSettings and DxvkSettings * Moved the install functions back to compatibilitytools * Removed a bunch of unused variables * Restored UnixDalamudRunner to be much closer to original * Moved WINEDLLOVERRIDES completely to compatibilitytools * Renamed some vars for better clarity.

rankynbass · 2023-06-28T05:31:49Z

First, marzent, thank you for your thorough review of this PR. It was very helpful.
Second, rather than spam everyone with 20 emails for responding to your feedback, I’ll address it all here (hopefully in order).

Renamed WineRunner and DxvkRunner to WineSettings and DxvkSettings. I suppose I could have kept Dxvk instead of DxvkSettings, but I think this is better just for consistency.
WineDLL overrides: yeah, it’s all over the place. I’ve fixed it to just do it in compatibilitytools now, by checking for DxvkSettings.IsDxvk and setting it appropriately. There is still an override for wineD3D in big RunInPrefix() function, but I think that should stay there. There might be a reason we would want to launch a process with wineD3D while keeping the game in Dxvk.
Got rid of the extra special environment variable for the moment. Might bring it back later if it’s needed, but it wasn’t actually used.
Renamed beta to wine in all cases I could find. It compiles and puts files in the right place, so I assume I got them all.
Moved the two install functions from the two Settings classes back into CompatibilityTools. Those functions are now called in the EnsureTool() function. Renamed them to EnsureWine() and EnsureDxvk().
The logfile entries with (string) and (array) were in there because I was trying to track down exactly which one was being called and causing me problems. This has been removed.
The LD_PRELOAD merge is there because I was trying to get mangohud to work with opengl. This is actually the reason a lot of the extra variables are in there; I had to basically hijack the WineSettings.RunCommand, set it to mangohud, pass a bunch of arguments (including the original wine binary), and set an LD_PRELOAD value to make sure it got loaded properly. This happened in Dxvk.Environment, which could conflict with WineSettings.Environment and the gamemode setting. So the merge was to make sure they played nice with each other. I’ve left this in for now.
I’ve added a public property to CompatibilityTools, and passed it through the constructor. It’s not used for anything yet, but might be useful in the future, and now it’s in the Common.Unix side. I commented out the FLATPAK_NOTRIGHTNOW stuff.
I left the GetUnixProcessIdByName() as public, because I still want to call it in one specific case.
Cleaned up a bunch of leftover variables from DxvkSettings and WineSettings. The runCmd variable in the WineSettings constructor has been renamed to customwine, to indicate that’s what it actually is used for.
The is64only stuff was for Wine built with WoW64, which supports 32-bit windows programs without needing 32-bit linux libs. You’re absolutely right that this can be done transparently, though.
I restored the UnixDalamudRunner.cs file, mostly. I added an extra call to GetUnixProcessIdByName() into the catch statement at the end, because sometimes Dalamud will through out an extra error line that can’t be parsed by the JsonReader because it’s obviously not json. But Dalamud often keeps running regardless, and we can still potentially find the game process. So I think it’s better to make a second attempt than to return null, since that can cause the XIVLauncher.Core process to stick around after FFXIV exits.

For reasons unknown, after doing all these changes the Wine 7.11 that I tested for you the other day started working. It still fails in the current flatpak and rpm builds (and on the previous test builds I posted), but with these changes it now properly loads Dalamud, as if there were no problems at all. Previously, the game would load, but the process would hang trying to read the output. With these changes it doesn’t, and I have no idea why.

I’ll look at the XL.Core side tomorrow.

Thanks!

rankynbass · 2023-06-29T05:39:26Z

I made a mistake in detecting wine or wine64 binary. This was actually the cause of the Wine 7.11 build working: it actually wasn't. Instead, I was accidentally trying to find wine (not wine64), failing, and then falling back to whichever Managed wine option was last selected. So the 7.11 wine still doesn't work, and I've fixed the logic.

code to DownloadTool().

marzent · 2023-07-23T17:31:16Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

        }
+        WineDLLOverrides = wineoverrides;


This is just a very complicated way of writing WineDLLOverrides = $"msquic=,mscoree=n,b;d3d9,d3d11,d3d10core,dxgi={dxvkSettings.IsDxvk ? "n,b" : "b"}";

marzent · 2023-07-23T17:35:12Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

-        await Dxvk.InstallDxvk(Settings.Prefix, dxvkDirectory).ConfigureAwait(false);
+
+        // Check to make sure dxvk is valid
+        if (DxvkSettings.IsDxvk)


DxvkSettings.IsDxvk sounds a bit weird (what are DxvkSettings that are not DXVK?) maybe renaming it to DxvkSettings.Enabled would be nicer

marzent · 2023-07-23T17:36:20Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

+        await DownloadTool(wineDirectory.FullName, WineSettings.Folder, WineSettings.DownloadUrl);
+
+        // Use wine if wine64 isn't found. This is mostly for WoW64 wine builds.
+        WineSettings.RunCommand = WineSettings.SetWineOrWine64(Path.Combine(wineDirectory.FullName, WineSettings.Folder, "bin"));


We are not setting anything there... SetWineOrWine64 is a a pure function (and not really needed anyways)

marzent · 2023-07-23T17:42:49Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

-#endif
+        var wineEnvironmentVariables = new Dictionary<string, string>();
+        if (wineD3D)
+            wineEnvironmentVariables.Add("WINEDLLOVERRIDES", "msquic=,mscoree=n,b;d3d9,d3d11,d3d10core,dxgi=b");


The logic here is completely different from WineDLLOverrides above and should be consolidated (ideally here)

marzent · 2023-07-23T17:43:54Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

+        MergeDictionaries(psi.Environment, wineEnvironmentVariables);
+        MergeDictionaries(psi.Environment, environment);
+
+// #if FLATPAK_NOTRIGHTNOW


Not sure about commenting out the code here, but don't have strong opinions on that... Maybe @goaaats has some ideas about it

marzent · 2023-07-23T17:49:32Z

src/XIVLauncher.Common.Unix/Compatibility/CompatibilityTools.cs

@@ -253,16 +283,46 @@ public Int32 GetProcessId(string executableName)
        return GetProcessIds(executableName).FirstOrDefault();
    }

-    public Int32 GetUnixProcessId(Int32 winePid)
+    public Int32 GetUnixProcessId(Int32 winePid, string executableName)


This was not my original suggestion (and makes this function harder to understand)
GetUnixProcessId maps a Wine PID to its Unix one... If winedbg doesn't have the necessary patch for this then the process name can be inferred from the wine PID by winedbg(and not passed as extra parameter) and be fed into GetUnixProcessIdByName

marzent · 2023-07-23T18:02:44Z

src/XIVLauncher.Common.Unix/UnixDalamudRunner.cs

+                throw new DalamudRunnerException("An internal Dalamud error has occured");
+            Console.WriteLine("[DALAMUD] " + output);         
+            dalamudErrorCount++;
+        } while (!output.StartsWith('{') && dalamudErrorCount <= 2);


This is a very crude check for valid JSON we should try to parse lines until they can be deserialized to a DalamudConsoleOutput, something like marzent@f50acf4 would be better IMO...
Although I generally dislike both of these solutions that issue should really not be happening in the first place, but I guess some configurations are special

marzent · 2023-07-23T18:03:31Z

src/XIVLauncher.Common.Unix/UnixDalamudRunner.cs


        new Thread(() =>
        {
            while (!dalamudProcess.StandardOutput.EndOfStream)
            {
                var output = dalamudProcess.StandardOutput.ReadLine();
                if (output != null)
-                    Console.WriteLine(output);
+                    Console.WriteLine("[DALAMUD] " + output);


This is confusing since it really is Injector and not Dalamud proper here (in any case best to leave it be)

marzent · 2023-07-23T18:05:02Z

src/XIVLauncher.Common.Unix/UnixDalamudRunner.cs

        try
        {
            var dalamudConsoleOutput = JsonConvert.DeserializeObject<DalamudConsoleOutput>(output);
-            var unixPid = compatibility.GetUnixProcessId(dalamudConsoleOutput.Pid);


If GetUnixProcessId is properly implemented all of this can be really left as as (save the JSON reading adjustment and spelling mistakes) to keep it simple

* fixed WineDLLOverrides * Renamed IsDxvk to Enabled * Improved logic for RunCommand for readability. * Uncommented #FLATPAK_NOTRIGHTNOW block * Removed second arg from GetUnixProcessId() * Added private function to find gameExe from winedbg info proc

…ole output

rankynbass · 2023-07-26T15:12:19Z

Closing and replacing with #1366. There were too many changes needed to rework this branch directly, and it was better to just start over from current master.

rankynbass added 8 commits June 17, 2023 00:59

First successful compile and launch. May need tweaking

4a3f2d6

Did a better LD_PRELOAD merge

6b698e3

Removed old constants from compatibilitytools

6e29c1d

Move more logic to runners

4d62a15

More work done

99f8d75

Move WineRunner, DxvkRunner to FFQL repo

0634482

Add unpatched wine fix

b0a8bd3

Move MacVideoFix to xlcore

c37927c

rankynbass mentioned this pull request Jun 24, 2023

Compatibility rework goatcorp/XIVLauncher.Core#59

Closed

Blooym approved these changes Jun 25, 2023

View reviewed changes

rankynbass added 3 commits June 24, 2023 19:01

Fixed wrong folder in DxvkRunner.cs

d8e66ce

Revert "Move MacVideoFix to xlcore"

78e02be

This reverts commit c37927c. It's better to keep all this on the XL side.

Merge branch 'master' into compatibility-rework

5c02352

marzent suggested changes Jun 27, 2023

View reviewed changes

rankynbass added 2 commits June 27, 2023 21:30

Forgot to fix Environment assignment in Settings classes

9284ecb

Fixed broken logic for finding wine or wine64.

6b73619

rankynbass added 5 commits June 28, 2023 22:40

Fixed missing !

4624e0a

Clean up the Ensure() functions, moved duplicate

22b5b08

code to DownloadTool().

Fix logic in GetUnixProcessId(), error handling of UnixDalamudRunner

3076a12

Add workaround for REST & proton wine

a16e3b1

No need for winedbg command if winePid is 0.

c07806f

rankynbass mentioned this pull request Jul 6, 2023

[xlcore][fix] Allow new versions of patched proton wine to work. Improve compatibility with unpatched wine. #1316

Closed

Re-added global LD_PRELOAD so it gets merged.

ffef16d

marzent reviewed Jul 23, 2023

View reviewed changes

rankynbass added 2 commits July 23, 2023 13:15

Code cleanup per marzents coments

11cc916

* fixed WineDLLOverrides * Renamed IsDxvk to Enabled * Improved logic for RunCommand for readability. * Uncommented #FLATPAK_NOTRIGHTNOW block * Removed second arg from GetUnixProcessId() * Added private function to find gameExe from winedbg info proc

Merge in marzent/FFXIVQuickLauncher@f50acf4, remove [DALAMUD] in cons…

1f697f3

…ole output

rankynbass added 2 commits July 23, 2023 14:17

Get rid of unnecessary LD_PRELOAD merge.

e7794d3

Rename some vars to make their purpose more apparent.

ec316fe

rankynbass mentioned this pull request Jul 26, 2023

Compatibility rework 2 #1366

Open

rankynbass closed this Jul 26, 2023

rankynbass deleted the compatibility-rework branch March 15, 2024 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility rework #1348

Compatibility rework #1348

rankynbass commented Jun 24, 2023 •

edited

Loading

marzent Jun 27, 2023

rankynbass Jun 27, 2023

marzent Jun 27, 2023

marzent Jun 27, 2023

marzent Jun 27, 2023

marzent Jun 27, 2023

marzent Jun 27, 2023

marzent Jun 27, 2023

marzent Jun 27, 2023

marzent Jun 27, 2023

marzent Jun 27, 2023

marzent Jul 23, 2023

rankynbass commented Jun 27, 2023 •

edited

Loading

rankynbass commented Jun 28, 2023

rankynbass commented Jun 29, 2023

marzent Jul 23, 2023

marzent Jul 23, 2023

marzent Jul 23, 2023

marzent Jul 23, 2023

marzent Jul 23, 2023

marzent Jul 23, 2023

marzent Jul 23, 2023

marzent Jul 23, 2023

marzent Jul 23, 2023

rankynbass commented Jul 26, 2023


		namespace XIVLauncher.Common.Unix.Compatibility;

		public class WineRunner


		public Dictionary<string, string> Environment { get; }

		public WineRunner(string runCmd, string runArgs, string folder, string url, string rootFolder, Dictionary<string, string> env = null, bool is64only = false)

Compatibility rework #1348

Compatibility rework #1348

Conversation

rankynbass commented Jun 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rankynbass commented Jun 27, 2023 • edited Loading

rankynbass commented Jun 28, 2023

rankynbass commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rankynbass commented Jul 26, 2023

rankynbass commented Jun 24, 2023 •

edited

Loading

rankynbass commented Jun 27, 2023 •

edited

Loading