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

Add a GraphicsPropertiesFetcher class #2071

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

ImperatorS79
Copy link
Contributor

Fetch:

  • Graphics card vendor and name
  • OpenGL version
  • Vulkan version

Limitation : cannot distinguish 32 bits and 64 bits support.

@ImperatorS79
Copy link
Contributor Author

How do I format the code correctly ?

@ImperatorS79
Copy link
Contributor Author

ImperatorS79 commented Aug 25, 2019

Hum, I have currently a bug when I launch two times a script using this. The second time it cannot load the OpenGL library :/.

@ImperatorS79
Copy link
Contributor Author

Fixed 😉.

@ImperatorS79
Copy link
Contributor Author

What should I use for the exception ?

<activation>
<os>
<family>unix</family>
<arch>amd64</arch>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we limiting us in any way if we specify amd64 here?

Copy link
Contributor Author

@ImperatorS79 ImperatorS79 Aug 25, 2019

Choose a reason for hiding this comment

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

This comes straight from LWJGL website so I do not know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On linux and macOS LWJGL is only available in 64 bits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What would happen if you run the code with Linux 32 bit or an older mac?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it would not work, obviously. De we really want to support such old architecture ? If so, the only possible solution is to write that as an external C++ code or to simply use glxinfo/vulkaninfo as stated in #2061.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should try to support as many platforms as we comfortably can. In my opinion it is likely that quite a lot of people still use older 32 bit hardware, with an older Linux 32 bit OS, but i think we should ask for some more opinions, @plata @qparis what do you think about this?

@madoar
Copy link
Collaborator

madoar commented Aug 25, 2019

What exception do you mean?

@ImperatorS79
Copy link
Contributor Author

Codacy is not happy due to RuntimeException. I also have an IllegalStateException. That code comes straight fro LWJGL start guide. I do not know what I should do here.

@ImperatorS79
Copy link
Contributor Author

I am also interested if other people could test it by adding:

.preInstall(function() {
        const graphicsProperties = Bean("graphicsPropertiesFetcher").getProperties();
        print(graphicsProperties.vendor);
        print(graphicsProperties.renderer);
        print(graphicsProperties.openGLVersion);
        print(graphicsProperties.vulkanVersion);
    });

in the 7-zip script for example. My output looks like:

Intel Open Source Technology Center
Mesa DRI Intel(R) HD Graphics 630 (Kaby Lake GT2) 
3.0
1.1.0

@madoar
Copy link
Collaborator

madoar commented Aug 25, 2019

The IllegalStateException is fine. The RuntimeException is bad because it is quite generic. Replace it with an IllegalStateException as well and you should be fine I think.

@ImperatorS79
Copy link
Contributor Author

Seems maven cannot find LWJGL on macos, but it should 🤔.

@ImperatorS79
Copy link
Contributor Author

ImperatorS79 commented Aug 27, 2019

Now with this

.preInstall(function() {
        const graphicsProperties = Bean("graphicsPropertiesFetcher").getProperties();
        print(graphicsProperties.vendor);
        print(graphicsProperties.renderer);
        print(graphicsProperties.openglVersion);
        print(graphicsProperties.openglCoreVersion);
        print(graphicsProperties.vulkanVersion);
    });

in the 7-zip script for example, you should get something like this:

Intel Open Source Technology Center
Mesa DRI Intel(R) HD Graphics 630 (Kaby Lake GT2) 
3.0
4.5
1.1.0

@Zemogiter, @madoar, @plata feel free to test and report here 😁 👍.

@Zemogiter
Copy link
Contributor

I'll test it in DXVK/D9VK/VK9 PRs once #2067 is merged.

@ImperatorS79
Copy link
Contributor Author

I would like to know, before continuing here, how we want to handle the graphics properties fetching.
It is really a useful tool IMO (for example to set VK_ICD_FILENAME).

@madoar
Copy link
Collaborator

madoar commented Oct 3, 2019

We need to have a solution that either:

  • works for all supported operating systems/architectures
  • works only for a subset of the supported OSes/architectures, but doesn't crash on other OSes/architectures

Correct me if I'm wrong but I think your proposed solution does not support 32 bit architectures and even leads to a crash on it.

@ImperatorS79
Copy link
Contributor Author

It does not work on pure/only 32 bits architecture simply because LWJGL is 64 bits only on linux.
If we do not use a pure java solution the other solution is to use (on linux) glxinfo and vulkaninfo (adding mesa-utils and vulkanutils as deps)

@madoar
Copy link
Collaborator

madoar commented Oct 3, 2019

I agree that writing a wrapper for these functions is something we should not do. Therefore my option two, i.e. simply do a check in the fetcher class where you check the architecture and OS. If the OS or architecture are unsupported by the used library return an object containing "unsupported" values

