Added missing functions for compatibility with Vulkan #273

zergon321 · 2020-02-23T22:40:21Z

Vulkan-go has its own fork of GLFW, but it's missing gamepad bindings which I really need. I've added some necessary functions from there to the original repository.

pchampio

Haven't tested the PR. Just some nitpicking comments ;)
Can you provide a simple main.go app to test this?

pchampio · 2020-02-24T09:14:32Z

v3.3/glfw/go.mod

@@ -1,3 +1,3 @@
-module github.com/go-gl/glfw/v3.3/glfw
+module github.com/tracer8086/glfw/v3.3/glfw


s/tracer8086/go-gl

Should I add it after module directive?

replace tracer8086 with go-gl ;)
Otherwise you are breaking go modules

pchampio · 2020-02-24T09:15:31Z

v3.3/glfw/vulkan.h

+
+// Retrieves the address of glfwGetInstanceProcAddress. This workaround is necessary because
+// CGO doesn't allow referencing C functions in Go code.
+void* getVulkanProcAddr();


add a newline to make git happy

pchampio · 2020-02-24T09:19:40Z

v3.3/glfw/window.go

@@ -201,6 +201,12 @@ func GoWindow(window unsafe.Pointer) *Window {
 	return &Window{data: (*C.GLFWwindow)(window)}
 }

+// GLFWWindow returns a *C.GLFWwindow reference (i.e. the GLFW window itself). This can be used for


There is already .Handle() for this purpose.

glfw/v3.3/glfw/window.go

Lines 191 to 196 in 6f7a984

// Handle returns a *C.GLFWwindow reference (i.e. the GLFW window itself).

// This can be used for passing the GLFW window handle to external libraries

// like vulkan-go.

func (w *Window) Handle() unsafe.Pointer {

return unsafe.Pointer(w.data)

}

zergon321 · 2020-02-24T17:06:57Z

@pchampio You can now test the PR using this repository which is a fork of the original vulkan-go demos repository.

pwaller · 2020-03-01T21:32:31Z

Thanks for your effort. I'd like to see this support added, of course.

However, I'm not keen on pulling in ~thousands of lines of code whose origin is not precisely known. Please take a look at my efforts over at #271 to make its so that GLFW is pulled from the source dictated by a specific commit, for example.

Having looked at this briefly, my understanding is that vulkan.h was generated by this repository at some revision: https://github.com/KhronosGroup/Vulkan-Docs -- I'm guessing you took the vulkan-go/glfw fork of this repository, which added a vulkan.h, but the commit in that repository doesn't precisely specify where the code came from either.

However, if you run the master branch of Vulkan-Docs, the code you get is quite different, it's no longer all in one header file but split into different headers for different platforms.

@tracer8086 Would you be willing to take a look at generating the vulkan headers from a commit? If I can run a short script whose behaviour is to fetch files from the official repository (or generate them if appropriate), then I would be happy.

zergon321 · 2020-03-04T18:31:52Z

@pwaller Sorry, I'm not sure I can do that, all of that is really convoluted for me. If this PR is merged, you could do everything else you need.

pwaller · 2020-03-09T15:43:11Z

@tracer8086 it seems you have missed my key constraint which is that I don't want to merge >4k lines of code whose provenance is not precisely known.

What should someone do if they want to update that code to fix a bug in it, for example? Where do they get the more recent version? How can I verify the license is correct?

I'm afraid that "Just merge this and you can do everything else you need" isn't a great answer in this case. Personally I have only very limited time to spare for this and I'm only here to help the general health of this project, not an active developer. In that way I'm acting as a gatekeeper to your change, and I apologise for that, I know it can be frustrating.

I do not require that you do everything I ask. Just that you tell me exactly how I can get the source code that you've provided me here, in a way that I can reproduce with minimal time and effort, and deduce that what you did doesn't cause harm to this project. You didn't provide a link to your source for example.

