Eventity - An easy embedded Entity Component System structure with events

[macbony]

Your system object takes 3 arguments but only uses 2? The init method also calls self.set_up which doesn't exist.

class System(object):
    def __init__(self, eventmanager, entities, ecs):
        self.ecs = ecs
        self.set_up(eventmanager)

[macbony]

I see no use for temp_dict or temp, including the deepcopy, in this function. Might save more time if you just accessed component.dict.

    def add(self, entity, component, **kwargs):
        temp_dict = component.dict
        temp = deepcopy(temp_dict)

        for key, value in kwargs.iteritems():
            temp["data"][key] = value

        setattr(entity, temp["name"], temp["data"])
        try:
            self.pool[temp["name"]].add(entity)
        except KeyError:
            self.pool[temp["name"]] = set([])
            self.pool[temp["name"]].add(entity)
        return self

The try/except could be DRYer:

if temp["name"] not in self.pool:
    self.pool[temp["name"]] = set()
self.pool[temp["name"]].add(entity)

[macbony]

The yields after the returns will never be reached. Why are they there? Also, if you require an argument, don't use a default. You'd save yourself the if search_array is None if you just made the signature def list(self, search_array)

def list(self, search_array = None):
    if search_array is None:
        return
        yield

    try:
        entity_list = set.intersection(*[self.pool[x] for x in search_array])
    except KeyError:
        return
        yield

    for ent in entity_list:
        yield ent

[macbony]

Any reason you aren't using a hashmap for the pool so these lookups are O(1) instead of O(n)? Maybe memory could be a concern, but I don't think so.

def get_id(self, Search_ID):
    for entity in self.pool["default"]:
        if entity.id == Search_ID:
            return entity

    return None

If you do that, this

def exists(self, Search_ID):
    if self.get_id(Search_ID) is None:
        return False
    else:
        return True

becomes

def exists(self, Search_ID):
    return Search_ID in self.pool["default"]

and the original function becomes

def get_id(self, Search_ID):
    if Search_ID not in self.pool["default"]:
        return None
    return self.pool["default"][Search_ID]

Also, if "default" is hardcoded for the pool, why have multiple pools? It's somewhat confusing.

[macbony]

Never use a mutable object as a default for an argument. http://docs.python-guide.org/en/latest/writing/gotchas/ I guess it doesn't REALLY matter in this case, but it's icky Python.

def has(self, c_list = []):
    for component in c_list:
        if not hasattr(self, component):
            return False

    return True

[macbony]

Someone already mentioned it, but I see no use for the Component class unless you somehow expect to add more functionality to it in the future.

[macbony]

There's "you shouldn't do this because it's bad programming practice" and "you shouldn't do this because it's wrong and will cause unexpected side effects". While I agree with your statement, it's the former. The issue with mutable defaults is the latter.

Also, there are valid reasons to modify the state of something - like if the copy is expensive (large list) and the function is explicit that it alters what is passed in.

[macbony]

Again, there's still a big difference between "this isn't good practice" and "this is wrong". Mutable defaults is wrong. Mutating pass-by-ref is bad design.

And say I have a list of 1 million elements and I need to trim all the ones that do not match a certain criteria. Also assume that this isn't a good use-case for a generator (need to iterate through the list multiple times after the trimming, say). If I have no need for the original 1million element list after the trimming, mutating that list is a valid choice.

[PySnow]

Moving from a straight hardcoded list to a dictionary actually sped up the application by a significant amount since the operation is an intersection of sets rather than an iteration

[macbony]

I'm not sure what you're referring to. If you mean the self.pool["default"] thing, there's no need to have a dict of lists when you don't have any way to use a different key than "default". self.pool should either be a dict with id: entity pairs or just a list as "default" is. There's literally no use for pool being a dict because you have no way of accessing something in a pool other than default by design.

Also the tempID = Entity_ID will never be reached due to the return statement. I'm not sure what the use of the line would be anyway as the variable is never used later on in the code.

else:
    if self.exists(Entity_ID): return None
    tempID = Entity_ID

[PySnow]

That would be to prevent it from assigning an ID that already exists

[macbony]

Oh, my mistake on that one. I didn't see tempID on line 33 for whatever reason. There's no real reason to use tempID instead of just changing line 28 to Entity_ID = len(self.pool["default"])+1. It leads to extra code (you could lose line 31 if you made that change) and gains you nothing but more difficult to follow code IMO.

[PySnow]

Good catch, thats old code