imput module keyboard/mouse ..is correct?

i want post this script in the resource forum since seem that work wery well and now is pretty tested,
anyway i’m not sure if:

A) is semantically correct (keys(), actives())
B) there are some mistake that i not see
C) has sense the name of the class “ImputBase”
D) is right instancing the class also if “self” do nothing
E) can be optimized

this the code:


import bge


bge_logic_keyboard = bge.logic.keyboard
bge_logic_mouse = bge.logic.mouse


ACTIVES_STATUS = {1, 2}


def _create_map(dictionary, postfix=""):
    numbers = {int(k) for k in dictionary}
    postfix = postfix.lower()
    strings = {}
    for att in dir(bge.events):
        numb = getattr(bge.events, att)
        if numb in numbers:
            att = att.lower()
            if att.endswith(postfix) and len(att) > len(postfix):
                att = att[:-len(postfix)]
            strings[att] = numb
            strings[att.upper()] = numb
    return strings




KEYBOARD_MAP = _create_map(bge.logic.keyboard.events, "KEY")
MOUSE_MAP = _create_map(bge.logic.mouse.events, "MOUSE")




class ImputBase:
    def keys(self, *strings):
        raise Exception


    def key(self, string):
        return self.keys(string)[0]


    def actives(self, *strings):
        return [v in ACTIVES_STATUS for v in self.keys(*strings)]


    def active(self, string):
        return self.actives(string)[0]




class Keyboard(ImputBase):
    def keys(self, *strings):
        d = bge_logic_keyboard.events
        return [d[KEYBOARD_MAP[s]] for s in strings]




class Mouse(ImputBase):
    def keys(self, *strings):
        d = bge_logic_mouse.events
        return [d[MOUSE_MAP[s]] for s in strings]




keyboard = Keyboard()
mouse = Mouse()





Imput? Don’t you mean Input?

Just from looking at the code I can’t really see what it is supposed to do … except it creates a map (which is internal code). As a beginner I would stop reading here. As an experienced reader I would question why an internal function is stated first.

As I want to review I will look what else does the code offer …

  • a set with two numbers
  • a dictionary supposed to be related to keyboard
  • a dictionary supposed to be related to a mouse
  • three classes
  • two instances of two of the classes - I guess this is what you want to offer

I do not see any users of that code. So it is hard to tell how to use it. To me it is not obvious.

So lets dig a bit deeper. All classes inherit from ImputBase. Assuming it means InputBase I can guess it has something to do with Input (keyboard, joystick, mouse). Base … a base class. The subclasses Keyboard and Mouse implement the device specifics - makes sense.

To see what the class can do I check the methods …

  • keys - hmm a property? can I get and set keys (from device)?
  • key - the same thing but a single entity what happens when I call it?
  • actives/active - active what? Do you mean keys? some verbs could improve what can be expected

Unfortunately I would need to dig deeper into the code to find out what to do. At that stage I would write my own code.

Regardless of that here are some remarks on improving the code:

  • create_map" is nice as it implies a map will generated. Unfortunately it does not tell what map that is, or what it contains. As it is marked as internal "” I (as reader) assume I do not need to know about - so I did not analyzed it.
  • ACTIVES_STATUS, KEYBOARD_MAP and MOUSE_MAP are only used in the according classes … Wouldn’t it be better to hide it inside the class? Why do I (the reader) need to know about? Am I supposed to manipulate them?
  • ImputBase.keys just throws an error. It looks like you try to implement some sort of “abstract” class (Java term). You can simply skip that method. If someone calls it … he gets an AttributeError already.
  • bge_logic_keyboard = bge.logic.keyboard looks strange. There is no additional information. It just adds more code. I can’t see how the code benefits from it. So I suggest to remove it.
  • This snippet could benefit from a docstring - I advice to add a use case example in there.
  • The code is good organized (except that the internals are listed first)
  • The inheritance tree makes sense

Hi Monster.

seem all rights points , i had thinked that UPPERCASE was already “internal” for some reason :smiley:
maybe is better add a underscore > _UPPERCASE < ?

the reason because i keeped almost all out of class are two:
1)performances:
DICT[k]
should be more fast than
self.DICT[k]

for a problem of name , since the class cannot store the real source -> bge.logic.keyboard.events
so i not know what name give to it… -> self.half_source ?
for that i had used -> bge_logic_keyboard (more clear but without too many punctations)

what about this?
better? the code seem a bit more short :wink:



import bge


_ACTIVES_STATUS = {1, 2}



class InputBase:
    def __init__(self, source, postfix=""):
        self._map = _create_map(source.events, postfix)
        self._source = source  #note that lack a piece... -&gt; .events (for that also i keeped it out)
    

    def keys(self, *strings):
        d, m = self._source.events, self._map
        return [d[m[s]] for s in strings]
    

    def key(self, string):
         return self._source.events[self._map[string]]


    def actives(self, *strings):
        d, m = self._source.events, self._map
        return [d[m[s]] in _ACTIVES_STATUS for s in strings]


    def active(self, string):
         return self._source.events[self._map[string]] in _ACTIVES_STATUS




class Keyboard(InputBase):
    pass
class Mouse(InputBase):
    pass


def _create_map(dictionary, postfix=""):
    numbers = {int(k) for k in dictionary}
    postfix = postfix.lower()
    strings = {}
    for att in dir(bge.events):
        numb = getattr(bge.events, att)
        if numb in numbers:
            att = att.lower()
            if att.endswith(postfix) and len(att) &gt; len(postfix):
                att = att[:-len(postfix)]
            strings[att] = numb
            strings[att.upper()] = numb
    return strings
    

keyboard = Keyboard(bge.logic.keyboard, "KEY")
mouse = Mouse(bge.logic.mouse, "MOUSE")



[EDIT:correct a bug]

the module work in this way:


import myinput       #input with the "n" has a strange color :D 


(...)


if myinput.keyboard.active("w"):          #return a bolean
    own.applyMovement(..)
## or
w,s,a,d = myinput.keyboard.actives("w","s","a","d")     #return 4 boleans


if myinput.keyboard.key("space") == 1:       #return 1 int (the status)
    jump()

w, s, a, d = myinput.keyboard.keys("w","s","a","d")       ##return 4 integers (the status)

By convention, UPPERCASE implies a constant value, and underscores imply a private member. Using double underscore ("__name") will mangle the name of the variable, such that it will not clash with subclasses, and is inaccessible (without inspection) from the outside scope (i.e you recall that a mangled member is accessible externally as “_X__name”, where X is the class name and __name is the name of the member). It also mangles the name for late-bound members:


&gt;&gt;&gt; class Foo:
...     __bar = 1
... 
...     def print_bar(self):
...         print(self.__bar)

&gt;&gt;&gt; foo = Foo()
&gt;&gt;&gt; foo.print_bar()
1

&gt;&gt;&gt; print(foo.__bar)
AttributeError: 'Foo' object has no attribute '__bar'

You don’t need to alias bge.logic.keyboard or bge.logic.mouse. You can reference the events dictionary directly if you wish (i.e keyboard_events = bge.logic.keyboard.events) without any problems.

I don’t know why you’re creating an input module that doesn’t really do anything.

I wrote a basic input handler for my game system here:
https://github.com/agoose77/PyAuthServer/blob/master/bge_game_system/inputs.py

This will create library events for bge events, (converting from BGE->InputButtons enum, and converting the state to a value of ButtonState).

I also wrote a remap “input context” to remap keys to named events (see LocalInputContext):
https://github.com/agoose77/PyAuthServer/blob/master/game_system/inputs.py

It’s very easy to start writing more and more complex input handlers, when there’s very little need. For example, to read multiple event states:


# Game loop-level manager
input_manager = BGEInputManager()
input_manager.update()

# Entity-level update
mapping = {InputButtons.AKEY: "a", InputButtons.AKEY: "b", InputButtons.AKEY: "c", InputButtons.AKEY: "d"}
buttons, ranges = local_input_context.remap_state(input_manager.state, mapping)

a, b, c, d = [buttons[x] for x in ("a", "b", "c", "d")]

I wrote my input manager with a specific requirement in mind - to be able to easily serialise remapped inputs and deserialise them without writing ugly boilerplate, and to support multiple game engines. Yours might not be that abstract.

I generally prefer hiding game engine specific features behind interfaces wherever possible, as you are doing, because using the engine’s own feature set implies design constraints (which are not always a bad thing). However, keep it simple unless you really need something more advanced :slight_smile:

“I don’t know why you’re creating an input module that doesn’t really do anything.”

