samdze / playdate-nim

Nim bindings with extra features for the Playdate SDK
66 stars 3 forks source link

Fix mapping of PDButton #7

Closed Nycto closed 1 year ago

Nycto commented 1 year ago

PDButtons is not currently behaving like a real Nim set. This change fixes that behavior, and also adds unit tests

Nim appears to already map enum values to a bitfield by doing a shl internally. Doing the shl in the binding code was forcing a "double encoding" problem. This was causing set operations against PDButtons to fail silently.

This change also removes the 'check' function, since we can now use regular Nim set operations. For example, we could just do:

kButtonA in buttons

Note that the unit tests were added as a separate commit. If you would like them as a separate PR, let me know

samdze commented 1 year ago

Alright, this was in fact a mysterious behaviour when I tested with API.

So, testing your branch I'm getting some weird results. When I build the project these warnings show up:

/Users/sam/.nimble/pkgs/playdate-0.7.0/playdate/bindings/system.nim:8:7: warning: case value not in enumerated type 'PDButtons' [-Wswitch]
        case ((PDButtons) 3):
             ^
/Users/sam/.nimble/pkgs/playdate-0.7.0/playdate/bindings/system.nim:8:7: warning: case value not in enumerated type 'PDButtons' [-Wswitch]
        case ((PDButtons) 5):
             ^
/Users/sam/.nimble/pkgs/playdate-0.7.0/playdate/bindings/system.nim:8:7: warning: case value not in enumerated type 'PDButtons' [-Wswitch]
        case ((PDButtons) 6):
             ^
/Users/sam/.nimble/pkgs/playdate-0.7.0/playdate/bindings/system.nim:6:10: warning: enumeration values 'kButtonDown', 'kButtonB', and 'kButtonA' not handled in switch [-Wswitch]
        switch (e) {
                ^
/Users/sam/.nimble/pkgs/playdate-0.7.0/playdate/bindings/system.nim:6:10: note: add missing switch cases
        switch (e) {
                ^
4 warnings generated.

And buttonsState.current is always 0, no matter what button I press. However, I noticed buttonsState.released reported the correct buttons being released. So I discovered that only the last non-nil PDButtons get the correct reading from the SDK here in getButtonsState:

proc getButtonsState* (this: ptr PlaydateSys): tuple[current: PDButtons, pushed: PDButtons, released: PDButtons] =
    privateAccess(PlaydateSys)
    var current, pushed, released: PDButtons
    this.getButtonState(cast[ptr PDButton](addr(current)), cast[ptr PDButton](addr(pushed)), cast[ptr PDButton](addr(released)))
    return (current: current, pushed: pushed, released: released)

So, using the code above, only released is being populated correctly, the other PDButtons remain 0 (or at least, no known button is reported as 1). And using this:

proc getButtonsState* (this: ptr PlaydateSys): tuple[current: PDButtons, pushed: PDButtons, released: PDButtons] =
    privateAccess(PlaydateSys)
    var current, pushed, released: PDButtons
    this.getButtonState(cast[ptr PDButton](addr(current)), nil, nil)#cast[ptr PDButton](addr(pushed)), cast[ptr PDButton](addr(released)))
    return (current: current, pushed: pushed, released: released)

current is correct and kButtonA in buttons works as expected.

Tested on macOS, SDK v1.13.1, the previous method with check works but it doesn't behave like a real Nim set indeed.

Nycto commented 1 year ago

Interesting -- could be platform distances? I'm busting on Ubuntu.

A fairly simple resolution would be to stop using a set completely and switch it to a distinct uint32. I'll put together a change to show you what I mean.

samdze commented 1 year ago

This is really weird. I like the syntax kButtonA in buttons though.

Yes, maybe testing with a plain uint32 could reveal what is going on, may be a Nim bug with enum sets?

EDIT: testing myself, providing a pointer to a uint32 returns the correct data.

samdze commented 1 year ago

This works:

proc getButtonsState* (this: ptr PlaydateSys): tuple[current: PDButtons, pushed: PDButtons, released: PDButtons] =
    privateAccess(PlaydateSys)
    var current, pushed, released: uint32
    this.getButtonState(addr(current), addr(pushed), addr(released))
    return (current: cast[PDButtons](current), pushed: cast[PDButtons](pushed), released: cast[PDButtons](released))

Also changed the parameters types in here:

#bindings/system.nim
...
        getButtonState {.importc: "getButtonState".}: proc (current: ptr uint32;
            pushed: ptr uint32; released: ptr uint32) {.cdecl, raises: [].}
...

However, my compiler still complains on case values not in enumerated type. We can fix this by not importing the C enum:

type PDButton* {.size: sizeof(uint32).} = enum
    kButtonLeft, kButtonRight, kButtonUp,
    kButtonDown, kButtonB, kButtonA
Nycto commented 1 year ago

Incorporated your suggestion. Great idea!

samdze commented 1 year ago

Thank you!