Question about coding practice

[SirAskLots]

Hi panda forums! I’ve been lurking for some time now and I was wondering if you could answer a question of mine? I am creating some basic movement for ralph and I currently calling the self.accept stuff in the init part of the class. I was wondering if it was ok to use a task to call a function to deal with the self.accept stuff instead of having it in the init part of the class.
Basically goes from this:

def __init__(self):
        self.keyMap = {"w" : False,
                "s" : False,
                "a" : False,
                "d" : False,
                "mouse1" : False,
                "mouse3" : False}
        self.accept("w", self.setKey, ["w", True])
        self.accept("s", self.setKey, ["s", True])
        self.accept("a", self.setKey, ["a", True])
        self.accept("d", self.setKey, ["d", True])
        self.accept("w-up", self.setKey, ["w", False])
        self.accept("s-up", self.setKey, ["s", False])
        self.accept("a-up", self.setKey, ["a", False])
        self.accept("d-up", self.setKey, ["d", False])
        self.accept("mouse1", self.setKey, ["mouse1", True])
        self.accept("mouse3", self.setKey, ["mouse3", True])
        self.accept("mouse1-up", self.setKey, ["mouse1", True])
        self.accept("mouse3-up", self.setKey, ["mouse3", True])

To this:

    def __init__(self):
        self.keyMap = {"w" : False,
                "s" : False,
                "a" : False,
                "d" : False,
                "mouse1" : False,
                "mouse3" : False}
        taskMgr.add(self.control, "Controls")
    def control(self,task):
        self.accept("w", self.setKey, ["w", True])
        self.accept("s", self.setKey, ["s", True])
        self.accept("a", self.setKey, ["a", True])
        self.accept("d", self.setKey, ["d", True])
        self.accept("w-up", self.setKey, ["w", False])
        self.accept("s-up", self.setKey, ["s", False])
        self.accept("a-up", self.setKey, ["a", False])
        self.accept("d-up", self.setKey, ["d", False])
        self.accept("mouse1", self.setKey, ["mouse1", True])
        self.accept("mouse3", self.setKey, ["mouse3", True])
        self.accept("mouse1-up", self.setKey, ["mouse1", True])
        self.accept("mouse3-up", self.setKey, ["mouse3", True])
        return task.cont

I know this still works as I have already tried it, but I was wondering if there was a better or “correct” way of doing this. You are probably wondering why I would do such a thing. The short answer is: I am attempting to load key strokes from a file and the only way I can accept a key is if I already know what that file says. For example: file says “w” is actually going to be “h” I would store “h” into self.keyw then apply it during control function via self.accept(self.keyw, self.setKey, [“w”, True]). Any advice or better ways of doing it would be nice. Thank you.

[Tober]

In your second example, you’re for some reason calling control() every frame. It only needs to be called once and doesn’t need to be a task at all.

I presume you are later going to query this class to find out if “s” is True, and you’ve set it up like this so you can remap “s” to “f” or whatever and the code still works. In this case you should definitely consider renaming the variable “s” to a description of what it does. The code doesn’t need to know the player pressed “s” so much as the player “wants to move backwards”. Which key the player presses to signify that is arbitrary, so it shouldn’t be used in the python code except for the original bindings of key presses to methods.

Also, your plan to for some reason read the key names out of a file and call them self.keyw is just silly. You can read the key names and bind them in the same method; the class doesn’t need to remember what they were after the method is finished. TaskMgr remembers the bindings for you.

[SirAskLots]

In your second example, you’re for some reason calling control() every frame. It only needs to be called once and doesn’t need to be a task at all.

I simplified the task quite a bit before posting here and messed up on the task.cont it should of been task.done. But I am using a task because there is actually other things going on in this function and I want to control when this function is run and what order (in comparison to other functions) it is run at.

I presume you are later going to query this class to find out if “s” is True, and you’ve set it up like this so you can remap “s” to “f” or whatever and the code still works. In this case you should definitely consider renaming the variable “s” to a description of what it does. The code doesn’t need to know the player pressed “s” so much as the player “wants to move backwards”. Which key the player presses to signify that is arbitrary, so it shouldn’t be used in the python code except for the original bindings of key presses to methods.

You presume correctly. The function that does this is setKey. There is then a task named Player Control that goes through and checks the setKey dictionary for objects that are true and performs tasks accordingly. So I don’t understand what part of that is a problem.

Also, your plan to for some reason read the key names out of a file and call them self.keyw is just silly.

I fail to see how having one program write key strokes captured from the user to a file and then another program load the file up and read those key strokes as being silly. Do you know of a better way of doing this? Maybe providing some code instead of nay-saying would be more helpful.

[Thaumaturge]

I largely agree with Tober above, I believe.

That said, I think that I see what you’re aiming for: it might arguably be a little cleaner to have a method named “setControls” (or something similar) which is called when you want to establish the controls.

If you want to allow it to react to changes in the control scheme, you could always allow it to take in a key dictionary, or have another method that takes a key to be replaced, something like this:

# I'm using a set of constants here to avoid
# matching strings, which is somewhat prone
# to simple typoes.
UP = 0
DOWN = 1
LEFT = 2
RIGHT = 3
ACTION_1 = 4
ACTION_2 = 5

def __init__(self):
    # This is the dictionary that you actually query
    # for game control
    self.controls = {UP : False,
                   DOWN : False,
                   LEFT : False,
                   RIGHT : False,
                   ACTION_1 : False,
                   ACTION_2 : False} 

    # This is the default key layout.
    # Storing this allows us to reset
    # our key-mapping later, if so desired.
    self.defaultKeys = {UP:"w",
                        DOWN:"s",
                        LEFT:"a",
                        RIGHT:"d",
                        ACTION_1:"mouse1",
                        ACTION_2:"mouse2"}

    # This stores our current key-mapping
    # so that we can cancel our bindings when
    # changing key-mappings.
    self.currentKeys = {}
                        
    self.setControls(self.defaultKeys)

def setControls(self, keyMap):
    for action, key in keyMap:
        self.setControl(key, action)

# Having this a separate method allows us to change
# a single key at a later stage if so desired.
def setControl(self, key, action):
    # We don't want the old key setting off
    # the event, presumably
    if action in self.currentKeys:
        # I think that this is correct -- you might
        # want to check that this does indeed cancel
        # all future events from the given key.
        self.ignore(self.currentKeys[action])
    self.accept(key, self.setKey, [action, True])
    self.accept(key+"-up", self.setKey, [action, False])
    self.currentKeys[action] = key

[SirAskLots]

@Thaumaturge

self.defaultKeys = {UP:"w",
                    DOWN:"s",
                    LEFT:"a",
                    RIGHT:"d",
                    ACTION_1:"mouse1",
                    ACTION_2:"mouse2"}

Using another dictionary makes sense. Thank you and the idea should be helpful.

Still don’t understand why having a permanent place in a file for key strokes is such a bad idea?

[Thaumaturge]

I think that it depends on what you mean: if you’re simply writing out user preferences to a file for use on the next run, then I don’t see a problem. If that’s not your intended purpose, then what is?

[Tober]

@SirAskLots

I simplified the task quite a bit before posting here and messed up on the task.cont it should of been task.done. But I am using a task because there is actually other things going on in this function and I want to control when this function is run and what order (in comparison to other functions) it is run at.

I don’t understand… if you’re setting up a task that always returns task.done the first time it’s run, how is this in practice different from calling a method instead?

You presume correctly. The function that does this is setKey. There is then a task named Player Control that goes through and checks the setKey dictionary for objects that are true and performs tasks accordingly. So I don’t understand what part of that is a problem.

The problem is: your Player control is checking the setKey dictionary and seeing if “w” is True or False… but what currently decides whether “w” is true or false might be whether the player is holding down “f” if they’ve remapped their keys. So why call it “w” if it’s only sometimes telling you if “w” is pressed?

I fail to see how having one program write key strokes captured from the user to a file and then another program load the file up and read those key strokes as being silly. Do you know of a better way of doing this? Maybe providing some code instead of nay-saying would be more helpful.

I’m sorry, I’m having a hard time understanding exactly what you mean. There’s no definition of program where your usage of it makes sense. Thaumaturge’s solution is adequate for what you want to do, in any case.

Still don’t understand why having a permanent place in a file for key strokes is such a bad idea?

It’s not that it’s a bad idea for it to know what key strokes it has so much, it’s that you didn’t need to do that in order to do the thing you wanted to do. What keys are mapped to what functions is not a thing your class needs to know in the way you told it. The class automatically keeps track of it using the methods and attributes it inherited from the controller class. It could be argued that you need them to display to the player when they are rebinding, but you could always just reread the file you read the bindings from anyway.

Completely aside from whether the class needs to know that information, the way you suggested storing it is bad practice. That’s the main reason I called it silly. Like Thaumaturge suggested, a dictionary is a much more sensible way to store the data.

[SirAskLots]

@Tober

I don’t understand… if you’re setting up a task that always returns task.done the first time it’s run, how is this in practice different from calling a method instead?

Tasks have priorities and sort therefore you can put like functions together and have them run in a particular order. Methods run as soon as they are called. Why is this important? well if I have a method that reads a file I wouldn’t want the method immediately after it(which uses that file information) to begin running before the prior method completes its task. Using a task guarantees that this won’t happen. There are other uses as well but this was my case.

The problem is: your Player control is checking the setKey dictionary and seeing if “w” is True or False… but what currently decides whether “w” is true or false might be whether the player is holding down “f” if they’ve remapped their keys. So why call it “w” if it’s only sometimes telling you if “w” is pressed?

The “w” dictionary spot is just a true or false entry so anything I attach to that entry through self.accept should be ok. I realized that I should have posted this as the accept code because that is what I am actually using: self.accept(anyKeyYouWant, self.setKey, [“w”, True])
The player control method deals with anything in quotes that I put in anyKeyYouWant due to the way I have written it. I gotta hand it to you, however, I went and threw a couple of odd strokes (such as l,6,') just to see if it worked because you made me second guess myself. So that code roughly looked like:

anyKeyYouWant = "6"
self.accept(anyKeyYouWant, self.setKey, ["w", True])

so on and so forth with l,',etc.
It works entirely fine and I have no idea why I shouldn’t do this.

I’m sorry, I’m having a hard time understanding exactly what you mean. There’s no definition of program where your usage of it makes sense. Thaumaturge’s solution is adequate for what you want to do, in any case.

My bad I haven’t slept in 24 hours so I am sure that is not helping. My program is going to have a title menu that accepts information from the user like keys and the such while in the background I am going to be loading the actual game to cut down on load time. Yes I could create a bunch of calls to the title class and apply them to the control class but then none of those controls that the user sets up would be there next time the program loads. Furthermore, the garbage collection would be more extensive due to those extra class to class calls. Which bring about why I wanted to use a task. I can make the task wait until the user is done in the title menu before running the method and loading the key strokes. Also I like Thaumaturge’s solution and it works, and a list would also work. Not sure which is faster? (betting dictionary however I am not sure)

Completely aside from whether the class needs to know that information, the way you suggested storing it is bad practice. That’s the main reason I called it silly. Like Thaumaturge suggested, a dictionary is a much more sensible way to store the data.

I know not of any method that will store a variable after the program has completed and garbage collection has cleaned up the classes,etc. Maybe you do? I really don’t want my user to have to remap keys every time the program starts up. I would imagine that to be extremely annoying. So storing in a file makes sense in that situation and is not silly. Now while the program is running… well that might be a different story but I kinda doubt loading a small file once is going to cause any kind of performance effect. Please feel free to correct me if I am wrong.

[Thaumaturge]

I think that the main objection to calling your dictionary key “w” (or “d”, etc.) is that it makes your code less clear: the key doesn’t reflect what’s actually going on. If, for argument’s sake, you later decide that you want to change the default control scheme such that the arrow keys move the player and “w” and “e” operate the two actions, you then have a situation in which commands from the “w” key are being checked under a dictionary key named “mouse1” and commands from the up arrow are being checked under a dictionary key named “w” – in short, “w” doesn’t map to “w” any more.

Of course, you may not encounter this problem in your current project. But what happens if you decide to reuse this code for the next, and the one after that, etc.? Having clearly-named variables and keys can prevent problems further down the line.

[edit]
As to the files, I think that the problem here may be that your description of how you’re using them has been a little unclear. I would imagine that all that would be called for would be to load from file at startup (if the file exists) and save to file when the key-bindings are changed (and then only after they’ve been changed – there’s little good reason to perform the saving until the user has finished making whatever changes they desire, I think).

I may be missing it, but I don’t see a good reason to read from or write to the file outside of those points.
[/edit]

As to the tasks, it reads as though you’re worried about threading issues, and threading is not a strong point of mine, I fear. Nevertheless, unless you’re doing something that I’m not seeing, either because it’s not included in the code that you posted or because I’m missing it, I’m not sure that there’s as great a problem as you believe (or even any), and I’m not sure that using tasks would help you if it were…

That said, I stand for correction on any of these points.

My bad I haven’t slept in 24 hours so I am sure that is not helping.

This is known to have undesired side-effects, yes.

I hope that you get some good sleep soon!

[Tober]

I’m with Thaumaturge on this one. Your objections to what we’re saying make no sense because they rely on information you didn’t share with us (or not clearly enough).

If you’re not using threads, then a method in python is guaranteed to return (finish what it’s doing) before the next method is called. So what you’re suggesting simply isn’t a problem.

Both Thaumaturge and I have explained why using “w” in your setKey dictionary is a bad idea. I don’t know how else to say it.

I know not of any method that will store a variable after the program has completed and garbage collection has cleaned up the classes,etc.

You seem to have made an assumption about something that I missed. At no point did I ever come anywhere near suggesting you didn’t need to save information to a file to make it survive the program closing. I simply said the class you have doesn’t need to remember the key maps in the way that you suggested writing it. I didn’t know that you were displaying a key map to the player in the loading screen. That changes things considerably. If you were setting it up like that, then yes, having the class know what keys are mapped to is needed. But once again, you didn’t tell us that, so how were we supposed to know you needed to do that?

You’re also confused about what exactly I said was silly. I never said saving data to a file was silly. I said saving the key maps to self.keyw etc. was silly. Within the problem you presented, the class simply doesn’t need to know this bit of information except to call self.accept, which as I said, can be done in the same method as the one that reads the key binds from a file. Therefore, saving the information to a self.attribute is pointless because no other part of your class needs to know what this value is. That you happened to need to know what the key binding is in your specific instance is the sort of thing you need to say, because it’s not a typical case. And as I said again, I called it silly mainly because of the weird way you chose to store the data, not that you chose to store it.

I’m hoping this has cleared up some of the confusion.

[SirAskLots]

Here is how I fixed my problem. I just wanted to give everyone an update. After reading a file into an array (list in python) I then defined this:

#begin control function
    def control(self, array): #takes key stroke array and sets up controls

        self.accept(array[0], self.setKey, ["w", True])
        self.accept(array[1], self.setKey, ["w-up", True])
        self.accept(array[2], self.setKey, ["s", True])
        self.accept(array[3], self.setKey, ["s-up", True])
        self.accept(array[4], self.setKey, ["a", True])
        self.accept(array[5], self.setKey, ["a-up", True])
        self.accept(array[6], self.setKey, ["d", True])
        self.accept(array[7], self.setKey, ["d-up", True])
        so on and so forth....
#end control function

As was pointed out earlier in this thread I could of used a dictionary. I just felt this was easier as I was already loading a file into an array line by line.