veandco / go-sdl2

SDL2 binding for Go
https://godoc.org/github.com/veandco/go-sdl2
BSD 3-Clause "New" or "Revised" License
2.2k stars 219 forks source link

inconsistent types between JoyDeviceEvent.Which and JoystickOpen() #333

Closed mbenkmann closed 6 years ago

mbenkmann commented 6 years ago

For some reason the type of JoystickOpen() was changed from JoystickID to int. This creates an inconsistency with a very common source of this argument JoyDeviceEvent.Which.

In the future, please do not make changes that breaks client code and makes it WORSE (by requiring casts).

mbenkmann commented 6 years ago

After checking the newest SDL sources it seems that the actual problem is the type of ev.Which. That one should not be JoystickID. In any case ev.Which should be able to be plugged into JoystickOpen without cast.

mbenkmann commented 6 years ago

sigh After an even closer look it seems like the meaning of ev.Which differs for add and remove events, which conflicts with go-sdl2's use of a special JoystickID type. I think the only proper way to fix this is to split JoyDeviceEvent into 2 structs where the AddedEvent has ev.Which as int and the RemovedEvent has ev.Which as JoystickID.

malashin commented 6 years ago

For some reason the type of JoystickOpen() was changed from JoystickID to int.

This is the way it is supposed to work. There are two different functions now as they are in SDL2.

// JoystickOpen opens a joystick for use.
// (https://wiki.libsdl.org/SDL_JoystickOpen)
func JoystickOpen(index int) *Joystick {
    return (*Joystick)(C.SDL_JoystickOpen(C.int(index)))
}

// JoystickFromInstanceID returns the Joystick associated with an instance id.
// (https://wiki.libsdl.org/SDL_GameControllerFromInstanceID)
func JoystickFromInstanceID(joyid JoystickID) *Joystick {
    return (*Joystick)(C.SDL_JoystickFromInstanceID(joyid.c()))
}

I think the only proper way to fix this is to split JoyDeviceEvent into 2 structs where the AddedEvent has ev.Which as int and the RemovedEvent has ev.Which as JoystickID.

Ye, I think that's the way we should do it.

malashin commented 6 years ago

I've committed the changes into the master branch. There are two types of JoyDevice events now:

// JoyDeviceAddedEvent contains joystick device event information.
// (https://wiki.libsdl.org/SDL_JoyDeviceEvent)
type JoyDeviceAddedEvent struct {
    Type      uint32 // JOYDEVICEADDED
    Timestamp uint32 // the timestamp of the event
    Which     int    // the joystick device index
}

// JoyDeviceRemovedEvent contains joystick device event information.
// (https://wiki.libsdl.org/SDL_JoyDeviceEvent)
type JoyDeviceRemovedEvent struct {
    Type      uint32     // JOYDEVICEREMOVED
    Timestamp uint32     // the timestamp of the event
    Which     JoystickID // the instance id
}

Previous behavior:

Type: JOYDEVICEADDED,   Timestamp: 11051, Which: 0(sdl.JoystickID)
Type: JOYDEVICEREMOVED, Timestamp: 12312, Which: 0(sdl.JoystickID)
Type: JOYDEVICEADDED,   Timestamp: 18056, Which: 0(sdl.JoystickID)
Type: JOYDEVICEREMOVED, Timestamp: 19113, Which: 1(sdl.JoystickID)
Type: JOYDEVICEADDED,   Timestamp: 23048, Which: 0(sdl.JoystickID)
Type: JOYDEVICEREMOVED, Timestamp: 24342, Which: 2(sdl.JoystickID)

Current behavior:

Type: JOYDEVICEADDED,   Timestamp: 7671,  Which: 0(int)
Type: JOYDEVICEREMOVED, Timestamp: 9962,  Which: 0(sdl.JoystickID)
Type: JOYDEVICEADDED,   Timestamp: 16704, Which: 0(int)
Type: JOYDEVICEREMOVED, Timestamp: 17811, Which: 1(sdl.JoystickID)
Type: JOYDEVICEADDED,   Timestamp: 21710, Which: 0(int)
Type: JOYDEVICEREMOVED, Timestamp: 23174, Which: 2(sdl.JoystickID)