benmoran56 / esper

An ECS (Entity Component System) for Python
MIT License
537 stars 67 forks source link

`World().create_entity()` does not create entity if no components are given #80

Closed gretkierewicz closed 1 year ago

gretkierewicz commented 1 year ago

Describe the bug As a factory method I assume it creates entity for sure but that do not happen is edge case of missing components.

To Reproduce

import esper

world = esper.World()
entity = world.create_entity()
assert world.entity_exists(entity)  # raises AssertionError
import esper

world = esper.World()
entity = world.create_entity()
assert not world.entity_exists(entity)  # not existing entity

entity_2 = world.create_entity()
assert entity_2 == 2  # Incrementing counter even though 1st one seems not to exist

Expected behavior Imo proper behaviour is to be able to create "empty" entity and provide it with components later. Kind of how you do it with builder pattern. Plus I've found that add_component() has logic for creating missing entity and that is most probably result of that bug. Following Single Responsibility Principle, that should not be part of add_component() method imo. See example below:

import esper

world = esper.World()
made_up_entity = 123
world.add_component(made_up_entity, None)  # Imo should raise KeyError because of missing entity
world.entity_exists(made_up_entity)  # Works just fine and should not

Development environment:

Fix proposal To fix that it is simple enough to pull https://github.com/benmoran56/esper/blob/master/esper/__init__.py#L227

if entity not in self._entities:
    self._entities[entity] = {}

out of for loop in create_entity() method. Consider removing entity creation from add_component() method.

Tell me what do you think about it, I can implement change and unit tests later on

benmoran56 commented 1 year ago

That looks like a valid bug, and should give a tiny performance boost by moving that entity check out of the for loop. Thanks for finding this. A pull request would certainly be welcome.