Implement keyboard API #53

koxu1996 · 2021-07-06T08:57:02Z

Base API

Functions:

p5.KeyIsDown(code string) bool - checks if given key is already pressed

Variables:

p5.KeyIsPressed bool - whether any key is pressed now
p5.Key string - name of last pressed/typed key

Callbacks

Callbacks can be set with following functions:

p5.SetKeyPressedCallback(f KeyEventFunc)
p5.SetKeyTypedCallback(f KeyEventFunc)
p5.SetKeyReleasedCallback(f KeyEventFunc)

Caveats:

There is no way to detect lower case characters, so we always use upper case. Bear that in mind, especially when working with KeyTyped event.

@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 #52. Feel free to suggest changes, this is just proposal 😀

codecov · 2021-07-07T12:56:39Z

Codecov Report

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

@@            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%)`	⬇️
keyboard.go	`0.00% <0.00%> (ø)`
p5.go	`0.00% <0.00%> (ø)`
proc.go	`63.28% <13.88%> (-8.09%)`	⬇️

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 · 2021-07-07T13:22:55Z

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 · 2021-07-08T07:40:53Z

I made little refactoring, now callbacks and global variables are moved to single struct.
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.
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{}

map with bool is recommended for set-like data - see https://blog.golang.org/maps#TOC_4
we cannot use key.Event as key, because we could not search this map in simple way

sbinet · 2021-07-09T15:35:53Z

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 · 2021-07-12T21:01:08Z

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 · 2021-07-25T18:40:57Z

@sbinet Do you have time to look at it?

sbinet

hi,

apologies for the belated answer.
please find my comments inlined.

do you have an idea on a testing strategy for this new feature?
(something testable through the usual go test scaffolding)

also: could you rebase off the latest main?

thanks.

sbinet · 2021-08-02T13:51:04Z

keyboard.go

@@ -0,0 +1,33 @@
+// Copyright ©2020 The go-p5 Authors. All rights reserved.


Suggested change

// Copyright ©2020 The go-p5 Authors. All rights reserved.

// Copyright ©2021 The go-p5 Authors. All rights reserved.

sbinet · 2021-08-02T13:53:47Z

proc.go

+			if e.State == key.Press {
+				p.onKeyPressed(e)
+			} else if e.State == key.Release {


Suggested change

if e.State == key.Press {

p.onKeyPressed(e)

} else if e.State == key.Release {

switch e.State {

case key.Press:

p.onKeyPressed(e)

case key.Release:

sbinet · 2021-08-02T13:59:12Z

keyboard.go

+
+import "gioui.org/io/key"
+
+var Keyboard struct {


perhaps move this as an anonymous field of the global Event variable, much like we did for Mouse ?

p5/event.go

Line 9 in 6b7ccd9

Mouse struct {

ie:

var Event struct { Mouse struct { ... } Key struct { ... } }

sbinet · 2021-08-02T14:02:13Z

proc.go

+	KeyPressed  KeyEventFunc
+	KeyTyped    KeyEventFunc
+	KeyReleased KeyEventFunc


instead of "exploding" the KeyCb fields into 3 different fields in Proc, perhaps just add a Key KeyCb field to Proc ?

sbinet · 2021-08-02T14:06:16Z

keyboard.go

+// KeyEventFunc is the type of key functions users provide to p5.
+type KeyEventFunc func(key.Event)
+
+type KeyCb struct {


this new type probably needs a bit of documentation.
also, as we spelled out the func-option WithKeyCallback, what about renaming this type KeyCallback ?
(alternatively, we could go with WithKeyCbk / KeyCbk)

the meaning of (and their semantical nuance) the Pressed and Typed probably need to be documented and/or explained.
this can be done as part of the documentation of each of these fields.

sbinet · 2021-08-02T14:09:30Z

api.go

 	gproc.DrawImage(img, x, y)
 }
+
+// KeyIsDown checks if given key is already pressed.


Suggested change

// KeyIsDown checks if given key is already pressed.

// KeyIsDown checks whether a given key is already pressed.

sbinet · 2021-08-02T14:09:55Z

api.go

 }
+
+// KeyIsDown checks if given key is already pressed.
+func KeyIsDown(code string) bool {


Suggested change

func KeyIsDown(code string) bool {

func KeyIsDown(name string) bool {

sbinet · 2021-08-02T14:11:06Z

keyboard.go

+// KeyIsDown checks if given key is already pressed.
+func (p *Proc) KeyIsDown(code string) bool {


Suggested change

// KeyIsDown checks if given key is already pressed.

func (p *Proc) KeyIsDown(code string) bool {

// KeyIsDown checks whether a given key is already pressed.

func (p *Proc) KeyIsDown(name string) bool {

sbinet · 2021-08-02T14:12:29Z

keyboard.go

+	if _, ok := Keyboard.downKeys[code]; ok {
+		return true
+	}
+	return false


Suggested change

if _, ok := Keyboard.downKeys[code]; ok {

return true

}

return false

_, ok := Keyboard.downKeys[code]

return ok

sbinet · 2021-08-16T09:50:01Z

ping?

koxu1996 · 2021-08-16T15:17:45Z

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

koxu1996 added 3 commits July 6, 2021 11:03

p5: implement keyboard API

9afb5a4

example/key-pressed: first import

c4f2ecd

example/key-snake: first import

518e232

p5: refactor keyboard API

75dcc08

koxu1996 added 2 commits July 12, 2021 22:56

p5: replace map[T]bool with map[T]struct{}

8e3c6a0

p5: store key callbacks in proc and pass via single option

11a665b

sbinet reviewed Aug 2, 2021

View reviewed changes

koxu1996 closed this Aug 16, 2021

sbinet mentioned this pull request Nov 12, 2021

KeyPress event absent #55

Closed

koxu1996 mentioned this pull request Nov 10, 2023

p5: implement more of p5js API #6

Open

15 tasks

		@@ -0,0 +1,33 @@
		// Copyright ©2020 The go-p5 Authors. All rights reserved.

	// Copyright ©2020 The go-p5 Authors. All rights reserved.
	// Copyright ©2021 The go-p5 Authors. All rights reserved.

	// KeyIsDown checks if given key is already pressed.
	// KeyIsDown checks whether a given key is already pressed.

	func KeyIsDown(code string) bool {
	func KeyIsDown(name string) bool {

		// KeyIsDown checks if given key is already pressed.
		func (p *Proc) KeyIsDown(code string) bool {

Implement keyboard API #53

Implement keyboard API #53

Uh oh!

Conversation

koxu1996 commented Jul 6, 2021

Base API

Callbacks

Uh oh!

codecov bot commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbinet commented Jul 7, 2021

Uh oh!

koxu1996 commented Jul 8, 2021

Uh oh!

sbinet commented Jul 9, 2021

Uh oh!

koxu1996 commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koxu1996 commented Jul 25, 2021

Uh oh!

sbinet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbinet commented Aug 16, 2021

Uh oh!

koxu1996 commented Aug 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jul 7, 2021 •

edited

Loading

koxu1996 commented Jul 12, 2021 •

edited

Loading