DavidoTek / ProtonUp-Qt

Install and manage GE-Proton, Luxtorpeda & more for Steam and Wine-GE & more for Lutris with this graphical user interface.
https://davidotek.github.io/protonup-qt
GNU General Public License v3.0
1.27k stars 40 forks source link

Lutris: Add vkd3d sources #142

Closed sonic2kk closed 1 year ago

sonic2kk commented 1 year ago

Implements #73

Overview

Adds support for downloading vkd3d for Lutris. Like with manual DXVK version downloads, this has to be entered manually in the Advanced Options for a Lutris game, but this is general Lutris usage and would apply if this was done manually as well :-)

Two sources are added here: vkd3d-proton (the project used by Valve in Proton) and vkd3d-lutris (a fork of this project with seemingly minimal changes, at least at the time of writing) - The latter is just a Lutris fork of upstream vkd3d-proton simply called "vkd3d" - This is slightly further behind than regular vkd3d-proton, and releases as a .tar.xz instead of a .tar.zst like vkd3d-proton does.

The benefit of having both is the ability to use the latest vkd3d release from upstream vkd3d-proton and using it "raw", in case there are any changes in the downstream Lutris build. Upstream is on v2.7, Lutris is on v2.6. A brief skim of the commits appears like there are no real changes in the fork, maybe they just host separately for packaging? I am not sure of the reason, but both should work. The folder structure is identical.

This change does not impact Steam, as this is irrelevant to Steam.

image image

Implementation

Two Ctmods

I considered taking a similar approach to what I did with SteamTinkerLaunch: Having one "main" ctmod for vkd3d, and then having a separate one for vkd3d-lutris. Since I thought the code would be much the same, just different URLs.

It turns out the URLs are different, and some of the code needed for vkd3d-proton was different. The release asset extensions are different and the extraction and paths would be different. Since they are in reality two separate repositories, I figured having two separate ctmods made the most sense in the end and would be cleaner as there wouldn't be a bunch of conditional checks.

Zstandard Library

Okay, the main "problem" with this implementation. Upstream vkd3d-proton is releases as a .tar.zst file, and this is not supported by the Python tarball library. There is a stale request for this upstream, but I wouldn't hold my breath on this. And even so, ProtonUp-Qt does not use the latest Python version, so a version bump would be required to make use of that.

To implement this, I used the Zstandard Python library and mostly followed an implementation from a GitHub Gist. It basically uses this Zstandard library to extract the tar data to a temporary tarfile, which is then extracted properly using the tarfile library.

The problem is that this adds an extra dependency. Creating a PR that adds an extra dependency is probably not very helpful, and I am really sorry about that. I did investigate to see if there was a way to do this without pulling in an extra dependency, but that is how I came across the upstream Python issue.

If there is a way to fix this, I would be happy to remove this dependency.

Remaining Issues

The only remaining issue with this PR is that the vkd3d downloads don't show up in the compatibility tool list. I figure the Lutris runner vkd3d directory just needs to be added somewhere in the code, but I couldn't figure out where to put it.

Apart from this issue, I am happy with the state of the PR and think it's ready for review.


Phew, I was meant to take a break but I couldn't resist trying to tackle this :sweat_smile: As always, feedback is appreciated! :-)

DavidoTek commented 1 year ago

Two Ctmods

I think it makes sense to have two ctmods.

Zstandard Library

I don't think we have an alternative to using the Zstandard Python library. It will increase the package a bit, but the binary inside the package contains debug info which can be stripped to reduce the size to around 940KB (Flatpak and AppImage Builder automatically strip the binaries AFAIK). I think that is fine

The only remaining issue with this PR is that the vkd3d downloads don't show up in the compatibility tool list

