go-p5 / p5

p5 is a simple package that provides primitives resembling the ones exposed by p5js.org
https://go-p5.github.io/
BSD 3-Clause "New" or "Revised" License
146 stars 12 forks source link

Implement keyboard API #53

Closed koxu1996 closed 3 years ago

koxu1996 commented 3 years ago

Base API

Functions:

Variables:

Callbacks

Callbacks can be set with following functions:


Caveats:


@sbinet I am not sure if the code is safe, so please take a look. I used your examples key-snake and key-pressed from https://github.com/go-p5/p5/pull/52. Feel free to suggest changes, this is just proposal :grinning:

codecov[bot] commented 3 years ago

Codecov Report

Merging #53 (11a665b) into main (cbaf5d8) will decrease coverage by 4.78%. The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
- Coverage   80.62%   75.84%   -4.79%     
==========================================
  Files           9       10       +1     
  Lines         609      654      +45     
==========================================
+ Hits          491      496       +5     
- Misses         96      133      +37     
- Partials       22       25       +3     
Impacted Files Coverage Δ
api.go 60.71% <0.00%> (-2.25%) :arrow_down:
keyboard.go 0.00% <0.00%> (ø)
p5.go 0.00% <0.00%> (ø)
proc.go 63.28% <13.88%> (-8.09%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cbaf5d8...11a665b. Read the comment docs.

sbinet commented 3 years ago

thanks for the PR.

I like you map-based approach. (I was initially thinking of using a uint32 mask, but, well, if the map approach is efficient enough, who cares)

I think I'd rather have only one struct that holds all the callbacks than increasing the number of globals. e.g.:

package p5

type Keyboard struct {
    down    map[string]struct{}
    release map[key.Event]struct{}
    last    string

    Pressed  KeyCallback
    Typed    KeyCallback
    Released KeyCallback
}

func WithKeyCallback(cbk Keyboard) Option {
    return func(p *Proc) { p.Key = cbk }
}

and:

package main

func main() {
    p5.Run(setup, draw, p5.WithKeyCallback(p5.Keyboard{
        Pressed: keyPressed,
        ...
    })
}

what do you think?

koxu1996 commented 3 years ago
  1. I made little refactoring, now callbacks and global variables are moved to single struct.

  2. Regarding your callback approach, user have to pass all 3 callbacks otherwise SIGSEGV is thrown. I would rather allow setting each individually. Additionally your example is using Proc.Key which is missing, and I am confused if you want to store callbacks in Keyboard or Proc structure.

  3. I don't see how uint32 mask could be used; for storing keys map is suitable and efficient container. Regarding following code:

down map[string]struct{} release map[key.Event]struct{}

sbinet commented 3 years ago

Regarding your callback approach, user have to pass all 3 callbacks otherwise SIGSEGV is thrown.

not necessarily. this would need some plumbing in p5.Run(...), though:

func Run(setup, draw Func, opts ...Option) {
...
    for _, opt := range opts {
        opt(gproc)
    }
    if gproc.Key.Pressed == nil {
        gproc.Key.Pressed = func() {}
    }
    ...
}

Additionally your example is using Proc.Key which is missing

I was indeed using notation from my previous PR :) I was also probably beeing overeager and starting to reduce the number of globals being used in the p5 package.

map with bool is recommended for set-like data - see https://blog.golang.org/maps#TOC_4

this blog entry is a bit old :) I think nowadays the map[T]struct{} is prefered as it brings the same semantics than a map[T]bool without the extra memory foot print of the useless boolean associated value (the empty struct struct{} will be optimized away).

we cannot use key.Event as key, because we could not search this map in simple way

you're right.

(I am sorry I'll have to disappear for a week w/o much internet connectivity and leave this PR unreviewed for the time being. I'll review it further coming back from holidays)

koxu1996 commented 3 years ago

Okay, I replaced map[string]bool with map[string]struct{}. I moved callbacks into Proc as you wished and now they are passed via single Option:

func main() {
    p5.Run(setup, draw, p5.WithKeyCallback(p5.KeyCb{
        Pressed:  keyPressed,
        Typed:    keyTyped,
    }))
    ...

Is it fine?

koxu1996 commented 3 years ago

@sbinet Do you have time to look at it?

sbinet commented 3 years ago

ping?

koxu1996 commented 3 years ago

Sorry, I am quite busy nowadays and I am no longer interested in using go-p5.