search

Dominaezzz / kgl

Thin multiplatform wrappers for graphics.
Apache License 2.0
106 stars 14 forks source link

add return values to global callback setters #27

Closed nlbuescher closed 4 years ago

nlbuescher commented 4 years ago

Glfw.setErrorCallback, Glfw.setJoystickCallback, and Glfw.setMonitorCallback (the last one is needed for imgui-docking) were not returning the previous callbacks. I missed those with the last fix, and this time I double checked that all functions named set…Callback have a return type.

Dominaezzz commented 4 years ago

Immutability (freezing) and threading on the native target might need a bit more thought.

nlbuescher commented 4 years ago

Probably. @ThreadLocal is probably not the best solution (that was already there). I usually put all the mutable properties top-level and have references to them in the object if they need to be public.

nlbuescher commented 4 years ago

A thought: (1) is it possible to detect if running on the main thread in native, and (2) should an exception be thrown if it's possible and a GLFW call is not running on main?

Dominaezzz commented 4 years ago

About (1), assuming main thread means, the thread in which GLFW is initialised, yeah there are a couple ways. You can store Worker.current and compare against future Worker.current calls or you can have a global @Threadlocal variable and do the same thing with it's hashCode() (as supposed to Worker.current).

About (2), I'm a bit sceptical about it but it's easier than dealing with frozen callbacks.

nlbuescher commented 4 years ago

Personally, I would circumvent freezing the callbacks by storing them top-level (above the Glfw object). I have a suspicion that that may cause memory leaks, but honestly, in this specific instance, that doesn't matter since the Glfw object is intended to be in scope globally until the program exits, and functions more as a namespace than anything else. I'd add cleanup code to Glfw.terminate (eg, set callbacks to null to release the references for GC), and call it a day

Another point to consider is how multithreading with GLFW is handled in a C/C++ context.

Overall though the native handling of multithreading is a necessary discussion, but I'm not sure it applies to this pull-request, since that wasn't one of the things I changed.

Dominaezzz commented 4 years ago

Oh, I just noticed that the callbacks are already being stored. I was reading the JVM version when I was thinking about it.

Dominaezzz commented 4 years ago

Thanks for the PR!

Dominaezzz commented 4 years ago

Published in 0.1.9-dev-10