@ImperatorS79
Copy link
Contributor Author

ImperatorS79 commented Oct 3, 2019

I agree that writing a wrapper for these functions is something we should not do

What do you mean ? ^^

simply do a check in the fetcher class where you check the architecture and OS

I do not know how it works in java, but how can I compile without LWJGL on unsupported platform ?

@madoar
Copy link
Collaborator

madoar commented Oct 3, 2019

I agree that writing a wrapper for these functions is something we should not do

What do you mean ? ^^

We have two choices if we want to provide these functions for 32 bit:

  • we use some other applications which return the result in text format (I think glxinfo and vulkaninfo go in this direction)
  • we write a small C/C++ library to fetch the necessary information, compile it for all necessary platforms and integrate it via JNI

Both solutions have their (large) issues. The first one requires that the used applications are available. The second one requires C and JNI. Both areas I would like to circumvent if we can.

simply do a check in the fetcher class where you check the architecture and OS

I do not know how it works in java, but how can I compile without LWJGL on unsupported platform ?

I expect maven to work an an unsupported platform as well. It just won't add LWJGL to the build output. This means that if you actually try to access one of its methods during runtime you will stumble over an error. Therefore we need to execute a check before any LWJGL methods are called to ensure that the library is actually available.

@ImperatorS79
Copy link
Contributor Author

And how do I do that check ?

@madoar
Copy link
Collaborator

madoar commented Oct 3, 2019

Another important note: I think there is no more Java 32bit version for a while now. So this may actually be the reason why LWJGL does not support 32bit?

@ImperatorS79
Copy link
Contributor Author

@madoar I do not know why the packaging fails on macOS, the pom.xml is directly taken from lwjgl website. If you have any idea...

@madoar
Copy link
Collaborator

madoar commented Oct 5, 2019

I have no idea as well. Maven suggests that all files exist (see https://repo1.maven.org/maven2/org/lwjgl/lwjgl-vulkan/3.2.2/ for example). Maybe the cache used by travis is out of date. I don't know if we can force an update manually. Maybe updating lwjgl to version 3.2.3 helps?

@ImperatorS79
Copy link
Contributor Author

ImperatorS79 commented Oct 5, 2019

Now it fails under macOS with

[ERROR]     'dependencies.dependency.version' for org.lwjgl:lwjgl:jar:${lwjgl.natives} is missing. @ org.phoenicis:phoenicis-tools:[unknown-version], /Users/travis/build/PhoenicisOrg/phoenicis/phoenicis-tools/pom.xml, line 146, column 21

[ERROR]     'dependencies.dependency.version' for org.lwjgl:lwjgl-glfw:jar:${lwjgl.natives} is missing. @ org.phoenicis:phoenicis-tools:[unknown-version], /Users/travis/build/PhoenicisOrg/phoenicis/phoenicis-tools/pom.xml, line 151, column 21

[ERROR]     'dependencies.dependency.version' for org.lwjgl:lwjgl-opengl:jar:${lwjgl.natives} is missing. @ org.phoenicis:phoenicis-tools:[unknown-version], /Users/travis/build/PhoenicisOrg/phoenicis/phoenicis-tools/pom.xml, line 156, column 21

Again I just copy-paste the pom.xml from lwjgl website

@plata
Copy link
Collaborator

plata commented Dec 28, 2019

What's the status here?

@ImperatorS79
Copy link
Contributor Author

I do not know why the macOS build fails. The lines added to the pom come directly from lwjgl website, and it works on linux.

@plata
Copy link
Collaborator

plata commented Dec 28, 2019

@qparis could you check the Mac OS build?

@qparis
Copy link
Member

qparis commented Dec 28, 2019

Ok, I'll have a look once I have managed to build wine 32on64. This is a difficult task that is taking most of the time I'm devoting to Phoenicis at the moment

@ImperatorS79
Copy link
Contributor Author

Ok I think I found where the error came from. Arch for macos is x86_64 instead of amd64. Which lwjgl website did not mention.

@plata
Copy link
Collaborator

plata commented Feb 17, 2020

What's the state of this pr?

@ImperatorS79
Copy link
Contributor Author

It compiles everywhere and works under linux. Needs to be tested on macos.

@madoar
Copy link
Collaborator

madoar commented Feb 17, 2020

It compiles everywhere and works under linux. Needs to be tested on macos.

I think it may be worth it to just merge this as is. If later on someone can test it on mac and finds out that it does not work it is still not too late to add a separate solution for macos. @plata what do you think?

@plata
Copy link
Collaborator

plata commented Feb 17, 2020

It's fine for me.

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.

5 participants