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_CONNECTED Joystick vs Monitor #170

Closed
cdelorme opened this issue Sep 18, 2016 · 5 comments
Closed

GLFW_CONNECTED Joystick vs Monitor #170

cdelorme opened this issue Sep 18, 2016 · 5 comments
Labels

Comments

@cdelorme
Copy link

Not sure if this is a bug so much as unexpected behavior given that the documentation says to refer to the documentation (eg. glfw's), which doesn't really help since go is not C.

This fails:

func (self *Game) joysticks(joy, event int) {
    if event == glfw.Connected {
        // setup joystick
        self.Debug("joystick %d connected...", joy)
    } else if event == glfw.Disconnected {
        // teardown joystick
        self.Debug("joystick %d disconnected...", joy)
    }
}

This does not:

func (self *Game) joysticks(joy, event int) {
    if event == int(glfw.Connected) {
        // setup joystick
        self.Debug("joystick %d connected...", joy)
    } else if event == int(glfw.Disconnected) {
        // teardown joystick
        self.Debug("joystick %d disconnected...", joy)
    }
}

Is this expected? If not is there a recommended way to refactor glfw.Connected such that it can be used as a MonitorEvent as well as a JoystickEvent? Does anyone even use dynamic joystick plug-and-play (on linux /w xboxdrv all wireless joysticks in /dev/input/ exist up front so I don't haven't tested this really)?

@dmitshur
Copy link
Member

dmitshur commented Sep 18, 2016

First, what package are you using? v3.2/glfw?

Second, let me clarify. Are you saying both versions of those code compile, and the only difference is:

 func (self *Game) joysticks(joy, event int) {
-     if event == glfw.Connected {
+     if event == int(glfw.Connected) {
          // setup joystick
          self.Debug("joystick %d connected...", joy)
-     } else if event == glfw.Disconnected {
+     } else if event == int(glfw.Disconnected) {
          // teardown joystick
          self.Debug("joystick %d disconnected...", joy)
      }
  }

Yet one works but the other doesn't?

That's definitely very unexpected, and I guess it's a part of the cgo unpleasantness that I forgot about.

I don't have easy access to a monitor or joystick to connect/disconnect to test this.

Looking at the GLFW C API, I can confirm that both monitor and joystick connection callbacks indeed reuse the same defines:

In the Go code, we currently have:

// MonitorEvent corresponds to a monitor configuration event.
type MonitorEvent int

// Monitor events.
const (
    Connected    MonitorEvent = C.GLFW_CONNECTED
    Disconnected MonitorEvent = C.GLFW_DISCONNECTED
)

But there's nothing for joystick events. I think we can either add:

// JoystickEvent corresponds to a joystick configuration event.
type JoystickEvent int

// Joystick events.
const (
    JoystickConnected    JoystickEvent = C.GLFW_CONNECTED
    JoystickDisconnected JoystickEvent = C.GLFW_DISCONNECTED
)

Unlike in C, we can't reuse Connected and Disconnected identifiers for both types, because that would result in duplicate symbols in the package. (/cc @elmindreda FYI)

We could also add MonitorConnected, MonitorDisconnected and deprecate the current Connected/Disconnected.

Another alternative solution is to reuse the same type for both. That would keep us closer to the C API, at least in spirit.

For example:

// MonitorJoystickEvent corresponds to a monitor/joystick configuration event.
type MonitorJoystickEvent int

// Monitor/joystick events.
const (
    Connected    MonitorJoystickEvent = C.GLFW_CONNECTED
    Disconnected MonitorJoystickEvent = C.GLFW_DISCONNECTED
)

Or give it a better name:

// PeripheralEvent corresponds to a peripheral device configuration event.
type PeripheralEvent int

// Monitor/joystick events.
const (
    Connected    PeripheralEvent = C.GLFW_CONNECTED
    Disconnected PeripheralEvent = C.GLFW_DISCONNECTED
)

Other possible names include DeviceEvent.

@cdelorme
Copy link
Author

Ah, sorry for the missing information and confusion:

  1. I am using 3.2
  2. only the one with explicit casting works, the other fails with type-mismatch at build time
  3. the controller connect/disconnect behavior is almost certainly my own problem, I was more curious if anyone finds benefit in bothering to use those callbacks, since you just end up polling controller states anyway and it does not appear to crash when I poll disconnected controllers. Coming from SDL where you had to "open" and initialize controllers is what makes this feel unnecessary in glfw.

Given that the glfw documentation refers to a single GLFW_CONNECTED constant, I think it would make the most sense to consolidate the types and reuse the constant at both locations, but I wasn't sure if it was intentionally not implemented this way. If we did add new joystick events the callback would also need to be updated to work with the new type otherwise I'd be back to casting.

@tapir
Copy link
Member

tapir commented Oct 1, 2016

I like the idea of renaming it to PeripheralEvent. I think that's what we should do next version since it will break API.

But.... that will not solve @cdelorme's problem. Go does not allow implicit type casts.
Second example is how it should be.

If we want to eliminate casting there we need to do this (which is inconsistent with the rest of the API so I don't like it but as always I'm open to discussion)

const (
    Connected  = C.GLFW_CONNECTED
    Disconnected = C.GLFW_DISCONNECTED
)

@tapir tapir modified the milestone: glfw-next Oct 1, 2016
@cdelorme
Copy link
Author

cdelorme commented Oct 1, 2016

If we want the bindings to provide consistent resource prefixes to each event then some redundancy is in order:

// PeripheralEvent corresponds to a peripheral device configuration event.
type PeripheralEvent int

// Monitor/joystick events.
const (
    Connected    PeripheralEvent = C.GLFW_CONNECTED
    Disconnected PeripheralEvent = C.GLFW_DISCONNECTED
)

// type MonitorEvent int
type MonitorEvent PeripheralEvent

// Monitor specific constants for consistent API
const (
    MonitorConnected MonitorEvent = Connected
    MonitorDisconnected MonitorEvent = Disconnected
)

// type JoystickEvent int
type JoystickEvent PeripheralEvent

// Joystick specific constants for consistent API
const (
    JoystickConnected JoystickEvent = Connected
    JoystickDisconnected JoystickEvent = Disconnected
)

Not sure if there is any value at that point in having PeripheralEvent, but keeping things separate without adding unused constants means more C. references:

type MonitorEvent PeripheralEvent

// Monitor specific constants for consistent API
const (
    MonitorConnected MonitorEvent = C.GLFW_CONNECTED
    MonitorDisconnected MonitorEvent = C.GLFW_DISCONNECTED
)

type JoystickEvent PeripheralEvent

// Joystick specific constants for consistent API
const (
    JoystickConnected JoystickEvent = C.GLFW_CONNECTED
    JoystickDisconnected JoystickEvent = C.GLFW_DISCONNECTED
)

tapir added a commit that referenced this issue Aug 6, 2018
@tapir tapir added the glfw-3.3 label Apr 8, 2019
@tapir tapir removed this from the glfw-next milestone Apr 8, 2019
pwaller pushed a commit that referenced this issue 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

Hopefully this is addressed in #256, which introduces glfw 3.3 support. If not please report back and we can reopen.

@pwaller pwaller closed this as completed Nov 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants