Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicit world default parameter. #1

Open
HexDecimal opened this issue Sep 28, 2021 · 3 comments · May be fixed by #5
Open

Implicit world default parameter. #1

HexDecimal opened this issue Sep 28, 2021 · 3 comments · May be fixed by #5

Comments

@HexDecimal
Copy link
Contributor

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.

@slavfox
Copy link
Owner

slavfox commented Sep 30, 2021

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.

@HexDecimal
Copy link
Contributor Author

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.

@slavfox
Copy link
Owner

slavfox commented Sep 30, 2021

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.

@HexDecimal HexDecimal linked a pull request Oct 12, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants