UPBGE / upbge

UPBGE, the best integrated game engine in Blender
https://upbge.org
Other
1.44k stars 181 forks source link

Ugly keyboard and mouse API #515

Open HG1-Public opened 7 years ago

HG1-Public commented 7 years ago

This is not bug. I just want to complain about the new API.

I am really unhappy with the new keyboard and mouse API. It worth ten the old, more complicated and "non Pythonic".

  1. Why the python keyboard and mouse mow return a dict instead of a list? It complicates everything. If you want to loop over the keys you have the convert it to a list. Most users don't know this.
  2. Why SCA_KeyboardSensor still returns a list when the other API returns a dict. Actually I don't see an reason to use a dict.
  3. Why there multiple ways to access the same same values? Like the dictionary keycode and the SCA_InputEvent.type returns the same thing. That only confuse the use.
  4. Splitting the status into status and queue is also no good thing. Because now it is not loop able. It also force the use to ugly "if in" code to don't get an error. And for the status you have to use [-1] to get the right value back. Most users don't know that and will try to use only status or status[0]. My suggestion instead would be to use three simple booleans instead like pressed, released and hold and keep status as it was.
panzergame commented 7 years ago
  1. What about for input in keyboard.inputs.values() ?
  2. Compatibility stuffs........ The usage of dict allow to easily found the corresponding input to a key value, it's harder with a list of pair tuple.
  3. It can be useful to know the keycode when iterating over the only the values of the input dict, or for the user when it pass the input to a function etc... I don't see a problem of duplicating this info.
  4. The new API was designed for a main issue: the miss of some input because we logged only the status at the end of the frame and not the different status/event during the frame, this is fixed by SCA_InputEvent.status/queue and allow to do proper recording of typed text or precisely count the how many a key is pressed even if it is two or more times by frame, but I admit that it can be hard to use for a novice.

So I join you and propose some attributes in SCA_InputEvent with a limited behaviour: up Return true when at least one active status is present down Return true when at least one none status is present activated Return true when at least one activating event is present deactivated Return true when at least one deactivating event is present

So here upand downcan both return true in same frame and same for activated and deactivated.

What do you think of it ?

HG1-Public commented 7 years ago
  1. Did you mean you want to make an new function .values()? Actually I use for key, status in keyInfo.items(): to solve this problem, but it is a conversion. I don't know how fast it is. Since that my code is a special case (KeyCode conversion) and the most users using the API in in an other way I am also satisfied with the current solution of using dict.
  2. Compatibility stuffs???? Inputs and activeInputs in SCA_PythonKeyboard and inputs in SCA_KeyboardSensor are new. I though more like a simple list with 256 elements which returns only the list of SCA_InputEvent's. It only takes a little bit more memory space, because of the unused numbers and gaps. (I think F19 = 180 BGE, page down 112 UPBGE are the highest values. But as described above at point 1, I am OK with the dict.
  3. One golden rule of Python is, that there is only one way to achieve something. So it is easier to find the right away. When you are iterating over over the dict you already get the key code back. Example: for key, input in sensor.activeInputs.items():
  4. I realized that this was the reason for it, if I was looking at the mouse x/y values. But for the keyboard I never had a problem. I had to reduce the frame rate under 28 fps to have a chance to get a more then one key in one frame. At 15 frames it sometimes happens with normal fast pressing the key. But for a game 15 FPS is unplayable.

I also don't like that status and queue behave different. Status returns a empty list while queue returns no list. Is there a reason for it.

Conclusion: The main problem is witch input type you are using. Input is easier if it returns a dict. But for activeInputs (or inputs from the keyboard sensor) it is easier to have a list. Because simply using
if JUST_ACTIVATED in keyboard.activeInputs [bge.events.WKEY].queue: will not work. So you have to check if the key is in the list (if bge.events.WKEY in keyboard.activeInputs:) too or loop over the active keys, which basically is the easier way in this case.

About up, down, activated, deactivated I need to think about it. Because if activeInputs is used it could be problem. The best would be if the key would be pressed multiple times a activated is high in this frame and always flowed by a deactivated in the next frame if the key is not pressed.

panzergame commented 7 years ago
  1. values() is a python dict function to return a list of the values.

I also don't like that status and queue behave different. Status returns a empty list while queue returns no list. Is there a reason for it.

statusis for state so it's impossible to have no state and so an empty list, queue is for the events, it can be empty if nothing changed for a given input during a frame. I don't understand what you mean by no list, it's None ? If yes, it's a bug because in source we return CListWrapper proxy in the getter without exceptions or so. For activeInput, why use a list if you can iterate over it currently ?

a activated is high in this frame and always flowed by a deactivated in the next frame I wanted to avoid this behaviour, it's quite imprecise : the user pressed and released the key in the same frame, but talking on states, the key will be active for two frames…

HG1-Public commented 7 years ago
  1. Is this new? Because keyboard.inputs.values() does not exist in UBGE 0.1.8? There is only values attribute in the SCA_InputEvent. Example: if bge.logic.KX_INPUT_JUST_ACTIVATED in keyboard.inputs[bge.events.DKEY].values.

With no list I meant the empty list. I thought it would be better to return 0 or -1 if no event is occurred. So you can use if keyboard.inputs[bge.events.AKEY].queue[0] == bge.logic.KX_INPUT_JUST_ACTIVATED: if keyboard.inputs[bge.events.AKEY].queue[-1] == bge.logic.KX_INPUT_JUST_ACTIVATED: instead of if bge.logic.KX_INPUT_JUST_ACTIVATED in keyboard.inputs[bge.events.AKEY].queue:

For activeInput, why use a list if you can iterate over it currently ?

No you can't iterate over activeInput because it returns a dict. You cant iterate over a dict without converting it with .items() to a list of tuples. So for inputs in SCA_KeyboardSensor (dict for pressed keys):

and SCA_PythonKeyboard (list in list for pressed keys)

As described above I don't have a problem with the dict or list. But both of the new API's should return the same.

I wanted to avoid this behavior, it's quite imprecise : the user pressed and released the key in the same frame, but talking on states, the key will be active for two frames…

Yes I know It is tricky. But you can only trigger one action per frame. If you want that minimum one key press will be trigger an action for one frame you have to do so. Otherwise the action will never activated or will never deactivated. With other words the player will never move or never stop moving if you press the button fast.

SCA_InputEvent.status/queue and allow to do proper recording of typed text By the way for typing text there is .text attribute which solve the problem with slow framerate.

I will have a look at some other game engines how the solve this problem.

panzergame commented 7 years ago

I though you were talking about only SCA_PythonKeyboard, now i understand and i'm going to change sensor.inputs to return a dict.

  1. https://docs.python.org/3.7/library/stdtypes.html#dict.values, no ?

Yes I know It is tricky. But you can only trigger one action per frame. If you want that minimum one key press will be trigger an action for one frame you have to do so. Otherwise the action will never activated or will never deactivated. With other words the player will never move or never stop moving if you press the button fast.

Keyboard sensor is still doing this behaviour.

HG1-Public commented 7 years ago

1.My bad. I only have done a quick test by copying your code for input in keyboard.inputs.values() and I got an error because of the missing colon.

Keyboard sensor is still doing this behaviour.

It work's only if the frame rate is high enough. I also made some tests with low frame rates and there you will get the problems with triggering the motions. With the actual status and queue you can get [0, 2, 0] [1, 3] as last value.

if you use a code like this the motion will never trigger.

if bge.logic.KX_INPUT_ACTIVE == keyboard.inputs[bge.events.WKEY].status[-1]: #-1 needed for activeInputs
     print("Forward!")

if you use a code like this the motion will never trigger.

if bge.logic.KX_INPUT_JUST_ACTIVATED in keyboard.inputs[bge.events.WKEY].queue:
     print("Activate Forward!")
if bge.logic.KX_INPUT_JUST_RELEASED in keyboard.inputs[bge.events.WKEY].queue:
     print("Deactivate Forward!")

if you use a code like this the motion will never stop.

if bge.logic.KX_INPUT_JUST_ACTIVATED in keyboard.inputs[bge.events.WKEY].queue:
     print("Activate Forward!")
elif bge.logic.KX_INPUT_JUST_RELEASED in keyboard.inputs[bge.events.WKEY].queue:
     print("Deactivate Forward!")

You will also have problems with your approach for up and down.

So I join you and propose some attributes in SCA_InputEvent with a limited behaviour: up Return true when at least one active status is present down Return true when at least one none status is present activated Return true when at least one activating event is present deactivated Return true when at least one deactivating event is present

Because if you use activeInputs and down Return true when at least one active status is present,

the motion will never stop even with high frame rates (last value from activeInputs [2, 0])

if keyboard.activeInputs[bge.events.WKEY].down:
     print("Activate Forward!")
else:
     print("Deactivate Forward!")

So i think it is better for down to return the last value in the status list. with results in the behavior that maybe the motion will not be triggered. But this will not solve the unpredictable behavior for solve frame rates.

That was the reason why I thought that it is maybe better to attach one extra event. This will allow use your approach for down and always trigger a motion for reach key press and stop the motion because the next frame will return [0] status and for [3] for queue.

panzergame commented 7 years ago

If you want to move when the key is pressed and that you consider that a press and release in one frame does nothing (similar to old behaviour):

q = keyboard.inputs[WKEY].queue
if len(q) == 1:
   if q[0] == KX_INPUT_JUST_ACTIVATED:
      # move
   else:
      # stop
else:
    # too quick to do something, or no events

Because if you use activeInputs and down Return true when at least one active status is present, the motion will never stop even with high frame rates (last value from activeInputs [2, 0])

If you know exactly which input is used for an action, iterate over the input in keyboard.inputs directly and avoid the case where an input is down (the key was at down state which can be perfectly true) and in the active input list.

That was the reason why I thought that it is maybe better to attach one extra event. This will allow use your approach for down and always trigger a motion for reach key press and stop the motion because the next frame will return [0] status and for [3] for queue.

The new system is simpler that the previous one for one thing, it log exactly the events it received from blender without any extra behaviour. Based on this we add behaviour for keyboard sensor (on the logic brick side not python side).

BluePrintRandom commented 7 years ago

HG1 want to take a look at my keybinder?

I use upbge** https://www.youtube.com/watch?v=1M78Dej4r8A

I iterate active input and build a list, you could build 3 (just activated list, activated, just released and inactive)

keyboard.active keyboard.inactive keyboard.justActive keyboard.justInactive

may be cool to be done in C really fast?

but I don't have any problems with new api.

On Fri, Jun 23, 2017 at 12:37 PM, panzergame notifications@github.com wrote:

If you want to move when the key is pressed and that you consider that a press and release in one frame does nothing (similar to old behaviour):

q = keyboard.inputs[WKEY].queue if len(q) == 1: if q[0] == KX_INPUT_JUST_ACTIVATED:

move

else:

stop

else:

too quick to do something, or no events

Because if you use activeInputs and down Return true when at least one active status is present, the motion will never stop even with high frame rates (last value from activeInputs [2, 0])

If you know exactly which input is used for an action, iterate over the input in keyboard.inputs directly and avoid the case where an input is down (the key was at down state which can be perfectly true) and in the active input list.

That was the reason why I thought that it is maybe better to attach one extra event. This will allow use your approach for down and always trigger a motion for reach key press and stop the motion because the next frame will return [0] status and for [3] for queue.

The new system is simpler that the previous one for one thing, it log exactly the events it received from blender without any extra behaviour. Based on this we add behaviour for keyboard sensor (on the logic brick side not python side).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/UPBGE/blender/issues/515#issuecomment-310755044, or mute the thread https://github.com/notifications/unsubscribe-auth/AG25WbqMeXW3-YnauA3O8y66TOWzaCmNks5sHBPtgaJpZM4N-sI2 .

HG1-Public commented 7 years ago

First I want to clarify that I personally can deal with the actual ugly API. I only tried to hep you to make the keyboard working correct as it should (no unpredictable behavior). The old and actual keyboard input is working fine as longe the frame rate is high enough. The new status and queue lists does not improve the old behavior much. It only fixes the counting problem for fast key presses at low frame rates.

With ugly I mean that now you need to write non obviously code.

#[-1] needed for activeInputs [0] will not stop the movement
if bge.logic.KX_INPUT_ACTIVE == keyboard.inputs[bge.events.WKEY].status[-1]: 
     print("Forward!")

if bge.logic.KX_INPUT_JUST_ACTIVATED in keyboard.inputs[bge.events.WKEY].queue:
     print("Activate Forward!")

for key, input in sensor.activeInputs.items():
     print(key, input.status, input.queue)
  1. For status you have to grab an element [0] or [-1]. Which one should an unexperienced user use?
  2. You need to use [-1] and == for status and in for queue for the if condition.
  3. ActiveInputs need to be converted to an list. Its not obviously for the most users that you can't iterate over a dict. In case that you always have to loop over activeInputs it can return a list instead. Sure I know then .inputs returns a dict and activeInputs returns a list may look inconsistent but generating a dict and then converting it to a list with a Python call makes less sense to me.
  4. Most users only want to move there charter. So the don't need that unusual lists. The only want a single bit like down, up, activated, deactivated like you suggested.

Something like this will look much nicer to me then the above code.

if keyboard.inputs[bge.events.WKEY].down: # or hold, pressed, active what you like 
     print("Activate Forward!")
else:
     print("Deactivate Forward!")

or for activeInputs

for key, input in sensor.activeInputs: # or .input for SCA_KeyboardSensor
     if input.down:
          if key == bge.events.WKEY:
               print("Activate Forward!")
          else:
               print("Deactivate Forward!") 

If you want to move when the key is pressed and that you consider that a press and release in one frame does nothing (similar to old behaviour):

I don't want to have the same behavior as in the BGE. So your example code does not solve the actual problem. By the way. Your example will only move the player only for on frame. Also the code need now an additional if statement. That is not really an improvement.

q = keyboard.inputs[WKEY].queue
if len(q) == 1:
   if q[0] == KX_INPUT_JUST_ACTIVATED:
      # move
   else: # should be if q[0] == KX_INPUT_JUST_RELEASED:
      # stop
else:
    # too quick to do something, or no events
    # Yes and what the user should do here????

If you know exactly which input is used for an action, iterate over the input in keyboard.inputs directly and avoid the case where an input is down (the key was at down state which can be perfectly true) and in the active input list.

I think I don't understand your answer. Or you did not understand what I was trying to explain. This was a second topic and don't has anything to do with the above text. I was talking about your suggested approach for up, down, activated and deactivated. It will not work with activeInputs. Because the motion will never stop (last value from activeInputs [2, 0]).

down Return true when at least one none status is present

Sorry for the wrong code example in the previous post (it was late night)

for key, input in sensor.activeInputs.items():
     if input.down:
          if key == bge.events.WKEY:
               print("Activate Forward!")
          else:
               print("Deactivate Forward!") # will never happens

Basically what I think how the keyboard should work is: 1. Every key press should be should be trigger an action. If there are multiple key press in one frame it should the activation have higher priority. 2. It must be ensured the an active action can be stopped in the next frame.

But actually without the extra event one of the two points can't be fulfilled with activeInputs or inputs with a keyboard sensor. Sure you can solve this also in Python, but it is not trivial for the most users. You need to store the last pressed keys in a list and check in the following frame if the are not pressed an more to stop the motion.

panzergame commented 7 years ago

To continue the discussion I want to clarify the test case: 1) move while key down and stop while key up. 2) move while key down and stop at release event. 3) move at press event and stop at release event.

Because my example:

q = keyboard.inputs[WKEY].queue
if len(q) == 1:
   if q[0] == KX_INPUT_JUST_ACTIVATED:
      # move
   else:
      # stop
else:
    # too quick to do something, or no events
    # just do nothing here, you can remove the else statement

Works for the situation 3 as your example want to:

if bge.logic.KX_INPUT_JUST_ACTIVATED in keyboard.inputs[bge.events.WKEY].queue:
     print("Activate Forward!")
if bge.logic.KX_INPUT_JUST_RELEASED in keyboard.inputs[bge.events.WKEY].queue:
     print("Deactivate Forward!")

By the way. Your example will only move the player only for on frame.

Isn't what is needed ? Only one activation and only one deactivation ? If not say me what example is taken in account.

HG1-Public commented 7 years ago

No your example is worth. Because for slow frame rates it some times don't move and some times times it don't stop.

Here my console output. There are some unpredictable cases. no movement. Because queue has more then one element [0] [] [0, 2, 0] [1, 3] [0, 2, 0, 2, 0] [1, 3, 1, 3] [0] []

no movement only stop [0] [] [0, 2, 0, 2] [1, 3, 1] [2, 0] [3] stop [0] []

no deactivation [0] [] [0, 2] [1] move [2, 0, 2, 0] [3, 1, 3] [0] []

And when you use my example the activation is directly flowed by the deactivation in the same frame. So no movement occurs.

Didn't you tried the examples yourself with low frame rates and multiple key press?

panzergame commented 7 years ago
from bge import *

queue = logic.keyboard.inputs[events.WKEY].queue
status = logic.keyboard.inputs[events.WKEY].status

act = list(queue).count(logic.KX_INPUT_JUST_ACTIVATED)
nact = len(queue) - act

print(queue, act, nact)

# More press event than release event
if act > nact:
    print("== Forward == (>)")
# More release than press event
elif act < nact:
    print("== Stop == (>)")
# Same number of press than release (or no events), which event consider in priority ?
else:
    pass

This script solves the deactivation and activation issue but not the no movement:

[] 0 0
[1] 1 0
== Forward == (>)
[3, 1] 1 1
[3, 1] 1 1
[] 0 0
[] 0 0
[3, 1] 1 1
[3, 1] 1 1
[3] 0 1
== Stop == (>)
[1] 1 0
== Forward == (>)
[] 0 0
[3, 1] 1 1
[3] 0 1
== Stop == (>)
[1] 1 0
== Forward == (>)
[] 0 0
[3, 1, 3] 1 2
== Stop == (>)
[1, 3] 1 1
panzergame commented 7 years ago

What about a new variable in SCA_InputEvent which returns True when the previous down/up status is different that the current one:

input.downLevel
input.upLevel

So it could be a frame precision event based on status, what do you think ? Or I'm going forward in the hell way…

HG1-Public commented 7 years ago

I think instead of let the user evaluate the correct key status in Python, it is better to use the previous down/up status internal in C++ to evaluate and return directly the corrected key event as bit (down, activated, deactivated).

youle31 commented 7 years ago

I didn't looked at the code but isn't the old API still working with just a deprecation warning? In this case could we just remove deprecation warning to let the user choose if he wants to use new or old API?

organicpencil commented 7 years ago

Is... is there a particular reason we can't just do this? I'm probably sounding like a broken record.

bool keyboard.pressed(key)
bool keyboard.held(key)
bool keyboard.released(key)
list keyboard.pressed_keys
list keyboard.released_keys

pressed_keys would return all keypresses that occurred in 1 frame - in order - including repeats. I usually end up using the same abstraction script anyway. The keyboard API has always been confusing, more-so now.

pressed/held/released would check for stuff behind the scenes so that inputs aren't missed.

elmeunick9 commented 7 years ago

Why not just make a wrapper on top of the actual system? http://coredoc.royalwebhosting.net/api/event.html#key-bindings

Yes it's adding another way to do the same, but having an easy limited way and a complex and pawerful way to do the same is a good design. It's pretty much the same as with blf and the text object.