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

3.3 beta - don't merge yet #219

Closed
wants to merge 18 commits into from
Closed

3.3 beta - don't merge yet #219

wants to merge 18 commits into from

Conversation

tapir
Copy link
Member

@tapir tapir commented Aug 4, 2018

Got rid of internal error callback. Never liked the way we solved it back then. It was not idiomatic and it was not mirroring the C API (worst of both worlds?). Now we have glfwGetError() function along with callback setting ability. It's still not idiomatic (only way to do it idiomatically is to return errors from all functions) but at least it mirrors the C API now. As always I'm open to suggestions. Any opinions?

I've also noticed that the 2 joystick functions we have were accepting the joystick IDs as arguments whereas we could have made it receivers. As our aim is to make the library as idiomatic as possible I've changed it. With 3.3 we have many more joystick functions so I think it looks better now. What do you think?

As a side note, I see no reason to implement the Vulkan functions. Thanks to Window.Handle() vulkan-go packages can access the GLFW window handle and create the necessary surface. So as is, it's ready for Vulkan.

README.md has the changelog as always, if you want to see the details.

Looks complete now according to this: http://www.glfw.org/docs/3.3/news.html
Did I miss anything?

@kivutar
Copy link

kivutar commented Oct 7, 2018

@tapir On OSX 10.14 with

	glfw.WindowHint(glfw.ContextVersionMajor, 3)
	glfw.WindowHint(glfw.ContextVersionMinor, 2)
	glfw.WindowHint(glfw.OpenGLProfile, glfw.OpenGLCoreProfile)
	glfw.WindowHint(glfw.OpenGLForwardCompatible, glfw.True)
	glfw.WindowHint(glfw.TransparentFramebuffer, glfw.True)

I'm getting:

# github.com/go-gl/glfw/v3.3/glfw
ld: framework not found MoltenVK
clang: error: linker command failed with exit code 1 (use -v to see invocation)
# github.com/go-gl/glfw/v3.3/glfw
native_darwin.c:7:17: warning: implicit declaration of function 'glfwGetCocoaWindow' is invalid in C99 [-Wimplicit-function-declaration]
native_darwin.c:7:9: warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]
native_darwin.c:11:17: warning: implicit declaration of function 'glfwGetNSGLContext' is invalid in C99 [-Wimplicit-function-declaration]
native_darwin.c:11:9: warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]

Has MoltenVK become a mandatory dependency?

What about the other errors in native_darwin.c?

@Noofbiz
Copy link

Noofbiz commented Oct 7, 2018

Yes, Mojave (10.14) has depreciated opengl in favor of molten.

@kivutar
Copy link

kivutar commented Oct 31, 2018

I understand that GL is deprecated. But Elmindreda told me on IRC that GLFW could also be used with Metal and that GL will continue to work on OSX even if they are not going to add newer versions and that a few things will break.

go-gl/glfw/v3.2 is go getable without the need to install any additional library on the system.

Molten is not yet something that one can easily install on OSX, there are no brew packages.

@tapir
Copy link
Member Author

tapir commented Oct 31, 2018

I have no knowledge at all about OSX. I hate it with passion. So somebody else needs to take the wheel there. @dmitshur maybe?

@dmitshur
Copy link
Member

dmitshur commented Oct 31, 2018

GLFW could also be used with Metal

Indeed it can. I'm currently using GLFW v3.2 on macOS with Metal here.

So somebody else needs to take the wheel there. @dmitshur maybe?

Sure, I'll help with the macOS side of things. I don't know when I'll have free time, but I'll try to get to it.

v3.2/glfw does not require MoltenVK framework as far as I know, so v3.3/glfw shouldn't either. @kivutar, does v3.2/glfw work okay for you?

@kivutar
Copy link

kivutar commented Nov 1, 2018

Yes v3.2/glfw works well with OpenGL on OSX. Especially on High Sierra. On Mojave there are a few bugs, a vsync bug, and a context update bug, declared in the GLFW tracker. These two bugs have workarounds. I'm using it here.

@mokiat
Copy link

mokiat commented Nov 15, 2018

Has anyone managed to get this pull request running on MacOS?

Now that with Mojave GLFW 3.2 is no longer running, I decided to give the GLFW 3.3 (be it unstable) a try. I am hitting the same problem as @kivutar :

# github.com/go-gl/glfw/v3.3/glfw
ld: framework not found MoltenVK
clang: error: linker command failed with exit code 1 (use -v to see invocation)
# github.com/go-gl/glfw/v3.3/glfw
native_darwin.c:7:17: warning: implicit declaration of function 'glfwGetCocoaWindow' is invalid in C99 [-Wimplicit-function-declaration]
native_darwin.c:7:9: warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]
native_darwin.c:11:17: warning: implicit declaration of function 'glfwGetNSGLContext' is invalid in C99 [-Wimplicit-function-declaration]
native_darwin.c:11:9: warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]

I downloaded Vulkan SDK and put the MoltenVK.framework folder in /Library/Frameworks. I also tried to set a bunch of environment variables like PATH and DYLD_LIBRARY_PATH to what I believe is correct. Unfortunately, I keep getting the same error. Any ideas?

@Noofbiz
Copy link

Noofbiz commented Nov 23, 2018

I actually have a gist here https://gist.github.com/Noofbiz/de83517b848f153750a43346dbdcd3e9 that uses sdl to call vulkan and draw a triangle. I'll try switching to glfw 3.3 tomorrow to see if it works. Since I can confirm that vulkan is working on Mojave it'll let us know if glfw is the issue

@Noofbiz
Copy link

Noofbiz commented Nov 23, 2018

Alright, so adding -F/Library/Frameworks to the linker options successfully adds Library/Frameworks to the ld path, so that error goes away. However after doing that I get

Undefined symbols for architecture x86_64:
  "__glfwCreateContextEGL", referenced from:
      __glfwPlatformCreateWindow in _x004.o
  "__glfwInitEGL", referenced from:
      __glfwPlatformCreateWindow in _x004.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
# github.com/go-gl/glfw/v3.3/glfw
native_darwin.c:7:17: warning: implicit declaration of function 'glfwGetCocoaWindow' is invalid in C99 [-Wimplicit-function-declaration]
native_darwin.c:7:9: warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]
native_darwin.c:11:17: warning: implicit declaration of function 'glfwGetNSGLContext' is invalid in C99 [-Wimplicit-function-declaration]
native_darwin.c:11:9: warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]

@gravypod
Copy link

@tapir are you planning on exposing more of the functions required for Vulkan? In your comment you said that "As a side note, I see no reason to implement the Vulkan functions. Thanks to Window.Handle() vulkan-go packages can access the GLFW window handle and create the necessary surface" but this doesn't make sense to me. Vulkan requires access to quite a few functions of GLFW for detecting required extensions, loading the vulkan function pointers, and many more features. Will this be addressed in this branch?

I know there's a divergent fork of the code base maintained by vulkan-go but I don't think it's good to fracture the implementation of these bindings. I've already run into multiple bugs on the vulkan-enabled fork of this library that have been patched by this team.

@tapir
Copy link
Member Author

tapir commented Jan 1, 2019

@kivutar looks like that one was already implemented:

// IsGamepad returns whether the specified joystick is both present and

Joystick functions are now methods. Please see 3.3 changelog.

@tapir
Copy link
Member Author

tapir commented Feb 17, 2019

Reminder to implement #234

@dmitshur
Copy link
Member

Got rid of internal error callback. Never liked the way we solved it back then. It was not idiomatic and it was not mirroring the C API (worst of both worlds?). Now we have glfwGetError() function along with callback setting ability. It's still not idiomatic (only way to do it idiomatically is to return errors from all functions) but at least it mirrors the C API now. As always I'm open to suggestions. Any opinions?

I'm open to discussion too, but so far, this doesn't look like an improvement to me.

You said:

It was not idiomatic and it was not mirroring the C API (worst of both worlds?).

Can you elaborate in what way it was not idiomatic? A Go function that can fail returning a (*Thing, error) seems pretty idiomatic to me.

Let's look at the diff of the example in README:

@@ -20,7 +20,7 @@ package main
 
 import (
 	"runtime"
-	"github.com/go-gl/glfw/v3.2/glfw"
+	"github.com/go-gl/glfw/v3.3/glfw"
 )
 
 func init() {
@@ -30,15 +30,14 @@ func init() {
 }
 
 func main() {
-	err := glfw.Init()
-	if err != nil {
-		panic(err)
+	if !glfw.Init() {
+		panic(glfw.GetError())
 	}
 	defer glfw.Terminate()
 
-	window, err := glfw.CreateWindow(640, 480, "Testing", nil, nil)
-	if err != nil {
-		panic(err)
+	window := glfw.CreateWindow(640, 480, "Testing", nil, nil)
+	if window == nil {
+		panic(glfw.GetError())
 	}

This looks like the Go API is becoming worse. In addition to that, it's a breaking API change.

I don't use the C GLFW API very often and I'm very used to the Go glfw API, so maybe I'm biased.

@AntyMew
Copy link

AntyMew commented Apr 22, 2019

Just my 2 cents, but I have to agree with @dmitshur. Errors in Go should be self-documenting. An error return type always means said function may fail, but scrapping error in favor returning a type which may potentially return nil is more ambiguous and more dangerous.

Besides, these changes would make the API unable to make use of the current Go 2 error handling draft.

@tapir
Copy link
Member Author

tapir commented Apr 23, 2019

It's been a while since this PR was made, now I think it's not necessary to break the API in such a huge way eventhough we're allowed to do breaking changes with new versions. So I'm OK with putting that away.

I'm not going to be around for couple weeks so maybe someone can take this PR over?

Things left to do are

  • Check for new methods implemented on GLFW side since this PR was made.
  • Check issue list for fixes postponed until 3.3
  • Make sure vulkan's working. I've ommited Vulkan specific things but we've had reports that they are necessary. Last time I've checked vulkan-go's glfw fork was not so different.
  • Re-base

@kivutar
Copy link

kivutar commented Apr 23, 2019

Could we also check that it can still build without MoltenVK on OSX? For people who are still using GL.

@GeertJohan
Copy link
Contributor

FWIW, I agree with @dmitshur and @AntyMew.
Go functions should return errors when they occurred. I really like that go-glfw converts the C-style error checking to Go-style multiple return values, which is really what a Go to C wrapper should do. I also agree that it's good to have consistency, but isn't it possible to consistently return errors when they occurred in a call? Is there a performance reason to not fetch the error automatically? (errors aren't really on the hot-path / happy-flow, right?)

@GeertJohan
Copy link
Contributor

GeertJohan commented May 18, 2019

Can I help by PR'ing #234 onto the 3.3-beta branch?

@kivutar
Copy link

kivutar commented Jun 19, 2019

I was able to compile and use this branch on OSX without Vulkan by doing the following modifications:

In v3.3/glfw/c_glfw_darwin.go, add #include "glfw/src/egl_context.c"
In v3.3/glfw/build.go, remove -framework MoltenVK and -D_GLFW_VULKAN_STATIC.

So there must be a way to detect if MoltenVK is there and apply these changes at preprocessor time.

Maybe using pkg-config?

@Tskken
Copy link

Tskken commented Aug 2, 2019

Hello I was wondering if there has been any updates on ether removing Vulkan and MultenVk from v3.3 or fixing the current errors on mac as it is still breaking for me even after trying some of the fixes here. I am wanting to try and use this version to try and make a Rainmeter like program that is cross platform but this current breaking issue is making it not possible on mac.

@xlab
Copy link
Contributor

xlab commented Aug 18, 2019

removing Vulkan and MultenVk from v3.3

No way, there is the only point of waiting for official 3.3 release from GLFW and go-gl.

@kivutar
Copy link

kivutar commented Aug 18, 2019

@xlab Vulkan is not the only point. There are also important bug fixes, and other features like the joypad database.

@pwaller
Copy link
Member

pwaller commented Sep 12, 2019

I'm not going to be around for couple weeks so maybe someone can take this PR over?

My understanding is that this PR needs someone to take over the work. Is that right @tapir? Any volunteers?

pekim added a commit to pekim/dull that referenced this pull request Sep 22, 2019
It turns out that glfw 3.2 does not support a window
in a headless environment.
Apparently glfw3.3 does have support, but go-glfw does not
support 3.3 yet. (go-gl/glfw#219)
@kivutar
Copy link

kivutar commented Oct 5, 2019

@tapir I don't see hints like GLFW_COCOA_RETINA_FRAMEBUFFER or GLFW_COCOA_MENUBAR in your PR. They are not usable in the current state of your PR.

Vulkan stuff should be optional, using tags, like here:

#cgo linux,vulkan LDFLAGS: -lvulkan

@pchampio pchampio mentioned this pull request Nov 2, 2019
5 tasks
@pchampio
Copy link
Contributor

pchampio commented Nov 2, 2019

Please consider #256

pwaller pushed a commit that referenced this pull request Nov 24, 2019
GLFW tag 3.3-stable @ glfw/glfw@e4e9581

* Fix C Include & address GLFW v3.3 removals
* Glfw moved away from posix_tls.c to posix_thread.c (this reflects that they now contain more than TLS)
* Glfw moved away from win32_tls.c to win32_thread.c (same as above)
* Glfw removed wayland-.*-unstable-v1-client-protocol.c
* Glfw added wl_platform.h
* Glfw removed GLFW_USE_RETINA, _GLFW_USE_CHDIR and _GLFW_USE_MENUBAR compile-time macros
* Glfw removed support for MIR
* Address deprecations of glfw 3.3
* Glfw deprecate glfwSetCharModsCallback
* Glfw deprecate the window parameter of glfwGetClipboardString and
  glfwSetClipboardString, NULL can be used as the window argument
* Add glfw.SetClipboardString(string) and glfw.GetClipboardString()
  function (no Window type required)
* Use casting and clang-format c code
* Use casting instead of callback in C helpers (same as #219)
* Update travis
* Fix Darwin build tag

Fixes: #245, #255, #240, #230, #170
@pwaller
Copy link
Member

pwaller commented Nov 24, 2019

@pchampio kindly took over maintenance of this PR which has now landed over in #256

@pwaller pwaller closed this Nov 24, 2019
@pwaller pwaller deleted the 3.3-beta branch November 24, 2019 19:55
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.