Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lutris: Add vkd3d sources #142

Merged
merged 5 commits into from Nov 24, 2022
Merged

Lutris: Add vkd3d sources #142

merged 5 commits into from Nov 24, 2022

Conversation

sonic2kk
Copy link
Contributor

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 馃槄 As always, feedback is appreciated! :-)

@DavidoTek
Copy link
Owner

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:

if install_loc.get('launcher') == 'lutris':
dxvk_dir = os.path.join(install_directory(), '../../runtime/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)

(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 馃槄 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 馃槃 As always, thanks!!

@sonic2kk
Copy link
Contributor Author

sonic2kk commented Nov 23, 2022

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
Copy link
Owner

DavidoTek commented Nov 23, 2022

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

All right. I will modify the code later.

@DavidoTek
Copy link
Owner

I made some changes to the code:

  • 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
  • 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 馃槅)

Will merge. Thanks! 馃帀

@DavidoTek DavidoTek merged commit 3d95b1d into DavidoTek:main Nov 24, 2022
@sonic2kk
Copy link
Contributor Author

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! 馃槃

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 馃槅)

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 馃槃

@sonic2kk sonic2kk mentioned this pull request Nov 24, 2022
DavidoTek added a commit that referenced this pull request Nov 29, 2022
* Converted every string concatenation to f strings

* Converted open with "with open"

* Converted manual counter with enumerate

* Rewrote the code in a more pythonic way

* Refactory of the code

* Refactory of the #144 python code

* Updated to #142 + refactory of the new code

* This is the refactory :)

* Clean imports pt1

* Clean imports pt2: ctmods

Co-authored-by: DavidoTek <54072917+DavidoTek@users.noreply.github.com>
@sonic2kk sonic2kk deleted the vkd3d-proton branch February 3, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants