[SOLVED]Need help on improving my code with Players and Skills

[Nero]

Hey, I just need some advice in improving my current code. Right now I accomplished what I wanted but thing is I did it in very poor way that it gets very frustrating if i decide to have lets say 50+ skills in my game this would be pain to do my way.

The idea here is that I got a 3 players which are put inside a list. Screen shows 3 players that are inside this list called "party_members". 1 of these players gets debuffed with lets say poison 2 turns after that happens small icon pops up beside player name with cooldown number 2 inside of a picture.
As I said I did exactly what I wanted in this code example but in a poor way.


So first off I was using my Player class for everything like skill cooldowns, skill images, enemies all that without creating other classes like Skill class Enemy class etc.
Now I'm more interested in writing the code a correct way and doing it right with classes and functions. I want to know how do I connect 2 classes together and attempted to do it in my code but im not sure if I did this correctly?

Second I used to create skill1_duration, skill2_duration etc for every skill i created i used 1 more line in Player class to do so. Now I want actually to define a skill just as I did it with player "default player1 = Player("Player1",100,10)" and actually use that Skill class I created. So no matter what debuff skill is being casted on Player it will always show the right cooldown and right image without me needing to add skill3_duration,skill4_duration etc.

So to make you understand simply what I want is exact same thing I have right now but written in right way.

Code: Select all

init python:
    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER",skill1_duration=0,skill_image="SKILL"):
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            self.skill1_duration = skill1_duration # MY POOR VERSION OF SKILL COOLDOWNS...
            self.skill_image = skill_image # THIS IS ALSO WRONG I WANT TO CALL THIS FROM "Skills" Class not in here...



init python:
    class Skills(Player): # I want this and Player object to be connected in code but how do I do this is this the right way?
        def __init__(self,name="NoName",damage=0,picture="NoImage",duration=0,target=None):
            Player.__init__(self,name,hp)
            self.name = name #Name of the skill
            self.damage = damage #Damage skill will inflict each turn
            self.picture = picture #Skill image for tooltips 120x120 and effect display 35x35
            self.duration = duration # Skill duration -1 per terun
            self.target = target # Which player will be attacked



default player1 = Player("Player1",100,10)
default player2 = Player("Player2",150,10)
default player3 = Player("Player3",200,10)

default party_members = [player1,player2,player3]


screen battle_test_screen():
    hbox:
        xalign 0.250
        yalign 0.400
        spacing 80
        for player in party_members: # Always show players pictures on screen
            if player.hp > 1:
                add player.picture


    hbox:
        xalign 0.250
        yalign 0.450
        spacing 80
        for i, player in enumerate(party_members):
            if player.skill1_duration >=1 :
                text "Skill Cooldown [player.skill1_duration]"
                add player.skill_image




# The game starts here.

label start:
    show screen battle_test_screen

    "Game starts"
    "Lets add skill duration"
    $ player1.skill1_duration = 2
    "This worked nicely"
    # Just an example here...
    #

    $ player1.skill1_duration -= 1
    "Turn 1 decrease skill duration"
    $ player1.skill1_duration -= 1
    "Turn 2 decrease skill duration and debuff skill image is removed as expected..."

    "Everything here worked as I want it to work only problem is in code that is written very poorly. So please if someone can help me in inproving my code..."

    return

[DannX]

Could you clarify why do you need to have your Skill class inherit from Player? I think you can just make them independent, and, for example, add each player's skills as a parameter, maybe a list or a dictionary. Here's what I mean:

Code: Select all

init python:
    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER", skills=[]):
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            self.skills = skills # a list of skills the player knows
    
    # Skill class
    class Skill(object): 
        def __init__(self,name="NoName",damage=0,picture="NoImage",duration=0,target=None):
            self.name = name 
            self.damage = damage 
            self.picture = picture
            self.duration = duration
            self.target = target 

#Some skills defined with default, with their own parameters
default attack = Skill(name="Attack", damage=10, duration=0)
default poison = Skill(name="Poison", damage=5, duration=2)

default player1 = Player("Player1",100,10, skills=[attack]) 
default player2 = Player("Player2",150,10, skills=[attack, poison])
default player3 = Player("Player3",200,10, skills=[poison]) 

This way you can make it so each player has their own skills, and with a little more functions you can add any number you like, remove them, etc.

[Nero]

I don't know if i needed to inherit from Player class since you said those 2 classes can both be objects then there is no need I guess(im asking because it looks like its needed in "screen battle_test_screen" or maybe im wrong)? I was never before mixing classes like that I used only 1 class and did everything inside of it now it backfired at me. Now im trying to understand how 2 classes can work together but defined independently.

I understand everything you did here now problem is in "screen battle_test_screen():" in those cooldowns how they can be displayed for every player and every skill.
How Player class and Skill class can show me what is the poison duration for player 3.. or player 1 for example? Which player is affected by poison and on which player will pup up the small poison icon above his avatar. Just like in my first example of course i used text to simulate that but you get the idea.

I'm pointing to exactly this line of code which is very confusing I just cant understand how to do this part? I used to do it this way keep making skill_duration numbers which is very bad to do. Could you give me an example how this should look when implemented with your code above so I can get the idea how those classes interact with each other and the logic behind the code.

Code: Select all

screen battle_test_screen():
    hbox:
        xalign 0.250
        yalign 0.450
        spacing 80
        for i, player in enumerate(party_members):
            if player.skill1_duration >=1 :
                text "Skill Cooldown [player.skill1_duration]"
                add player.skill_image
            if player.skill2_duration >=1 :
                text "Skill Cooldown [player.skill2_duration]"
                add player.skill_image
            if player.skill3_duration >=1 :
                text "Skill Cooldown [player.skill3_duration]"
                add player.skill_image

[DannX]

Ah, I think now I understand what you need better.

I don't have renpy at hand at the moment so I can't give you a working implementation, but let me explain the logic a bit:

Using the method I described earlier, apart from the skills themselves, you need to implement a way for their effects to be applied to each player independently. So if an enemy hits them with a poison-inducing skill, the player is inflicted with the Poison status ailment. Specifically you need to keep track of wich player has wich ailment, or buff, debuff, etc., and for how long. For this, what I do is create yet another parameter for the player class, a list that holds all of the status changes affecting him and the duration of those ailments.

So:

Code: Select all

 
 init python:
    class StatusChange(object): 
        def __init__(self, name="", damage=0, picture="",duration=0):
            self.name = name 
            self.damage = damage 
            self.picture = picture
            self.duration = duration

    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER", skills=[], status=[]):
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            self.skills = skills # a list of skills the player knows
            self.status = status # a list of status changes, ailments or buffs the player holds            
default poison = StatusChange(name="Poison", damage=25, duration=2) #example poison effect, would inflicts 25 damage for 2 turns

You can then add a parameter to your skill class, so each skill can inflict one or more ailments or status changes.

To represent this on a screen, you can do something similar to what you did in your first example:

Code: Select all

# THIS CODE IS UNTESTED, WILL PROBABLY NEED TWEAKING:
screen battle_test():
hbox:
    for p in party_members: #for every member in your party_members list
            text p.name #display their name
            if len(p.status) > 0: #if their status list is not empty
                vbox: 
                    for s in p.status: #for each status change in their status list
                        text "{}, {} turns".format(s.name, s.duration) #display the status' name, and it's duration

This (theoretically, I can't be sure it will display itself correctly) will display each party member's status effect and their cooldown time.

There is a lot more to do with this, but the bottom line is that when you work when classes and objects, everything you create can be considered
an object, and the most basic way to establish relations between them is to give them meaningful properties. What I do is to try to think how this works in games I have played, by seeing and trying to figure out how can I replicate that behaviour using what Ren'Py an Python.

I hope I made sense here, when I have the time I'll try to make a proper working example.

[Nero]

Hmm I did something similar like this before but again same problem. It seems it works but problem is that every player get affected by this effect even I gave poison effect to a player2 status list... or maybe I'm appending this the wrong way? Im using this line to append to status "$ player2.status.append(poison)" is this correct?

And second thing I know I will need to make a Function that will have to decrease duration of effect on every player that gets inflicted and when duration gets to 0 it should just remove status from the list.
I'm interested to see how would you make it decrease the duration of inflicted player right now looking at your method I did something like this however it decreases cooldown by -3 instead of -1 I understand that it is because 3 players in list but how to have their cooldowns decreased independently?

Code: Select all

init python:
    def next_turn(): # Function that will run each time the player have its turn
        for p in party_members:
            for s in p.status: #for each status change in their status list
                s.duration -= 1

And here is the full code I have right now. So 2 errors. One player gets debuffed everyone else gets debuffed somehow. Cooldown is decreased by -3 instead of -1 seems like those 2 errors share the same problem.
By the way thanks a lot for helping me it means a lot to me I see its getting closer and closer to a solution.

Code: Select all

init python:
    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER", skills=[], status=[]):
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            self.skills = skills # a list of skills the player knows
            self.status = status # a list of status changes, ailments or buffs the player holds

init python:
    class StatusChange(object):
        def __init__(self, name="", damage=0, picture="",duration=0):
            self.name = name
            self.damage = damage
            self.picture = picture
            self.duration = duration


default player1 = Player("Player1",100,10)
default player2 = Player("Player2",150,10)
default player3 = Player("Player3",200,10)

default party_members = [player1,player2,player3]
default poison = StatusChange(name="Poison", damage=25, duration=2) #example poison effect, would inflicts 25 damage for 2 turns


screen battle_test_screen():
    hbox:
        xalign 0.250
        yalign 0.400
        spacing 80
        for player in party_members: # Always show players pictures on screen
            if player.hp > 1:
                add player.picture

screen battle_test_screen():
    hbox:
        xalign 0.250
        yalign 0.400
        spacing 80
        for p in party_members: #for every member in your party_members list
            text p.name #display their name
            if len(p.status) > 0: #if their status list is not empty
                vbox:
                    for s in p.status: #for each status change in their status list
                        text "{}, {} turns".format(s.name, s.duration) #display the status' name, and it's duration


# The game starts here.
init python:
    def next_turn(): # Function that will run each time the player have its turn
        for p in party_members:
            for s in p.status: #for each status change in their status list
                s.duration -= 1





label start:
    show screen battle_test_screen

    "Game starts"
    $ player2.status.append(poison)
    "Lets test it"
    "Seems like every player got affected even we appended poison to Player2."
    $ next_turn()
    "Lets see duration decrease each turn."
    $ next_turn()
    "Nope something is wrong duration is decreased by 3."
    return

[Rastagong]

Both bugs come from the __init__ constructor of the Player class! More specifically, from the status parameter, where you used an empty list ([]) as a default value for status.

To begin with, lists are mutable: they can be modified (with append(), remove(), etc).

The problem is that in Python, when you define a mutable object (the [] empty list) as default value for a function parameter (status=[]), the mutable object is created only once, at the time the the function is created, and then is reused each time you call that function.
You expected a new [] empty list to be created for each Player, each time you called Player(), right?
That does not happen in Python: the same and unique list is reused each time you create a new Player. In other words, all Players share the same status list.
Since all Players share the same list, if you add a poison effect to the list of one Player, then all Players will share the same Poison effect too.

More details but skip this section if you want

If you're interested in more details, note that this happens only because a list is a mutable object:
Its contents can change. You can create an empty list with status = [], add items with status.append(poison), remove items later, etc.

But not all objects are mutable: some are immutable, and their value cannot change.
If status was not supposed to be a list, but say, a string instead (for instance "poison" or "sleeping"), there would be no problem with setting a default value (for instance status="normal"), because strings are immutable.
The "normal" status would still be shared by all Players, but since it is immutable, changing the status of one Player would not change the status of the other Players.
Since it's impossible to change the value of an immutable object, the only way to modify an immutable variable is to bind the variable to a new, different value, which is therefore different from the one shared by all Players!

For instance, to change player2's status to poisoned, we could do player2.status = "poison" and this would only affect player2's status, by putting a wholly different string ("poison") in the place of "normal": while "normal" was shared among Players, the new value we assign to player2's status is unique to player2! It's not the same object to begin with: since we cannot change the value of immutable objects, we literally replaced the object, and this freed us.

This differs from the situation where status is a list (and therefore mutable), where appending a new effects is always done by referring to the same object, the one shared shared among Players. When you do player2.status.append(poison), you append poison to the object at player2.status, but this object is the one shared by all Players.

I'm not sure if I'm very clear on this whole thing, so here are additional links if you're interested in the topic:

• The Hitchhiker’s Guide to Python has a section dedicated to “common gotchas”, one of them being Mutable default arguments. It's a very clear and succinct explanation!
• Drastically Improve Your Python: Understanding Python's Execution Model is an article which explains in detail what objects are, and in particular how immutable/mutable objects work.

Anyway. How can we solve this mutable default value problem?

It's still probably desirable to use a list to store the status of the player! I talked about immutable types in the previous section, but in your case, a list is clearly the best choice, since you want Players to be able to be affected by multiple effects.
And you probably still want to have an empty status (an empty list) be the default case (with a default value).

The solution to use mutable objects like lists as default cases is to instantiate the default object only in the body of the function.
In your case, in the __init__ constructor of the Player class, there should be a self.status = [].
This way, the mutable object is created only for the current Player instance (self), and for no one else.

However, we still need to have a default value in the function parameters, to allow Players to be instantiated both without any status by default, for instance with player1 = Player(), and with a custom status if needed, as in poisonedPlayer = Player(status=[poison]).
To do so, people usually put None as a default value: so we would have status=None instead of status=[].
The goal of None is just to be a marker which tells the constructor “this is a default status case, create an empty status list for this player”:
If the __init__ constructor sees that the status parameter is None, it sets self.status to an empty list (as I was saying above).
If it finds any other value than None, it sets self.status to that value, since that value is the intended status.
(You could actually choose any other default value than None, though preferably an immutable one, and one that is not used anywhere else. None signifies this pretty well.)

One last note: in your Player class, you used an empty list as a default value not only for status, but also for skills. So we also need to use None as a default value for skills, or you'll have similar problems with it later down the line.
The other parameters are already good, since they have immutable default values: name and picture have strings as default values, and strings are immutable. hp and en have numbers (integers) as default values, and those are immutable too.

So here is the final modified Player class:

Code: Select all

    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER", skills=None, status=None):
            if skills is None: #We use None as a default value to signify “create an empty list by default”
                skills = []      #An empty list should not be put as a default value directly, since lists are mutable
            if status is None: #Idem
                status = []
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            self.skills = skills # a list of skills the player knows
            self.status = status # a list of status changes, ailments or buffs the player holds

[DannX]

You expected a new [] empty list to be created for each Player, each time you called Player(), right?
That does not happen in Python: the same and unique list is reused each time you create a new Player.

Thank you for clarifying this, Rastagong, I had always assumed a new list was being created for each instance, and as I always overwrote them later, I never noticed all instances where using the same list all along. I'll be sure to read the links you suggested so this doesn't happen again.
Sorry for the trouble

[Nero]

Okay so I get it Python uses one list as I defined them self.status = status(global list for every player I can see this method can be used somewhere else in my project) instead of using self.status = [] (separated list for each player) . Is it really necessary to have list with list = [None] in it if it is empty instead of just having list = [] are there any particular downsides to it sorry if i didn't get this part correctly? Because you gave me idea to do something like this then:

This does the exact same thing however when its empty it doesn't give a list None item.

Code: Select all

init python:
    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER", skills=[], status=[]):
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            self.skills = [] # a list of skills the player knows
            self.status = [] # a list of status changes, ailments or buffs the player holds

You said goal of None is just to be a marker? So you meant this just helps me identify that list is empty and that it is not necessary to have it in code or in a long run this might prove to be useful to have? Just curious about your opinion.
So if I needed to check if this list is empty for some reason I would do this:

Code: Select all

    default x = []
    if not x:
        "Nothing in list"

Ok now that I figured out how this work I'm still in a second problem. Even if I have separated lists created this code will still decrease skill duration based on how many debuffs are in list no matter what player is affected. By this I mean if for example player1 and player2 get poison effect for 5 turns next_turn function will decrease duration for individual player by -2 even I said to decrease it for -1 per player. This time duration is not a list

You said problem for this bug is also in Player class but defining separated lists for players didn't fixed this problem Python still look at this as it is one single list and decreases duration based on number of debuffs in those lists?

This function runs every turn:

Code: Select all

init python:
    def next_turn(): # Function that will run each time the player have its turn
        for p in party_members:
            for s in p.status: #for each status change in their status list
                s.duration -= 1

DannX it is okay you tried to help me im grateful you came closer to solution than I did

[DannX]

Ok now that I figured out how this work I'm still in a second problem. Even if I have separated lists created this code will still decrease skill duration based on how many debuffs are in list no matter what player is affected. By this I mean if for example player1 and player2 get poison effect for 5 turns next_turn function will decrease duration for individual player by -2 even I said to decrease it for -1 per player. This time duration is not a list

You said problem for this bug is also in Player class but defining separated lists for players didn't fixed this problem Python still look at this as it is one single list and decreases duration based on number of debuffs in those lists?

That is another mistake on my part (I swear I will never again fly-type code like this without testing it first). You see,the way it is, as poison is a single
instance, it exists in each player's list and it is decreased each time. So to fix this, what you can do is pass new status instances to each player, and the only way I can think of right now to do this, maybe (probably) there's a better one, is to copy it like this:

Code: Select all

# add this method to your player class
        def add_status(self, status):
            
            from copy import deepcopy #import deepcopy so we can append a new object with the same properties
            
            #This is so the same status is not added twice, just in case
            for s in self.status:
                if status.name == s.name:
                    return
            
            self.status.append(deepcopy(status)) #we append the new status to the character's list

default silence = StatusChange(name="Silence", damage=0, duration=5) #another ailment to test

label start:
    show screen battle_test_screen

    "Game starts"
    $ player2.add_status(poison)
    $ player2.add_status(silence)
    $ player3.add_status(poison)
    "Lets test it"
    
    "Player2 and Player3 were afected with different status."
    $ next_turn()
    "Lets see duration decrease each turn."
    
    $ player1.add_status(silence)
    "Lets add a status to player one too."
    $ next_turn()
    "Another turn passes. Seems ok."
    return

[Nero]

Interesting it works now its first time I see something like this used in code though "from copy import deepcopy" I'm going to read about it on internet. Well it all works now as it should if anyone have more suggestions how to improve the code feel free to post. And thanks for help guys!

[Rastagong]

Thank you for clarifying this, Rastagong, I had always assumed a new list was being created for each instance, and as I always overwrote them later, I never noticed all instances where using the same list all along. I'll be sure to read the links you suggested so this doesn't happen again.
Sorry for the trouble :oops:

Don't worry, it's nothing at all! :)
And to be fair, a lot of people consider this problem a design flaw of Python, because it's just so… counter-intuitive.
It makes default values more complicated to understand, even though the concept of default values itself is really useful and simple.
Apparently, Ruby has default values too but doesn't have this problem with mutable default values.

Is it really necessary to have list with list = [None] in it if it is empty instead of just having list = [] are there any particular downsides to it sorry if i didn't get this part correctly?

None is not mandatory!
As you say, it's possible to check if a list is empty like this: if not status
The downside of leaving the [] empty list as a default value of the __init__ function is that it makes your code slightly less readable: the empty list remains as a default value, but since you ignore it later in your code to initialise a different empty list (self.status = []), it becomes somewhat useless.
Since None already means “nothing”, when status is None, it's clear that a real status must be created by default. In other words, using None as a default value for a parameter is just a convenient way of saying “this function should create the real default parameter itself”.
That said, this is highly subjective, it's just the way I would do it!
As you said, it's perfectly fine to just use the empty list as a default value (status=[]), and then to check if it's empty or not (if not status).

Because you gave me idea to do something like this then:

Code: Select all

init python:
    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER", skills=[], status=[]):
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            self.skills = [] # a list of skills the player knows
            self.status = [] # a list of status changes, ailments or buffs the player holds

This does the exact same thing however when its empty it doesn't give a list None item.

Yup, your example works well, except in one regard: by writing self.status = [], you prevent any custom value to be provided.
In your example, I can easily create a new Player with no status like this: player1 = Player()
However, I cannot created a player with special status right away: writing player2 = Player(status=[poison]) would not work, because that status parameter isn't taken into account: an empty status will be created with self.status = [], whether I passed a real status or not.


Case #A1: an [] empty list as a default value
This is very easy to fix, though: as you said, we can just check if the value of the status parameter is empty or not.
If it's empty (because it's the default value), we create a new empty list (to prevent the status from being shared between Players), as you already do in your example.
If the status parameter is not empty, we use this value directly, because it's a real status that we want the Player to have.

Here's the code of the Player class to do this.

Code: Select all

init python:
    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER", skills=[], status=[]):
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            if not skills:
                skills = []
            if not status:
                status = []
            self.skills = skills
            self.status = status

Case #A2: None as a default value
If you wanted to do the same thing with None instead, the Player class would be written like this:

Code: Select all

init python:
    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER", skills=None, status=None):
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            if not skills:
                skills = []
            if not status:
                status = []
            self.skills = skills
            self.status = status

The code of case #A2 is very similar to that of #A1, except for skills=None, status=None.
The reason why we do not need to change if not skills and if not status is that None evaluates negatively.
This means that when you put None in a condition, it works just like False, or like an empty list: it's considered to have a falsey value.
So when status is None, if not status evaluates positively, and a new empty list is created on the line below.




Case #B: not having a status parameter at all
With all that being said, it's probably easier not to use a parameter for status at all!

It's pretty much what you already do in your code: you define a status parameter in the __init__ function (status=[]), but you don't actually use it, you just create an empty status list no matter what: self.status = [].
Therefore, to add a new effect to a player, you rely on DannX's add_status method.
First all Players start with an empty status list, and then you can add custom effects to specific Players using player.add_status().

This is probably the best design for your Player class!
But in this case, we should remove the status parameter entirely, since it becomes useless.
This would be the code of the Player class in this case:

Code: Select all

init python:
    class Player(object):
        def __init__(self,name="NoName",hp=0,en=0,picture="PLAYER"):
            self.name = name # Player name
            self.hp = hp # player hp
            self.en = en # Player energy
            self.picture = picture # image of player
            self.skills = []
            self.status = []
            
        # Put DannX's add_status method here

It's basically the code you'd written, but we've removed the status and skills parameters: there's no skills=[], status=[] in the definition of the function anymore.
We don't check if skills or status are empty, because these parameters don't exist anymore: now we assume that all Players start with no skills or status, no matter what.
So we just create an empty list of skills with self.skills =[] and an empty status list with self.status = [].



So, yeah, these would be 3 possibilities to deal with the status list in the Player class.
I think #B best suits the way you two wrote this Player class, though!

[Nero]

Wow that's some real good teaching effort and explanation right here well done I enjoyed reading this. Case B looks most attractive also even I like that you posted other options of doing this. I used to write noob codes that had 2000 lines and it got old pretty fast that is why I want to do things the right way and correct my mistake. Now with this I can replace hundreds of lines with single line of code awesome!

Since I learned this making inventory system will be even easier now that I have the idea.

Thanks again guys for your efforts this helped me A LOT.