is simply much more handly than the “buildin”

comparison:


#####
import bge
w = bge.logic.keyboard.events[bge.events.WKEY]
s = bge.logic.keyboard.events[bge.events.SKEY]
a = bge.logic.keyboard.events[bge.events.AKEY]
d = bge.logic.keyboard.events[bge.events.DKEY]


#####
import myimput
w,s,a,d = myimput.keyboard.actives(*"wsad")


what else?:rolleyes:


from bge import logic, events
w, s, a, d = [logic.keyboard.active_events.get(getattr(events, "{}KEY".format(x.upper())), False) == logic.KX_INPUT_ACTIVE for x in "wsad"]

I’m only slightly teasing :slight_smile:

see agoose77’s explanation.

There is no speed difference.
if you read the explicit form it would be:
module.DICT[k] vs self.DICT[k]

I also meant to turn it to a class variable rather than a class member. So you only get one instance for all objects (less memory). As you have just one instance of each class it does not really matter.

You can use “bge.logic.keyboard.events” inside your class. It tells the reader where it comes from and that there is a dependency. You have that dependency already … why not using it?
A shorter name (see agoose77’s post) might help writing, but is not really required.

In the new code you can remove the sub-classes completely. The instances are configured via configuration .


keyboard = Input(bge.logic.keyboard, "KEY")
mouse = Input(bge.logic.mouse, "MOUSE")

This is as you could mix the configuration and classes:


mouse = Keyboard(bge.logic.mouse, "KEY")

Alternative you can “hard-code” the parameters in the sub-classes as in your first post.

With the use case the module makes more sense.

I still think “actives” is a bit … strange looking. I suggest to name it “pressed” or “pressedKeys”

alternatives:

  • pressed(key)
  • justPressed(key)
  • released(key)
  • justReleased(key)

keys() with an integer does not provide more handling than the existing bge method. It even reduces the amount of information to two states … which can be expressed as boolean (True/False).

If you do not want to loose this information, how about a KeyStatus object?


...
def getKeyStatuses(keys):
   return &lt;list of KeyStatus&gt;
...
class KeyStatus():
   __init__(self, defaultAsciiKey): ...
   pressed(self): ...
   justPressed(self): ...
   released(self): ...
   justReleased(self): ...

than you can write things like that:


w,a,s,d = myinput.keyboard.getKeyStatuses("w","a","s","d")
if w.justPressed():
   ...
if s.pressed():
   ...

surely named with your names and conventions :).

There is no speed difference.
if you read the explicit form it would be:
module.DICT[k] vs self.DICT[k]

You can use “bge.logic.keyboard.events” inside your class. It tells the reader where it comes from and that there is a dependency. You have that dependency already … why not using it?

probably i falled fully in the “premature optimization” trap
but the difference exist as -> (timelocal:timeglobal) = (4 : 3)

X
is ever more fast than
X.X
that is ever more fast than
X.X.X.X.X.X


import time


DICT = {str(n): n for n in range(50)}
R50000 = list(range(50000))




def test():
    
    class C:
        def __init__(self):
            self.DICT = DICT
            
        def test(self):
            k = "1"
            
            
            t1 = time.clock()
            for _ in R50000:
                v = DICT[k]
            t2 = time.clock()
            print("use global: ", t2-t1)
            
            
            t1 = time.clock()
            for _ in R50000:
                v = self.DICT[k]
            t2 = time.clock()
            print("use local: ", t2-t1)
            
            
    c = C()
    
    c.test()
    





anyway … “optmize later” is ever a good idea… :stuck_out_tongue:

If you do not want to loose this information, how about a KeyStatus object?


w,a,s,d = myinput.keyboard.getKeyStatuses("w","a","s","d")
if w.justPressed():
   ...
if s.pressed():
   ...

except the name of method ( i not like much is the name -> “getKeyStatuses”),
seem a very good solution, since is absolutely readable
and get a supershort code(supposing one want just a list of bolaeans) is anyway possible using a pretty basic list comp .

w,s,a,d = [i.pressed() for i in myinput.keyboard.XX(“w”,“s”,“a”,“d”)]

the names of method of KeyStatus are instead absolutely clear.(not omissions)
in my opinion this will be a “good buildin”!


class KeyBoardKeyStatus:
    def _get_status(self): #main method
        return bge.logic.keyboard.events[self._number]
    def released(self):
        return self._get_status() is 0
    def just_pressed(self):
        return self._get_status() is 1
    def pressed(self):
        return self._get_status() is 2
    def just_released(self):
        return self._get_status() is 3

PS: But the real implementation has some part redundant hard to remove (90 lines :evilgrin:)

70…lines, removed all “optimizations”


import bge




class KeyStatus:
    def __init__(self, number):
        self._number = number
        
    def released(self):
        return self._get_status() is 0
    
    def just_pressed(self):
        return self._get_status() is 1
    
    def pressed(self):
        return self._get_status() is 2
    
    def just_released(self):
        return self._get_status() is 3






class KeyboardKeyStatus(KeyStatus):
    def _get_status(self):
        return bge.logic.keyboard.events[self._number]
    
class MouseKeyStatus(KeyStatus):
    def _get_status(self):
        return bge.logic.mouse.events[self._number]
    




class InputBase:
    def __init__(self, instances):
        self._instances = instances
        
    def _get_instances(self, *strings):
        return [self._instances[s.upper()] for s in strings]
    
    def keys(self, *strings):
        return self._get_instances(*strings)
    
    def key(self, string):
        return self._get_instances(string)[0]
        
    


def input_factory(kind="keyboard"):
    def get_map(numbers, suffix=""):
        strings = {}
        suffix = suffix.upper()
        for att in dir(bge.events):
            numb = getattr(bge.events, att)
            if numb in numbers:
                att = att.upper()
                if att.endswith(suffix) and len(att)&gt;len(suffix):
                    att = att[:-len(suffix)]
                strings[att] = numb
        return strings
    
    if kind == "keyboard":
        instances = {s:KeyboardKeyStatus(n) for s,n in get_map(bge.logic.keyboard.events,"KEY").items()}
    elif kind == "mouse":
        instances = {s:MouseKeyStatus(n) for s,n in get_map(bge.logic.mouse.events,"MOUSE").items()}
        
    return InputBase(instances)


keyboard = input_factory("keyboard")
mouse = input_factory("mouse")





The optimization idea is usually nice and it is important to keep potential bottlenecks in mind. But you always need to consider the other aspects of development. In most cases it is more important to keep the code readable, otherwise it can be really hard to be maintained.

Lets look at the use cases of your code:

  • it is used for keyboard input.
  • if you have more that 10 users at the same time the design of the game should be reviewed
  • 10 times your controls is not really much that deserves hard optimizations
  • if you really rely on speed … you would need to move that to native code … rather than Python

-> no need to press the last microsecond out of the processing time.

With your changes the code looks much simpler and is easier to read.
I still have a few minor remarks:

  • Rather than checking 0, 1, 2, 3 it would improve the readability by using the predefined constants from the BGE API. Nevertheless the function names already tell what is meant. Therefore it would be a small improvement
  • The code would be even smaller by defining status as (read-only) property -> look the Python docs (@Property annotation). The changes to your code are really small
  • The name InputBase does not match that good anymore … maybe you reduce it to Input
  • I’m not a native English speacker … but I would replace “kind” with “type”
  • You might already noticed I prefer when functions tell what they are doing. I would read “input_factory” sounds like “do input a factory” - I’m pretty sure this is not what you mean. As a class this name would be fine because I expect a “input factory” creates an object “input”. As this is a factory method (rather than a class) I suggest to name it “createInput” or “buildInput”.
  • You can define public constants to allow users to enter the input type. It prevents typing errors too (“mouse” vs “Mouse”):

INPUT_KEYBOARD = "keyboard"
INPUT_MOUSE = "mouse"

  • I strongly recommend to raise an error it the entered type is not processed. Otherwise the user does not get any feedback why the code is not working. The current processing [“InstanceBase(None)”] would result in a NoneType error at a later stage .

if kind ...
elif kind ...
else raise ...

  • Currently I’m a little bit confused, why the prefixes “KEY”, and “MOUSE” are present just once. Typically you need this twice - one on writing and one on reading. If it is present just once it is usually a sign, that it is not needed for processing (but can help to read the data). [edit]Now I know … because the the it is the prefix provided by the BGE API - so … it is fine[/edit]
  • You explicitly change prefix to upper case. It is entered as uppercase already. I think this is a touch to much especially as the prefix is provided by the same module (not the user). The same belongs to the “att” variable.