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

GLFW 3.3 #256

Merged
merged 12 commits into from Nov 24, 2019
Merged

GLFW 3.3 #256

merged 12 commits into from Nov 24, 2019

Conversation

@pchampio
Copy link
Contributor

pchampio commented Nov 2, 2019

This PR adds support for the latest stable version of GLFW (3.3-stable from github).

I've addressed the issues of #219 :

  • idiomatic Go function (returns (*Thing, error))

  • Check for new methods implemented on GLFW side since #219 was made. (http://www.glfw.org/docs/3.3/news.html)

  • Check issue list for fixes postponed until 3.3 (except #235)

  • Re-base

  • 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.

    (Not realy sure what to do here) Even if vulkan isn't very polished, important bug fixes, and other features are available.

I've renamed the v3.2 folder to v3.3 and then done the changes, this allows a 'cleaner' PR. I can also make this even cleaner with some filter (removes first commit)
I'll add the v3.2 folder back after everything is green on you side.

pchampio added 9 commits Nov 1, 2019
GLFW latest tag 3.3-stable
glfw/glfw@e4e9581
* Glfw moved away from posix_tls.c to posix_thread.c (This to reflect 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

More info -> https://www.glfw.org/docs/3.3/news.html#removals_33
* 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 instead of callback in C helpers (same as #219)
In order to compile using docker (cross-compiling linux to darwin).
I had to remove some Vulkan flag
@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 2, 2019

I'm pretty sure vulkan isn't working on darwin.
We need to set some compile flag for this platform.
This can be done in another PR!!!

I am runing my tests using one of 'my' project (https://github.com/go-flutter-desktop/hover | https://github.com/go-flutter-desktop/go-flutter).
hover allows for cross compiling, and I can assert that each platform does compile properly.

Here are an example of hover created application using go-gl/glfw v3.3:
windows: windows.zip
linux: linux.zip
macos: darwin.zip (7 days only, wetransfer link)

@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 2, 2019

A tag named 'v3.3/glfw/v0.1.0' needs to be pushed (and updated) for go modules to work.

@xlab

This comment has been minimized.

Copy link
Contributor

xlab commented Nov 4, 2019

I'm pretty sure vulkan isn't working on darwin.
We need to set some compile flag for this platform.

Hey, any specific error report for that? I'm pretty sure that for darwin it requires just one more framework -framework Metal -framework MoltenVK
https://github.com/vulkan-go/glfw/blob/v3.3/v3.3/glfw/build.go#L19

Is there something beyond that?

@xlab

This comment has been minimized.

Copy link
Contributor

xlab commented Nov 4, 2019

Amazing work, I hope this will be merged soon. I especially like the concept of having Vulkan in another PR. Let's merge GLFW mainline, then we'll consider how to handle vulkan part in separate PR.

@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 4, 2019

Hey, any specific error report for that? I'm pretty sure that for darwin it requires just one more framework -framework Metal -framework MoltenVK
https://github.com/vulkan-go/glfw/blob/v3.3/v3.3/glfw/build.go#L19

Is there something beyond that?

If I put those flags, then cross-compiling using Docker does not work anymore.


Keep in mind that those LD-flag should only be used when the builg tag 'vulkan' is set (like the README says)

/*
#cgo darwin,vulkan CFLAGS: -D_GLFW_VULKAN_STATIC
#cgo darwin,vulkan LDFLAGS: -F/Library/Frameworks -framework Metal -framework MoltenVK -framework IOSurface -framework QuartzCore
*/
import "C"

TODO (In another PR) port back vulkan functions from vulkan-go@2a913f7

@kivutar

This comment has been minimized.

Copy link

kivutar commented Nov 11, 2019

I'm not sure about the #include "glfw/src/linux_joystick.c" in the freebsd build. It gives me errors. Is it something new?

@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 11, 2019

What's your error?
And no it's not something new, the linux_joystick import hasn't changed, it was already here in GLFW 3.2.

If you look at the GLFW imports here the only available files are {cocoa,linux,null,win32}_joystick.*
Freebsd should use the Linux import.

@kivutar

This comment has been minimized.

Copy link

kivutar commented Nov 12, 2019

My error is about some missing symbols because of a missing linux specific header. I can't copy the error messages because I'm using VirtualBox.

These lines also suggest that this file is Linux specific: https://github.com/go-gl/glfw/pull/256/files#diff-e4ef75560f509962482af9c7dc7397e5R66-R69

But you're right that this is nothing new, and we may just ignore it for now.

@rcoreilly

This comment has been minimized.

Copy link

rcoreilly commented Nov 17, 2019

What is the status of 3.3 here? I see this PR, and a 3.3-beta branch which just received a commit -- are there two separate 3.3 versions? One way or another, it would be great to get 3.3 finally available after a really long time since the upstream lib was released. Thanks!

@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 17, 2019

Your comment is misleading.
The 3.3-beta branch (#219) hasn't received a any commit since last year. https://github.com/go-gl/glfw/pull/219/commits

As discussed on #219 (comment) and stated in my PR description, I'm taken over #219.

I haven't received any updates from the maintainers of go-gl. I've invested not payed time into this PR (like everyone😑), and I also would like it get merged before upstream lib diverge too much.

@rcoreilly

This comment has been minimized.

Copy link

rcoreilly commented Nov 17, 2019

Sorry I was confused -- I received an email notification about #257 which looked new but apparently it was just closed! And I mistook the name of that for the 3.3-beta branch.. Anyway, I guess the only valid remaining point is: when will this PR be merged!?

@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 17, 2019

I can see why you got confused, I realllllly hope this PR gets merged before the end of the year.
If not, even though I don't like the idea, I will certainly start my own fork 😒.

(I don't want to throw the work I've done, and I don't want to stress out open source maintainers, as I know how hard it can be)

Speaking of which, if you want to add me as a contributor for go-gl/glfw, I will be honored!

@kivutar

This comment has been minimized.

Copy link

kivutar commented Nov 18, 2019

@pchampio you may want to take a look at these lines for OSX https://github.com/go-gl/glfw/pull/219/files#diff-c114ff13290eefc1d62b13112e94f1e5R13-R19

I don't know what -D_GLFW_USE_RETINA does exactly, but maybe it would help some people.

You can certainly add -framework Metal safely.

However, since not everybody has -framework MoltenVK, adding this framework would better be done with a build flag. If not it would break go get for users that don't have MoltenVK installed.

I can help you test this branch on OSX at least for OpenGL, as I'm already maintaining my own fork for 3.3. I would prefer to not use a fork.

@hajimehoshi

This comment has been minimized.

Copy link
Member

hajimehoshi commented Nov 18, 2019

However, since not everybody has -framework MoltenVK, adding this framework would better be done with a build flag. If not it would break go get for users that don't have MoltenVK installed.

Agreed. MoltenVK should be optional.

@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 18, 2019

@pchampio you may want to take a look at these lines for OSX https://github.com/go-gl/glfw/pull/219/files#diff-c114ff13290eefc1d62b13112e94f1e5R13-R19

I don't know what -D_GLFW_USE_RETINA does exactly, but maybe it would help some people.

I've intentionally removed those line (as described in a643862 commit 😃) because those compile-time macros have been removed in 3.3. https://www.glfw.org/docs/3.3/news.html#removals_33

You can certainly add -framework Metal safely.

It breaks compatibility with MacOSX10.10.sdk.
I'm basing this conclusion on https://github.com/docker/golang-cross.

https://s3.dockerproject.org/darwin/v2/MacOSX10.10.sdk.tar.xz Doesn't not contain Metal.framework
while https://raw.githubusercontent.com/ndeloof/golang-cross/113fix/MacOSX10.11.sdk.tar.xz does.

I don't think -framework Metal is necessary. https://github.com/glfw/glfw/blob/2bac7ee8da526257d808bd026b027246c98e4f2f/CMakeLists.txt#L277-L297
Am I missing something?

@frou

This comment has been minimized.

Copy link

frou commented Nov 19, 2019

All Macs that can run 10.10 can also run 10.11, so in theory no one is stuck on 10.10

@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 19, 2019

@frou yes, I get it. But is the flag necessary?
And since I am using 'docker/golang-cross', I'm stuck with 10.10.

I'm not against adding this flag, I just want to know why I should add it. As far as I understand, it's not needed.

@xlab

This comment has been minimized.

Copy link
Contributor

xlab commented Nov 19, 2019

Afaik Vulkan works through Metal, so afaik it's only needed for MoltenVK to work.

@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 20, 2019

Sooo.., Is the PR ready to be reviewed?
Friendly ping @tapir, @pwaller, @dmitshur, @slimsag.

Copy link
Member

pwaller left a comment

@pchampio thanks very much for this effort, and everyone else who chimed in.

I'm low on time, but I have done my best to move all the bits through my eyeballs. It looks good to me, and thanks for your efforts on making it easy to review. I only found a couple of trivial things to comment on.

Moving forward, my intent would be to merge this with the "squash" feature on github so that we don't get the removal/addition of v3.2 in the git history.

I will give a couple of days in case anyone else has any comments, but unless there is a show stopper, I don't see any reason not to merge. I agree with the rationale "Vulkan may not be working, but this is a step forward". Future PRs can fix other things.

If there is no further input on this thread by Saturday 23rd Nov, then I will merge. If I don't merge then, ping the thread and I'll do it by the end of the day on Sunday -- and if I don't respond then, I am inundated, so please feel free to ping the thread weekly or so.

v3.3/glfw/window.go Outdated Show resolved Hide resolved
v3.3/glfw/window.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
e4e9581557826bd522158ab9159859e20e10700e

This comment has been minimized.

Copy link
@pwaller

pwaller Nov 20, 2019

Member

I note that this choice of revision is after the 3.3 tag.

Is that really what we want? Or do we want to stick to "released" versions? I could be persuaded either way.

This comment has been minimized.

Copy link
@pchampio

pchampio Nov 22, 2019

Author Contributor

On the glfw.org/download webpage, it is stated:

the 3.3-stable branch only adds bug fixes for patch releases.

I'm guessing we want the latest patch available in go-gl/glfw

The previous `SetIcon` go doc was more accurate, reverse the change.
Move Changelog bullets points description under the vXX specific changes
@pwaller pwaller merged commit e5ec435 into go-gl:master Nov 24, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pwaller

This comment has been minimized.

Copy link
Member

pwaller commented Nov 24, 2019

Merged. Great work, thanks!

@pchampio

This comment has been minimized.

Copy link
Contributor Author

pchampio commented Nov 25, 2019

Just for the sake of sharing numbers.
After a very fast investigation, here are some well stared project that rely on go-gl/glfw:

Thanks to the go-gl team ❤️ !
Putting the 5-6h of work into this PR was well worth-it!

@hajimehoshi

This comment has been minimized.

Copy link
Member

hajimehoshi commented Nov 26, 2019

hajimehoshi/ebiten has already switched to 3.3. @pchampio, thank you for your awesome work!

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Nov 26, 2019

Thank you @pchampio for authoring this PR, @pwaller for reviewing it, @tapir for creating the original PR #219, and everyone else who contributed to this change! 🎉 ❤️ 🚀

I've sent CL 208877 to update golang.org/x/exp/shiny/driver/mtldriver to use the new GLFW v3.3 library.

@kivutar

This comment has been minimized.

Copy link

kivutar commented Nov 26, 2019

libretro/ludo is also switching to 3.3. Thanks to all the people involved in this PR.

@rcoreilly

This comment has been minimized.

Copy link

rcoreilly commented Nov 26, 2019

Yep same with goki/gi and I can already say that it has made things more stable and functional! Thank you!

dmitshur added a commit to go-gl/example that referenced this pull request Nov 26, 2019
It has been released recently in go-gl/glfw#256. Start using it.
@dmitshur dmitshur mentioned this pull request Nov 26, 2019
dmitshur added a commit to goxjs/glfw that referenced this pull request Nov 26, 2019
It has been released recently. Start using it.

Updates go-gl/glfw#256
gopherbot pushed a commit to golang/exp that referenced this pull request Nov 27, 2019
GLFW v3.2 was old and started generating some warnings on macOS 10.15
Catalina (and warnings get upgraded to errors on builders). Update to
GLFW v3.3 that was recently released to resolve the problem.

There is a small regression affecting the newer version of GLFW that is
being tracked in issues glfw/glfw#1543 and
go-gl/glfw#262. It causes the application to
crash when glfwWaitEvents is called before the first window is opened:

	$ go run -tags='example metal' .../shiny/example/basic
	2019-11-25 22:19:24.715 basic[9412:69556] *** Assertion failure in -[NSApplication run], /BuildRoot/Library/Caches/com.apple.xbs/Sources/AppKit/AppKit-1894.10.126/AppKit.subproj/NSApplication.m:3313
	2019-11-25 22:19:24.715 basic[9412:69556] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'NSApp with wrong _running count'
	*** First throw call stack:
	(
		0   CoreFoundation                      0x00007fff30503f53 __exceptionPreprocess + 250
		1   libobjc.A.dylib                     0x00007fff665c9835 objc_exception_throw + 48
		2   CoreFoundation                      0x00007fff3051f810 +[NSException raise:format:arguments:] + 88
		3   Foundation                          0x00007fff32bff5d1 -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] + 191
		4   AppKit                              0x00007fff2d657ed3 -[NSApplication run] + 1007
		5   basic                               0x00000000040d473d _glfwPlatformCreateWindow + 77
		6   basic                               0x00000000040cd945 glfwCreateWindow + 485
		7   basic                               0x00000000040da71b _cgo_78603e0816ec_Cfunc_glfwCreateWindow + 43
		8   basic                               0x0000000004058430 runtime.asmcgocall + 112
	)
	libc++abi.dylib: terminating with uncaught exception of type NSException
	SIGABRT: abort
	PC=0x7fff67a7b49a m=0 sigcode=0
	signal arrived during cgo execution
	[...]

Work around it by waiting for the first window open request before
entering the main loop. That way, glfwWaitEvents is not called
before a window has been created, and the crash does not occur.
This temporary workaround can be removed after the bug is fixed.

Fixes golang/go#35766
Updates go-gl/glfw#256
Updates glfw/glfw#1543
Updates go-gl/glfw#262

Change-Id: Ie9b15ab6632af39871d94993a3e3097ea798a7e2
Reviewed-on: https://go-review.googlesource.com/c/exp/+/208877
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.