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.17k stars 39 forks source link

DXVK Async: Make DXVK Async inherit from DXVK #269

Closed sonic2kk closed 11 months ago

sonic2kk commented 11 months ago

Another code refactor like #262, but this time for DXVK and DXVK Async.

This PR allows DXVK Async to inherit from the DXVK ctmod, removing a lot of the repeated code between the two ctmods. The ctmods were so similar that no changes at all were needed to the DXVK ctmod, which is why there's only a diff for DXVK Async.

If you compare the code for DXVK and DXVK Async, they are almost identical. Even the archive formats for releases are identical and have been for several releases at least (DXVK releases and DXVK Async releases), they both use .tar.gz.

In fact, the only difference between the ctmods (aside from the ctmod URLs being different) is that DXVK checks where it should be extracted to because it now supports Heroic. Before that they were very similar.

Another benefit of this change is that it would make it trivial to add Heroic support for DXVK Async. We won't need to copy the extraction directory check or change any other logic, as that is all taken care off by the parent DXVK ctmod. All we would need to do is add 'heroicwine', 'heroicproton' to the CT_LAUNCHERS list for DXVK Async. The only reason I didn't do it in this PR is because it may be a good idea to test this separately, though I'd be happy to add it for this PR as well.

So the main benefits to this PR are:

  1. Less duplicated code, always nice to see :smile:
  2. Makes it easy to extend support to Heroic (or other launchers in future) as the logic only has to be added to the parent class

There may be other reasons why this is not desired, such as a preference to have explicitly different ctmods for projects from different repos. But if it is desired, it would probably be possible to extend this kind of inheritance to other DXVK ctmods as well, such as DXVK Nightly and D8VK. That might require a little bit more work more akin to the recent changes for vkd3d (extraction logic changes, etc). This pattern may also make it easier to add support for other DXVK variations, such as DXVK Async-gpl (#243) and D8VK stable. This PR could be considered a "proof-of-concept" for applying this inheritance pattern to DXVK.

Hope my explanation and rationale made sense.

Thanks! :smile:

DavidoTek commented 11 months ago

That makes sense, they are in fact the same :)

such as DXVK Nightly and D8VK

Yes, they are a bit different as they require artifacts from nightly.link. It should be possible to implement a "universal nightly.link downloader" though.

such as DXVK Async-gpl (https://github.com/DavidoTek/ProtonUp-Qt/issues/243)

Having base classes that handle similar tools seems to be a good idea, that makes adding tools much easier. I wonder how well we could just override the methods for use with Gitlab (maybe add a __fetch_gitlab_data to the child class and then override get_tool, just an idea).


Thanks.

PS: Did some quick testing, the PR seems to work.

sonic2kk commented 11 months ago

I wonder how well we could just override the methods for use with Gitlab (maybe add a __fetch_gitlab_data to the child class and then override get_tool, just an idea).

I was wondering something similar :-) In regards to this and and a universal nightly.link downloader, I had actually looked into something along these lines: Creating some "unified" helper methods for downloading files with requests. The main "issue" I came across with implementing a feature like this is that the __download methods in ctmods do things like update the progress bar or check if the download has been cancelled.

I think having more methods for repeated code in ctmods that we can call/override is a great idea. Maybe for GitHub/GitLab we could have a single __fetch_data method, and then in that method we can check if a URL is GitHub or GitLab. It's something to think about down the road :-)

DavidoTek commented 11 months ago

I think having more methods for repeated code in ctmods that we can call/override is a great idea.

class CtInstaller(QObject):

I noticed that most super classes still inherit from QObject. We could create a "real" super class SuperCtInstaller from which all ctmods inherit from.

That could implement the download functionallity, fetch_data for GitHub and GitLab and maybe even get_extract_dir (though that would need an additional parameter so it knows whether it's dealing with dxvk, vkd3d, proton or wine)