It's a bit hacky, but DXVK tools are separately added to the list here: https://github.com/DavidoTek/ProtonUp-Qt/blob/99b0c15a15288a256dd847aa9c1b33258104f424/pupgui2/pupgui2.py#L181-L186
(If this gets "too hacky" we may need add a get_installed_versions method to each ctmod, but I think it's fine for now)


Phew, I was meant to take a break but I couldn't resist trying to tackle this :sweat_smile: As always, feedback is appreciated! :-)

I know that all to well. Once you've started something, it's hard to take a break for a while until every idea is put down :smile: As always, thanks!!

sonic2kk commented 1 year ago

Thank you! I did an initial draft by just copying the code to display DXVK in the list, but it seemed very repetitive and also fairly straightforward to break out into a function, so I did just that :-)

def get_installed_versions(self, ctool_name, ctool_dir):
    for ct in get_installed_ctools(ctool_dir):
        if not ctool_name in ct.get_displayname().lower():
            ct.displayname = f'{ctool_name} {ct.displayname}'
        self.compat_tool_index_map.append(ct)

I created a function called get_installed_versions that takes the ctool_name (i.e., dxvk, vkd3d) and the ctool_dir and adds them to the ctool list. This seems to work, but if there is any problem with this I can revert this and just do a straight copy of the DXVK code like:

# DXVK
for ct in get_installed_ctools(dxvk_dir):
    if not 'dxvk' in ct.get_displayname().lower():
        ct.displayname = 'DXVK ' + ct.displayname
    self.compat_tool_index_map.append(ct)

# vkd3d-proton
for ct in get_installed_ctools(vkd3d_dir):
    if not 'vkd3d' in ct.get_displayname().lower():
        ct.displayname = 'vkd3d ' + ct.displayname
    self.compat_tool_index_map.append(ct)

This is how the ctool list looks now :-) (most of these came with Lutris, but you can see the vkd3d-proton version that I downloaded)

image

DavidoTek commented 1 year ago

Thanks, that looks good. I will take a final look if something else needs to be considered and merge it tomorrow.

@DavidoTek Flatpak: Requires extra permissions for the Lutris vkd3d folder: https://github.com/flathub/net.davidotek.pupgui2/blob/5cb5991c01c43de539ad4d99aea74bb49d76d012/net.davidotek.pupgui2.json#L16-L20

DavidoTek commented 1 year ago

I'm not sure how well tempfile works inside Flatpaks. There was an issue a while back where the temporary directory has no space left.

I will test it and if needed change the folder, that should be simple to do.

PS: Is tempfile actually creating a file on disk or using some sort of ramdisk?

sonic2kk commented 1 year ago

Ah sorry, that could be a problem, I didn't even think of that!

From research, tempfile creates an actual file I believe. The default location varies between systems but on my system (and I presume the vast majority of distros) it uses /tmp. You can change this with tempfile.tempdir (https://docs.python.org/3.8/library/tempfile.html#tempfile.tempdir). It also seems like TemporaryFile can take a dir parameter: https://docs.python.org/3.8/library/tempfile.html#tempfile.TemporaryFile

So if there is a preferred Flatpak location or a better location, we could set it in either of these ways. Maybe setting the tempfile location would be better if we want different locations for Flatpaks vs everything else, and passing it in the TemporaryFile constructor could be the cleanest if we want the same location all the time :-)

This StackOverflow answer has some more background too: https://stackoverflow.com/questions/18280245/where-does-python-tempfile-writes-its-files

DavidoTek commented 1 year ago

All right. I will modify the code later.

DavidoTek commented 1 year ago

I made some changes to the code:

Will merge. Thanks! :tada:

sonic2kk commented 1 year ago

I removed the tempfile and pathlib dependency and made sure the temporary .tar file is created in the temp_dir provided by the get_tool method

Nice! :smile:

I cleaned the code a bit, for instance removed destination=destination which is remnant from the original ProtonUp-Qt ctmods (and is present in almost all ctmod :laughing:)

Hehe, I noticed this but I didn't want to touch too much of the other code just in case it was added as a specific workaround or for another reason.

Thanks and glad I could help out some more :smile: