Creating a complex hierarchy for "Player Effects"

Question

I'm coding a plugin for a game that has a Player class which has a boolean attribute frozen, which decides if the player is frozen or not. There are more "effects" like frozen, but I'm using it as an example, since solving one should solve them all.

The problem is, this frozen attribute only supports True/False, and if a player attacks an other player, freezing him for 5 seconds, and then someone else freezes the player permanently, the permanent freeze will go off after the other person's 5 second freeze ends (due to delay(5, setattr, (player, 'freeze', False)). This is why I need to implement a system that manages all the freezes applied to a player, and then unfreezes the player only when all of the freezes are gone.

Here's my original take on it, which I think is messy and I don't like the fact that I had to add the _effects dictionary to the Player class, when it's only used by the effect objects. I also have to rely on external _name attribute to identify the effect instances, and thus have to pass in an additional name parameter for the PlayerEffect decorator.

from ticks import delay, cancel_delay # For delayed function calls
import game.core # For the game's `Player` class
class PlayerEffect(object):
 def __init__(self, name, on_f=None, off_f=None, effect_cls=None):
 if effect_cls is None:
 class effect_cls(_PlayerEffect):
 pass
 effect_cls.name = name
 effect_cls.on_f = on_f
 effect_cls.off_f = off_f
 self._name = name
 self._on_f = on_f
 self._off_f = off_f
 self._effect_cls = effect_cls
 def on(self, on_f):
 return type(self)(self._name, on_f, self._off_f, self._effect_cls)
 __call__ = on
 def off(self, off_f):
 return type(self)(self._name, self._on_f, off_f, self._effect_cls)
 def __get__(self, instance, owner):
 return self._effect_cls(instance)
class _PlayerEffect(object):
 name = None
 on_f = None
 off_f = None
 def __init__(self, player):
 self._player = player
 def __call__(self, duration=-1):
 self._player._effects[self.name].append(self)
 if duration > 0:
 self._delay = delay(duration, self._disable)
 self.on_f(self._player)
 def _disable(self):
 self._player._effects[self.name].remove(self)
 if not self._player._effects[self.name]:
 self.off_f(self._player)
 def cancel(self):
 if self._delay is not None:
 cancel_delay(self._delay)
 self._delay = None
 self._disable()
class Player(game.core.Player):
 def __init__(self, index):
 super().__init__(index)
 self._effects = collections.defaultdict(list)
 @PlayerEffect('freeze')
 def freeze(self):
 self.frozen = True
 @freeze.off
 def freeze(self):
 self.frozen = False

---

The reason I need to have the _name is so that I can do stuff like this:

class Player(game.core.Player):
 def __init__(self, index):
 super().__init__(index)
 self._effects = collections.defaultdict(list)
 @PlayerEffect('fly')
 def fly(self):
 self.move_state = MoveStates.FLY
 @fly.off
 def fly(self):
 if self._effects['hover']: # _name used here
 self.move_state = MoveStates.HOVER
 else:
 self.move_state = MoveStates.NORMAL
 @PlayerEffect('hover')
 def hover(self):
 self.move_state = MoveStates.HOVER
 @hover.off
 def hover(self):
 if self._effects['fly']: # And here
 self.move_state = MoveStates.FLY
 else:
 self.move_state = MoveStates.NORMAL

• Dan: On a side note, if don't you like having to add the effects dictionary to the player (one one hand, it makes - they are the player's effects), you could take a look at an entity component system.
• Mahi: I ended up with something similar as my original code, but managed to remove the name (well, I made it kind of optional). The whole code is rather long, but you can read it from my GitHub project's page: github.com/MarkusMeskanen/EasyPlayer/blob/master/easyplayer.py
• Simon Forsberg: @MarkusMeskanen You are allowed to answer your own question, but note that when you do, it should be written in the same way any other answer on this site is written: As a Code Review. Presenting the "final solution" is not acceptable, explaining your reasoning (how your solution works and how it improves upon the original) would be an acceptable answer. Also see what you may and may not do after receiving answers .
• Mahi: @SimonAndreForsberg Ah, alright :) thanks for clearing that up! My solution was anyways just one that worked for me, but it might not be the best one. So I'll keep the question open for a while, before rewriting my answer in a proper manner :)

Answer 1

I'd like to suggest a completely different implementation strategy for the, er, effects of the effects. You have a thing with mutable state (Player) and operations that change that state (self.move_state = ...) based on timers. You've had to add a special system to make the timers coordinate with each other, and if you ever make a mistake in the "off" methods you will have a broken effect that sticks around.

Instead, make the state of the player a function of the effects. Here's a simple implementation to illustrate (note: may be Python 2-ish because I'm not familiar with 3; also untested):

def apply_effects(effects, attribute, base_value):
 value = base_value
 for effect in effects:
 value = effect.modify(attribute, value)
 return value
class Player(game.core.Player):
 def get_frozen(self):
 return apply_effects(self._effects, 'frozen', False)
 def get_move_state(self):
 return apply_effects(self._effects, 'move_state', MoveStates.NORMAL)
class Effect(object):
 """Base class for effects."""
 def modify(self, attribute, value):
 return value
class FrozenEffect(Effect):
 def modify(self, attribute, value):
 if attribute == 'frozen':
 return True
 else:
 super(FrozenEffect, self).modify(attribute, value)
class HoverEffect(Effect):
 def modify(self, attribute, value):
 if attribute == 'move_state' && value != MoveStates.FLY:
 return MoveStates.HOVER
 else:
 super(HoverEffect, self).modify(attribute, value)
class FlyEffect(Effect):
 def modify(self, attribute, value):
 if attribute == 'move_state':
 return MoveStates.FLY
 else:
 super(FlyEffect, self).modify(attribute, value)

(Note: If all effects are this simple, then they could be just functions instead of classes. Making them classes allows adding more operations, for example "get name and icon" to show the player what effects apply.)

Then you would still manage the lifetime of the effects much like you are now, but the effects do not need to know each others' names. The only special rule here is that the FLY move state takes priority over the HOVER move state, which is presumably a basic rule about that enumerated type. (An alternative implementation would be to define a sort order for effects, sorting the list of effects so that the fly effect is always applied after the hover effect, or divide effects into groups that are applied in a specific order.)

This architecture also allows you to easily implement additive or multiplicative effects that stack with each other (e.g. speed/damage boosts).

If you need to actually have something happen when an effect ends, then save the computed values of the attributes, and when an effect ends compute the new attributes and do the relevant thing if they're different. This means that the happening is completely dependent on the state of the player and not what effects just happened. (For example, suppose two hover effects wear off at exactly the same moment: you don't want to start a "drowned because fell in water" event twice, only once. This strategy can't make that mistake.)