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

Consolidate Events and Commands #1251

Closed
alice-i-cecile opened this issue Jan 16, 2021 · 4 comments
Closed

Consolidate Events and Commands #1251

alice-i-cecile opened this issue Jan 16, 2021 · 4 comments
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

Events and Commands share very similar conceptual models, but use completely disjoint code.

What solution would you like?

Make Commands a built-in event type that stores data, and then use Event<Commands> to access commands in the system. Consume the commands one at a time as they're processed at the end of each stage.

Reuse the existing tech from Commands to allow for parallel write-to-end behavior of events when supplied via Event<MyEvent>, allowing us to run systems that add events of the same type at the same time.

What alternative(s) have you considered?
Leaving these distinct allows us to diverge behavior further if needed. I think the basic design space of Commands has been sufficiently explored that it's clear that they should work the same way however. If there's other tweaks or APIs needed for commands down the road, it's likely that other types of events will want it too.

We could also have Commands use events under the hood and leave the slightly shorter public API the same, but I think that's needlessly obfuscatory. The fact that we can handle Commands using a built-in events system is intuitive and cool, and makes them easier to extend for advanced users.

Additional context

Making events a system parameter in #1244 makes this even more natural, since event readers are now special-cased, rather than simply being resources.

@Moxinilian Moxinilian added C-Code-Quality A section of code that is hard to understand or change C-Enhancement A new feature A-ECS Entities, components, systems, and events and removed C-Enhancement A new feature labels Jan 16, 2021
@mockersf
Copy link
Member

Commands are currently made of two "parts":

  • the list of commands, that could work like the event queue
  • the entity reserver and the current entity

Unlike events, commands handle a state with the current entity.

I can see a few ways to split the two and make it work as events, but I don't think the user facing api would be nicer that the current one

@cart
Copy link
Member

cart commented Jan 16, 2021

Theres also a potential non-event layer to commands that I think we want: command optimization. Ex: we can cut down on archetype changes by merging "insert bundle and insert component" commands together.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Jan 16, 2021

That's very useful context. Handling the list of commands as an event queue seems like a fairly clear win, but the larger change would have more costs.

If we wanted to push forward with fully merging events and commands I think we would need to:

  1. Make Commands stateless by removing the current entity + entity reserver. As best as I can tell from looking at Command and Commands, these are only used by .with (and .with_bundle). For .with, we could cut down on the need for it dramatically by automatically flattening bundles ala Merge bundles together #792 (which should be done either way).

Then, you could have commands.spawn return the Entity that is scheduled to spawn (which would normally just be eaten by a ;), and then pipe that to commands.insert instead, removing .with entirely. Normally you wouldn't need to do this, and would instead just use a larger spawn expression. I would only see this happening for multi-stage assembly of very complex entities.

  1. Apply command optimization to an event iterator of commands. I think you could just process a clone of the commands and then regenerate a cleaned up iterator. A bit strange, but I don't think there's any fundamental blocker there. Honestly, you'll want to be able to use similar patterns on arbitrary events anyways, so I think it makes sense to build in / discover better first-class support for this.

Advantages

  1. You can use parallelism within your systems for generating and consuming commands much more easily, as each individual command will contain all of the relevant information. This is particularly noticeable with improved .spawn_batch ergonomics, but may also be handy when processing commands.
  2. Simplified API for Commands, making it easier for users to understand and extend and for us to process efficiently.
  3. Reduced code duplication means that improvements to commands and events feed into each other more smoothly.
  4. Makes adding components to entities less magical, because there's no internal state. This should help make creating complex multistage entities a bit less error-prone, especially with .spawn_batch.
  5. Condenses methods of component insertion.

Disadvantages

  1. Makes using commands as a system parameter more verbose (but also more clear).
  2. Makes sequentially adding components to existing entities slightly less ergonomic, since you need to store the entity created. This should only be seen for complex entities that are created one at a time with .spawn. Fixing Merge bundles together #792 solves this for the large majority of entities, and .with doesn't play nicely with .spawn_batch as is.
  3. Couples events and commands more tightly, which may cause unforeseen issues.
  4. Other future commands that operate on entities will also have to do so explicitly by Entity key.

Edit: it also looks like this functionality is used by .with_children. A similar solution would work, but I know that cart was poking around that code so that might change.

@alice-i-cecile
Copy link
Member Author

I'm closing this for now: the idea will crop up again naturally if there's ever a clean way to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

No branches or pull requests

4 participants