Put another way: how would you feel about using it if this publicly developed project pulled in ~4k lines of code from random people on the internet without scrutiny? At the moment that is what I perceive you're asking me (a volunteer with no commercial interests), or another person on this project in similar circumstances, to do.

zergon321 · 2020-03-09T18:23:53Z

@pwaller I just took the vulkan.h file from here. Actually, my purpose was just to make go-gl/glfw compatible with vulkan-go/vulkan.

pwaller · 2020-03-09T20:21:24Z

However, that is not the source of the file, @xlab presumably obtained that file from somewhere else. @xlab can you share where these files came from?

@xlab: I see in this commit you added "So expect this repo to be deleted, probably around end of Feb 2017." I take it you intended to upstream support for Vulkan into this repository. Am I right? Do you intend to?

xlab · 2020-03-10T15:21:28Z

@pwaller
Oh hi there! 👋

The file vulkan.h is part of the official distribution found there:
https://github.com/KhronosGroup/Vulkan-Headers/tree/master/include/vulkan

This is 100% official Vulkan API header distribution source. The file from vulkan-go/glfw/blob/v3.3 is simply outdated, but I don't think features of glfw/3.3 required something new, so I didn't update that file since 2016.

Regarding "expect this repo to be deleted". The whole concern of https://github.com/vulkan-go/glfw was because the GLFW v3.3 that introduced Vulkan API support has not been released officially at that time. And the date has been postponed many times — right until 15 Apr 2019!

So github.com/go-gl/glfw supported only v3.2 without Vulkan, and long anticipated v3.3 update PR was there - #256

In that PR maintainers and contributors decided that Vulkan support should be in a separate PR, as it may introduce additional problems and dependencies.

So I believe @tracer8086 just tries to make that PR adding Vulkan support, based on my commits from https://github.com/vulkan-go/glfw. I totally don't have much time to help now, but I really appreciate the efforts.

Let me know if there are questions.

pwaller · 2020-03-10T17:06:17Z

I don't currently understand why vulkan.h belongs in go-gl/glfw. It doesn't yet make sense to me.

Surely the system-installed header should be used consistently across all packages? It seems to me that really this functionality belongs in go-vulkan, no?
I understand that a call to glfwCreateWindowSurface is needed.
- Can this not be done from that other package?
I realise we want to avoid introducing a compile-time dependency on the vulkan headers to glfw.

It seems to me there must be a way of achieving this without actually copying the header into go-gl/glfw, which would be ideal.

xlab · 2020-03-10T17:45:06Z

glfw3.h can also be installed in the system, however this package includes it, meaning that it doesn't depend on pkg-config and is go-getable.

we want to avoid introducing a compile-time dependency on the vulkan headers to glfw

Why is that exactly? "GLFW is an Open Source, multi-platform library for OpenGL, OpenGL ES and Vulkan development on the desktop. It provides a simple API for creating windows, contexts and surfaces, receiving input and events.".

I think there is confusion behind your purpose of github.com/go-gl/glfw. That's not a binding to vanilla platform-provided GL, that's a binding to GLFW, which is a multi-purpose and multi-platform abstraction wrapper for underlying graphics and input devices.

An the original GLFW lib does support Vulkan, and also provides own set of Vulkan headers (for GLAD), including vulkan.h, see the sources:

https://github.com/glfw/glfw/releases/download/3.3.2/glfw-3.3.2.zip

pwaller · 2020-03-10T21:44:07Z

Sorry, I think I wrote something confusing since I was lacking in time to write my thoughts more clearly.

I'm concerned that this PR adds a vulkan.h header. If there is one already present by way of the glfw headers, why is adding one necessary?

I note that the number of symbols actually used from those headers is very small, and that glfw internal.h already has a minimal set of vulkan symbols in order to be able to define glfwCreateWindowSurface.

My general concern is of having header files which conflict and cause confusing and difficult to diagnose failures. This could arise through multiple means. The best solution I have in mind at the moment is that go-gl/glfw doesn't ship any additional vulkan.h files beyond what is provided by glfw/glfw or the system.

I can still be persuaded otherwise, so long as someone else can show they understand these issues reasonably well.

glfw3.h can also be installed in the system, however this package includes it, meaning that it doesn't depend on pkg-config and is go-getable.

Yes, however, this package is implementing glfw for Go, in some sense. And it was a design decision to provide glfw/glfw internally to this package so that users would not have to have any external compile-time dependencies available if they wanted to import ".../glfw". If we start shipping additional things (vulkan headers), then this is additional scope for which I think some care should be taken.

we want to avoid introducing a compile-time dependency on the vulkan headers to glfw

Why is that exactly?

Apologies, this was the thing which I wrote hastily, so it came out as a half-baked thought. What I think we want to avoid is to require users to apt install libvulkan-dev, for example. At least, if we add such a dependency it should be opt-in (e.g. through build tags) so that we don't break existing builds.

All that aside, as far as I can tell the only reason for having the 4kloc header is to be able to make use of the symbol glfwCreateWindowSurface. If I define the following that seems to do the trick:

#include "glfw/include/GLFW/glfw3.h"

typedef enum VkResult
{
    VK_SUCCESS = 0,
    VK_NOT_READY = 1,
    VK_TIMEOUT = 2,
    VK_EVENT_SET = 3,
    VK_EVENT_RESET = 4,
    VK_INCOMPLETE = 5,
    VK_ERROR_OUT_OF_HOST_MEMORY = -1,
    VK_ERROR_OUT_OF_DEVICE_MEMORY = -2,
    VK_ERROR_INITIALIZATION_FAILED = -3,
    VK_ERROR_DEVICE_LOST = -4,
    VK_ERROR_MEMORY_MAP_FAILED = -5,
    VK_ERROR_LAYER_NOT_PRESENT = -6,
    VK_ERROR_EXTENSION_NOT_PRESENT = -7,
    VK_ERROR_FEATURE_NOT_PRESENT = -8,
    VK_ERROR_INCOMPATIBLE_DRIVER = -9,
    VK_ERROR_TOO_MANY_OBJECTS = -10,
    VK_ERROR_FORMAT_NOT_SUPPORTED = -11,
    VK_ERROR_SURFACE_LOST_KHR = -1000000000,
    VK_SUBOPTIMAL_KHR = 1000001003,
    VK_ERROR_OUT_OF_DATE_KHR = -1000001004,
    VK_ERROR_INCOMPATIBLE_DISPLAY_KHR = -1000003001,
    VK_ERROR_NATIVE_WINDOW_IN_USE_KHR = -1000000001,
    VK_ERROR_VALIDATION_FAILED_EXT = -1000011001,
    VK_RESULT_MAX_ENUM = 0x7FFFFFFF
} VkResult;

typedef void* VkInstance;
typedef struct VkAllocationCallbacks VkAllocationCallbacks;
typedef uint64_t VkSurfaceKHR;

GLFWAPI VkResult glfwCreateWindowSurface(VkInstance instance, GLFWwindow* window, const VkAllocationCallbacks* allocator, VkSurfaceKHR* surface);

Would it work to delete the header and use the above instead?

v3.3/glfw/vulkan.go

v3.3/glfw/vulkan.c

pwaller · 2020-03-10T21:52:15Z

@tracer8086 I currently think this is close now, with the above suggestions. Sorry for the hassle. If you can respond to/test those suggestions (and if they work, implement them), I will respond by merging your changes reasonably promptly if I can.

zergon321 · 2020-03-10T22:06:06Z

@pwaller Ok, I will implement and test all the suggestions in the nearest free time.

xlab · 2020-03-11T08:23:56Z

The best solution I have in mind at the moment is that go-gl/glfw doesn't ship any additional vulkan.h files beyond what is provided by glfw/glfw or the system.
If we start shipping additional things (vulkan headers)

@pwaller I still have to insist that official GLFW started to ship Vulkan headers from 3.3. The reason why this package doesn't ship them is because GLFW prior to 3.3 didn't ship them. I mean, this could be the perfect time to decide which features of GLFW 3.3 require those headers and how to properly integrate them.

pwaller · 2020-03-11T17:45:50Z

@xlab please can you point at the files to which you refer? The only dependencies I see on glad are in glfw's tests, I didn't see an example of glfw providing an interface onto vulkan or glad, except via glfwCreateWindowSurface, as we've discussed so far. (And as far as I currently understand, using that doesn't seem to require committing glfw headers into this repository).

Another constraint in my mind is that we are actually pulling glfw from upstream verbatim -- see my in-progress effort in #271, which doesn't do much other than clone from remote. So my point that this commit shouldn't be adding headers stands -- we're already taking the v3.3 sources as they exist in upstream glfw/glfw as far as I understand. If you can point at something specific where I am wrong please do!

This reverts commit 877dda8.

…h."" This reverts commit 59c6a9d.

zergon321 · 2020-03-11T18:55:05Z

@pwaller @xlab I discovered something that's really great. We don't need that vulkan.h with ~4k lines at all, all the necessary stuff to run Vulkan applications is located here. So I just used functions from this file. All the demos work good with these changes. I'm going to update the PR.

v3.3/glfw/glfw/deps/vulkan/vk_platform.h

I presume keeping this file was an oversight.

Using both resulted in warnings of redefined macros. It seems to me that using the internal header is sufficient.

pwaller · 2020-03-11T21:09:32Z

With the current approach I get the following warnings:

# github.com/go-gl/glfw/v3.3/glfw
In file included from ./vulkan.go:5:0:
./glfw/src/internal.h:86:0: warning: "GL_VERSION" redefined
 #define GL_VERSION 0x1f02
 
In file included from ./glfw/include/GLFW/glfw3.h:210:0,
                 from ./vulkan.go:4:
/usr/include/GL/gl.h:655:0: note: this is the location of the previous definition
 #define GL_VERSION    0x1F02
 
In file included from ./vulkan.go:5:0:
./glfw/src/internal.h:90:0: warning: "GL_EXTENSIONS" redefined
 #define GL_EXTENSIONS 0x1f03
 
In file included from ./glfw/include/GLFW/glfw3.h:210:0,
                 from ./vulkan.go:4:
/usr/include/GL/gl.h:656:0: note: this is the location of the previous definition
 #define GL_EXTENSIONS    0x1F03
 
In file included from ./vulkan.go:5:0:
./glfw/src/internal.h:91:0: warning: "GL_NUM_EXTENSIONS" redefined
 #define GL_NUM_EXTENSIONS 0x821d
 
In file included from /usr/include/GL/gl.h:2050:0,
                 from ./glfw/include/GLFW/glfw3.h:210,
                 from ./vulkan.go:4:
/usr/include/GL/glext.h:896:0: note: this is the location of the previous definition
 #define GL_NUM_EXTENSIONS                 0x821D
 
In file included from ./vulkan.go:5:0:
./glfw/src/internal.h:92:0: warning: "GL_CONTEXT_FLAGS" redefined
 #define GL_CONTEXT_FLAGS 0x821e
 
In file included from /usr/include/GL/gl.h:2050:0,
                 from ./glfw/include/GLFW/glfw3.h:210,
                 from ./vulkan.go:4:
/usr/include/GL/glext.h:897:0: note: this is the location of the previous definition
 #define GL_CONTEXT_FLAGS                  0x821E
 
In file included from ./vulkan.go:5:0:
./glfw/src/internal.h:101:0: warning: "GL_CONTEXT_RELEASE_BEHAVIOR" redefined
 #define GL_CONTEXT_RELEASE_BEHAVIOR 0x82fb
 
In file included from /usr/include/GL/gl.h:2050:0,
                 from ./glfw/include/GLFW/glfw3.h:210,
                 from ./vulkan.go:4:
/usr/include/GL/glext.h:2584:0: note: this is the location of the previous definition
 #define GL_CONTEXT_RELEASE_BEHAVIOR       0x82FB
 
In file included from ./vulkan.go:5:0:
./glfw/src/internal.h:102:0: warning: "GL_CONTEXT_RELEASE_BEHAVIOR_FLUSH" redefined
 #define GL_CONTEXT_RELEASE_BEHAVIOR_FLUSH 0x82fc
 
In file included from /usr/include/GL/gl.h:2050:0,
                 from ./glfw/include/GLFW/glfw3.h:210,
                 from ./vulkan.go:4:
/usr/include/GL/glext.h:2585:0: note: this is the location of the previous definition
 #define GL_CONTEXT_RELEASE_BEHAVIOR_FLUSH 0x82FC

I have fixed this in 7ad7349.

@tracer8086 please confirm you're happy with the changes I made to your PR (which I did since I saw you had 'enabled maintainers to make changes', and thought we could save some round-tripping on some simple things) and I will squash merge this.

zergon321 · 2020-03-11T22:19:42Z

@pwaller Yeah, I confirm I'm happy with the changes. Everything is good, thank you.

pwaller · 2020-03-11T22:55:27Z

So I think there remains one outstanding thing from the conversation so far: @xlab are you happy with the changes as they are, or have we missed something big?

A last thing I noticed is unsafe.Pointer appearing in the API. It stands out that reflect is used to obtain one of the c-pointers (instance) but unsafe.Pointer the other c-pointer (allocCallbacks) within the same function definition[0]. Is there a good reason to have two ways of achieving the same thing there?

[0] (And further that the return type is a uintptr, which also feels like a no-no but I realise is likely to be safe in this context since it doesn't involve a garbage-collected pointer)

pwaller · 2020-04-19T11:00:43Z

Sorry for the delay. Ping @xlab: any further input before we merge this one?

I'll try to merge this within a week if there is no further input.

xlab · 2020-04-19T11:35:48Z

LGTM! :)
Amazing work

pwaller · 2020-04-19T15:26:56Z

👍 thanks for your effort @zergon321 and thanks for everyone's input!

zergon321 added 3 commits February 24, 2020 00:33

Added Vulkan support.

9f3c53c

Module path has been changed.

237b934

GLFWWindow function added.

8a00403

zergon321 requested a review from pwaller February 24, 2020 00:12

pchampio suggested changes Feb 24, 2020

View reviewed changes

zergon321 added 3 commits February 24, 2020 19:29

Removed unnecessary function.

e3f2c2c

Added a neline to make git happy.

702f1df

Restored original go.mod.

d8ef9b8

pchampio approved these changes Feb 28, 2020

View reviewed changes

pwaller reviewed Mar 10, 2020

View reviewed changes

v3.3/glfw/vulkan.go Outdated Show resolved Hide resolved

pwaller reviewed Mar 10, 2020

View reviewed changes

v3.3/glfw/vulkan.go Outdated Show resolved Hide resolved

pwaller reviewed Mar 10, 2020

View reviewed changes

v3.3/glfw/vulkan.c Outdated Show resolved Hide resolved

zergon321 added 3 commits March 11, 2020 20:58

glfw/src/internal.h used instead of glfw/deps/vulkan.h.

877dda8

Revert "glfw/src/internal.h used instead of glfw/deps/vulkan.h."

59c6a9d

This reverts commit 877dda8.

Revert "Revert "glfw/src/internal.h used instead of glfw/deps/vulkan.…

344b9a9

…h."" This reverts commit 59c6a9d.

glfwGetInstanceProcAddress reference added.

02caf43

pwaller reviewed Mar 11, 2020

View reviewed changes

v3.3/glfw/glfw/deps/vulkan/vk_platform.h Outdated Show resolved Hide resolved

pwaller added 2 commits March 11, 2020 20:56

Delete vk_platform.h

9246b8f

I presume keeping this file was an oversight.

Show error code in returned glfwCreateWindowSurface

24b9b66

pwaller force-pushed the add-vulkan-functions branch from 87b9dc7 to 24b9b66 Compare March 11, 2020 21:05

Use only the internal header

7ad7349

Using both resulted in warnings of redefined macros. It seems to me that using the internal header is sufficient.

pwaller merged commit bf0707b into go-gl:master Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added missing functions for compatibility with Vulkan #273

Added missing functions for compatibility with Vulkan #273

zergon321 commented Feb 23, 2020

pchampio left a comment

pchampio Feb 24, 2020

zergon321 Feb 24, 2020

pchampio Feb 24, 2020 •

edited

Loading

pchampio Feb 24, 2020

pchampio Feb 24, 2020

zergon321 commented Feb 24, 2020

pwaller commented Mar 1, 2020

zergon321 commented Mar 4, 2020

pwaller commented Mar 9, 2020

zergon321 commented Mar 9, 2020

pwaller commented Mar 9, 2020

xlab commented Mar 10, 2020 •

edited

Loading

pwaller commented Mar 10, 2020

xlab commented Mar 10, 2020 •

edited

Loading

pwaller commented Mar 10, 2020 •

edited

Loading

pwaller commented Mar 10, 2020

zergon321 commented Mar 10, 2020

xlab commented Mar 11, 2020

pwaller commented Mar 11, 2020

zergon321 commented Mar 11, 2020

pwaller commented Mar 11, 2020

zergon321 commented Mar 11, 2020

pwaller commented Mar 11, 2020

pwaller commented Apr 19, 2020

xlab commented Apr 19, 2020

pwaller commented Apr 19, 2020

		@@ -1,3 +1,3 @@
		module github.com/go-gl/glfw/v3.3/glfw
		module github.com/tracer8086/glfw/v3.3/glfw

	// Handle returns a *C.GLFWwindow reference (i.e. the GLFW window itself).
	// This can be used for passing the GLFW window handle to external libraries
	// like vulkan-go.
	func (w *Window) Handle() unsafe.Pointer {
	return unsafe.Pointer(w.data)
	}

Added missing functions for compatibility with Vulkan #273

Added missing functions for compatibility with Vulkan #273

Conversation

zergon321 commented Feb 23, 2020

pchampio left a comment

Choose a reason for hiding this comment

pchampio Feb 24, 2020

Choose a reason for hiding this comment

zergon321 Feb 24, 2020

Choose a reason for hiding this comment

pchampio Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

pchampio Feb 24, 2020

Choose a reason for hiding this comment

pchampio Feb 24, 2020

Choose a reason for hiding this comment

zergon321 commented Feb 24, 2020

pwaller commented Mar 1, 2020

zergon321 commented Mar 4, 2020

pwaller commented Mar 9, 2020

zergon321 commented Mar 9, 2020

pwaller commented Mar 9, 2020

xlab commented Mar 10, 2020 • edited Loading

pwaller commented Mar 10, 2020

xlab commented Mar 10, 2020 • edited Loading

pwaller commented Mar 10, 2020 • edited Loading

pwaller commented Mar 10, 2020

zergon321 commented Mar 10, 2020

xlab commented Mar 11, 2020

pwaller commented Mar 11, 2020

zergon321 commented Mar 11, 2020

pwaller commented Mar 11, 2020

zergon321 commented Mar 11, 2020

pwaller commented Mar 11, 2020

pwaller commented Apr 19, 2020

xlab commented Apr 19, 2020

pwaller commented Apr 19, 2020

pchampio Feb 24, 2020 •

edited

Loading

xlab commented Mar 10, 2020 •

edited

Loading

xlab commented Mar 10, 2020 •

edited

Loading

pwaller commented Mar 10, 2020 •

edited

Loading