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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change mangohud detection and fix mangohud loading #569

Merged
merged 3 commits into from Dec 11, 2022

Conversation

Jan200101
Copy link
Contributor

@Jan200101 Jan200101 commented Dec 8, 2022

Closes #91
Closes #165

In order of commits:

  • Re-adds setting LD_LIBARY_PATH to all upstream supported locations
  • replaces dlopen method of mangohud detection with a $PATH lookup for the wrapper

getExecutable should probably be offloaded to some sort of utilities class, but I hadn't found a proper location for it.

Upstream officially supports the use of $LIB/mangohud/

Signed-off-by: Jan200101 <sentrycraft123@gmail.com>
@getchoo
Copy link
Member

getchoo commented Dec 8, 2022

can solve #91 and #165

@Scrumplex
Copy link
Member

can solve #91 and #165

I think the latter issue also has a very interesting alternative to just hardcoding usr/local/$LIB/mangohud, as the path defined in the Vulkan layer manifest of MangoHud will probably be the more right one

@getchoo
Copy link
Member

getchoo commented Dec 9, 2022

yeah that would probably be best since that file is generated at build time and /usr isn't a guaranteed prefix

@flowln flowln added the bug Something isn't working label Dec 9, 2022
@Jan200101
Copy link
Contributor Author

Well, mangohud installs the vulkan layers into $PREFIX/share/vulkan/implicit_layer.d.
How can we detect the system prefix?

Here are my suggestions:

  • detect mangohud wrapper and check relatively to it (e.g. ../$LIB/mangohud/
  • extract LD_LIBRARY_PATH from wrapper
  • use mangohud wrapper

@Scrumplex
Copy link
Member

How can we detect the system prefix?

As described in this comment we can rely on the layer being present in XDG_DATA_DIRS. You probably wouldn't be able to use the MangoHud Vulkan layer if this wasn't the case, so it's as good as it gets. You could use QStandardPaths::locateAll with type QStandardPaths::GenericDataLocation to locate the file we are looking for (implicit_layer.d/MangoHud.json).
For systems where XDG_DATA_DIRS is not used, we can add some workarounds (like adding the predictable path on Flatpak to XDG_DATA_DIRS in our wrapper script)

@Jan200101
Copy link
Contributor Author

How can we detect the system prefix?

As described in this comment we can rely on the layer being present in XDG_DATA_DIRS.

Alright, so it appears that the spec gives us a set of predefined paths to look for, but it doesn't appear to be using XDG_DATA_DIRS.

Is using XDG_DATA_DIRS as an alternative to $HOME/.local/share supported by implementations of this?

You could use QStandardPaths::locateAll with type QStandardPaths::GenericDataLocation to locate the file we are looking for (implicit_layer.d/MangoHud.json).

This should work fine, but I worry that this wouldn't work in flatpak.

@Scrumplex
Copy link
Member

Qt should respect XDG_DATA_DIRS for QStandardPaths::GenericDataLocation. We could just set that env var in our Flatpak if Flatpak doesn't do it by itself.

@Jan200101
Copy link
Contributor Author

Jan200101 commented Dec 9, 2022

Is using XDG_DATA_DIRS as an alternative to $HOME/.local/share supported by implementations of this?

vulkan-loader uses XDG_DATA_DIRS for it and not $HOME/.local/share/, so the approach would work

We could just set that env var in our Flatpak if Flatpak doesn't do it by itself.

We could but I worry that this will have sideffects on Prismlauncher itself or other wrappers that may be used with it.

Worst case we can add our own env var along the lines of VULKAN_PREFIX and then resolve $VULKAN_PREFIX/share/vulkan/explicit_layer.d

EDIT: $VK_LAYER_PATH exists for this case.
while QStandardPaths::locateAll is neat, it will not work for this use case, because it cannot cover all paths supported by vulkan-loader.

@Jan200101 Jan200101 force-pushed the PR/fix-mangohud branch 2 times, most recently from bbc00ff to 499d98c Compare December 9, 2022 14:31
@Jan200101
Copy link
Contributor Author

Its now detecting the library path by looking for and reading out vulkan layer files.

launcher/MangoHud.cpp Outdated Show resolved Hide resolved
launcher/MangoHud.cpp Outdated Show resolved Hide resolved
launcher/MangoHud.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong button. Muscle memory be like

launcher/MangoHud.h Outdated Show resolved Hide resolved
launcher/MangoHud.cpp Outdated Show resolved Hide resolved
launcher/MangoHud.cpp Outdated Show resolved Hide resolved
launcher/CMakeLists.txt Show resolved Hide resolved
launcher/Application.cpp Outdated Show resolved Hide resolved
@Jan200101 Jan200101 force-pushed the PR/fix-mangohud branch 2 times, most recently from dcf1410 to 5c18c94 Compare December 11, 2022 09:59
Signed-off-by: Jan200101 <sentrycraft123@gmail.com>
Signed-off-by: Jan200101 <sentrycraft123@gmail.com>
@Scrumplex Scrumplex added this to the 6.0 milestone Dec 11, 2022
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@Scrumplex Scrumplex merged commit 7cc4226 into PrismLauncher:develop Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support MangoHud detection without LD_LIBRARY_PATH Mangohud is not detected
4 participants