snecs icon indicating copy to clipboard operation
snecs copied to clipboard

Implicit world default parameter.

Open HexDecimal opened this issue 4 years ago • 3 comments

A lot of functions take a world parameter but if you ever forget to add it then an implicit default world will be used instead. I'm paranoid that I'll forget to add this parameter somewhere which will create a bug I can't track down easily.

Have you considered other ways of handling this? I see that other libraries often put all of their main functionality in methods of their world object instead of as separate objects, or removing the implicit world as a default parameter could be done instead, or another class could be added which encapsulates both the world and the ECS functions which use it.

HexDecimal avatar Sep 28 '21 22:09 HexDecimal

Almost all uses of the library will only ever need one World - perhaps creating a short-lived copy on-demand for serialization/deserialization. For normal usage, you never need to use an explicit World; that's why there's a default one.

snecs is meticulously micro-optimized; as an ECS library, most of the calls to snecs's API will live in the hot path, so pretty much every line in the library was decided by profiling and benchmarking compared to alternative implementations. This is why I'm using a procedural approach to its API design; using "rich" classes with methods incurs attribute lookup overhead every time you need to interact with the library, unless you do something like:

from snecs import World

world = World()
new_entity = world.new_entity
add_component = world.add_component
...

and reexport every method of every World so you don't have to look up an attribute every time you want to call one of them. Since this would be equivalent to just redefining every funciton in snecs.ecs, it's both tidier and faster to pass in the World as an argument to those functions.

If it were the case that normal ECS usage requires multiple Worlds, I would be happy to remove the default parameter; however, I feel like for almost all users, they'll never need to specify an explicit world.

the-moonwitch avatar Sep 30 '21 11:09 the-moonwitch

I can understand how performance could be a priority, but I don't like how the API suffers for it. If given the options I'd use the bound World methods even though I knew they were slower. I'm also not suggesting to remove the current functions if we know they're really faster.

Attribute lookup overhead isn't the best example since it's a well known optimization to manually 'remove the dots' in performance critical loops which might be an even faster lookup than the current methods because then the functions are in the local namespace rather than global. You might also be implying that I'm using from snecs import * for all of my function calls which I don't plan on doing.

world = snecs.BoundWorld()  # Suggesting an encapsulating class or subclass since adding the methods directly to World will likely cause import issues.
world.new_entity(...)  # Normal/slow use.

new_entity = world.new_entity  # Well known optimization.
# world.new_entity could return `functools.partial(snecs.new_entity, world=self)` instead of a regular bound method.
for ...:
   new_entity(...)  # Hot path, lookup in local namespace (assuming this is in a function.)

Will there be a standard way to replace the implicit World with a new one that I've created? There is snecs.world.default_world but this gets bound to the functions so I can't just assign a new world to that name to have it take effect. I'd have need to do something more complex such as swapping snecs.world.default_world.__dict__ with one belonging to another World.

To disable the implicit parameter I'd also have to do del snecs.world.default_world.__dict__ so that using that world will raise an AttributeError instead of invisibly using it.

HexDecimal avatar Sep 30 '21 17:09 HexDecimal

You raise valid points; I'll need to think about this further.

Acknowledging that I've read your comment, but I won't be able to mess with snecs until next week. Hopefully I'll come to a decision on what to do about this by then.

the-moonwitch avatar Sep 30 '21 23:09 the-moonwitch