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

Vulkan backend bindings #120

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from
Draft

Vulkan backend bindings #120

wants to merge 30 commits into from

Conversation

tlf30
Copy link
Contributor

@tlf30 tlf30 commented Mar 7, 2022

Description

This PR implements:

  • JNI bindings for the vulkan dear imgui backend
  • CI update to support linking against vulkan loader for building natives
  • A "safe" LWJGL wrapper for the bindings
  • An example demonstrating using the binding

Design considerations:

I created the "unsafe" bindings for the backend, then created a more "safe" wrapper in the imgui-lwjgl3 project.
I saw that instead of creating a binding for the opengl3 support, you created an equivalent implementation in java; I felt that for vulkan this was going to be too complex and created a wrapper instead.
Something to take note: The current imgui vulkan backend does work, but some parts of the API are still in development. The next imgui release will have better vulkan support, specifically for supporting user supplied images.

Type of change

Please check options that are relevant.

  • Minor changes or tweaks (quality of life stuff)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

TODO:

  • Implement Allocation callback
  • Implement VK result callback
  • Add example implementation/usage
  • Update README.md
  • Document in README how the vulkan binaries work with the build process.

Questions:

Hello @SpaiR, I have some questions for you on this one:

  • Do you want the imgui-app project to support both an opengl and vulkan backend?
  • At the moment, these changes require the vulkan sdk to be installed to build natives locally, is this OK?
  • Do you have a better way to setup the vulkan sdk in the ci pipeline?

Please let me know what you think. I am still working on getting the callbacks in place and putting together an example. I'm not super happy with the CI changes, but it does build. If you have any ideas on how to improve them, please let me know. In the GenerateLibs script, there is a hack to make finding the lib work for both Windows and Linux when building Windows natives. Again, not super happy with it as I feel it is fragile, but I do not know of any other way to do it. I did not implement building linux or macos natives from windows (I'm not even sure if that is possible as it is without my changes, never tried).
I wanted to get this in front of you as soon as I could in case you want me to take some of this in a different direction.

Thanks,
Trevor

@tlf30 tlf30 marked this pull request as draft March 7, 2022 02:17
@SpaiR SpaiR added the feat New feature or request label Mar 7, 2022
@SpaiR
Copy link
Owner

SpaiR commented Mar 7, 2022

Hi!

Not sure about imgui-app. It seems like it will be ok to keep it to use opengl by default, but to add some abstraction layer to provide a custom vulkan support. I believe that OpenGL is a more frequently used library at the moment, so it won't be a problem.

Speaking about the realisation, there is already an implementation for Dear ImGui with Vulkan: https://github.com/lwjglgamedev/vulkanbook/blob/master/bookcontents/chapter-15/chapter-15.md
I see the perfect solution is to port that implementation as it is and go with it. I agree that at the moment current changes to CI look a bit overcomplicated.

@tlf30
Copy link
Contributor Author

tlf30 commented Mar 7, 2022

I have pushed an example of the abstraction layer that I started for imgui-app. There is an enum BackendType which can be set during configuration to use VULKAN or OPENGL, the default is OPENGL.
I created a Backend interface, and moved the opengl specific code into an implementing class ImGuiGlBackend. I have tested this and it works as expected. Now I am in the process of implementing the vulkan backend. No natives are required to be packaged as vulkan uses the system's Vulkan Loader (and for MacOS LWJGL uses MoltenVK which it handles internally). The only additional overhead is adding the lwjgl vulkan jar to the dependencies list.

As for the Vulkan Book implementation, I spent several days last week attempting to make it work, without much sucess. I think with the changes that are going to be made in ImGui for how their vulkan support works, especially with textures, it would be best to use the provided backend instead of attempting to build our own. If we were to have a custom backend implementation is would be a lot of work to maintain it as ImGui made changes.

Example of my failed attempt at getting the Vulkan Book code to work:
image

Some things work, but the colors are incorrect and textures/fonts are completely broken.

I think I have found a way to simplify the CI pipeline. I will update it when I get some time.

@tlf30
Copy link
Contributor Author

tlf30 commented Mar 7, 2022

OK, so instead of downloading/building the vulkan libraries for linking, I have placed them in the /bin/vulkan folder in the project. I got the copies directly from the Lunarg vulkan sdk downloads. These do not get placed in any jar, they are only for linking against during build time. This simplifies the build process, and eliminates the need to the vulkan sdk to be installed to do local builds for natives.
Thoughts?

@SpaiR
Copy link
Owner

SpaiR commented Mar 7, 2022

Thoughts?

This is fine until CI builds it. 😅

As for the Vulkan Book implementation, I spent several days last week attempting to make it work, without much sucess. I think with the changes that are going to be made in ImGui for how their vulkan support works, especially with textures, it would be best to use the provided backend instead of attempting to build our own. If we were to have a custom backend implementation is would be a lot of work to maintain it as ImGui made changes.

Pretty good point.
Full java implementation is useful when it's required to customise rendering routine. For example, in one of my app Command pattern was used to collect render commands ImGuiImplGl was modified to use a custom GL instance, which collects commands instead of constantly calling them.
Is it possible for a solution you've provided?

@tlf30
Copy link
Contributor Author

tlf30 commented Mar 7, 2022

This is fine until CI builds it. 😅

Can you elaborate on the issue? Is there a different place that those binaries should be placed?

Full java implementation is useful when it's required to customise rendering routine. For example, in one of my app Command pattern was used to collect render commands ImGuiImplGl was modified to use a custom GL instance, which collects commands instead of constantly calling them.
Is it possible for a solution you've provided?

Yes, for OpenGL there has been no change to how the implementation works. That should still be fine.
For Vulkan, commands are already placed in the command buffer (that is just how vulkan works). It is then up to your code to provide the command buffer and execute it when desired. All imgui does is populate a command buffer you give it.

By adding the backend abstraction, I could also add a third option for a CUSTOM backend, then let the user provide their own implementation of the backend for imgui-app. I can add that real quick.

@SpaiR
Copy link
Owner

SpaiR commented Mar 7, 2022

Can you elaborate on the issue? Is there a different place that those binaries should be placed?

bin is fine. That is why the dir named like that. To make things more clear you can add some notice in the bin/README.txt. Describe what are those files and how they're used.

Yes, for OpenGL there has been no change to how the implementation works. That should still be fine.
For Vulkan, commands are already placed in the command buffer (that is just how vulkan works). It is then up to your code to provide the command buffer and execute it when desired. All imgui does is populate a command buffer you give it.

Ok so... It's not a matter of the PR, but how do you think, can we make something you've made for VK, but for OpenGL? With addressing the case I've described. I really love the idea to remove the manual implementation and shift responsibility for the rendering to Dear ImGui itself.

@tlf30
Copy link
Contributor Author

tlf30 commented Mar 7, 2022

Ok so... It's not a matter of the PR, but how do you think, can we make something you've made for VK, but for OpenGL? With addressing the case I've described. I really love the idea to remove the manual implementation and shift responsibility for the rendering to Dear ImGui itself.

Are you proposing using the opengl backend provided by ImGui? I think that it would be simple enough to implement it the same way that I did for the vulkan bindings. I would have to look at how the ImGui opengl backend works, but in the context of recording the commands as you described I think it is possible. I can investigate that after this PR (that keeps the number of changes here smaller).

@tlf30
Copy link
Contributor Author

tlf30 commented Mar 30, 2022

I've hit a bit of a road block and am trying to work around it.
For the native vulkan backend, I needed to implement the native glfw backend as there are key differences between its functionality and the current java conversion. It was very simple to build a binding for the glfw backend as it is only a handful of function calls. I did NOT want to build glfw for linking against right now, so I pulled the LWJGL binaries. But I am having issues that at runtime when the jvm tries to load the imgui-java.dll, it fails to find the glfw3.dll, take note the name as LWJGL's glfw dll is glfw.dll missing the 3 (although I am not sure if that is important or not...).

So the question I have right now, do you know how to tell the jvm to use the LWJGL glfw bindings?
I think it has something to do with: https://github.com/LWJGL/lwjgl3/blob/164fcdff248f9497d9e4a3fb9200f382949e5ad4/modules/lwjgl/glfw/src/generated/java/org/lwjgl/glfw/GLFW.java#L30.

Thanks,
Trevor

@tlf30
Copy link
Contributor Author

tlf30 commented May 18, 2022

@SpaiR After playing with this a bit more, I'm still not sure on the issue. The JVM is not recognizing that the native library has been loaded, I have force loaded the GLFW dll using GLFW.getLibrary(); to trigger the native library to be loaded from LWJGL prior to loading the native bindings, but the JVM still did not recognize the native as being loaded.

If you have any ideas, I would love to know.

Thanks,
Trevor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants