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

Flexible ECS System Params #798

Merged
merged 12 commits into from
Nov 8, 2020
Merged

Flexible ECS System Params #798

merged 12 commits into from
Nov 8, 2020

Conversation

cart
Copy link
Member

@cart cart commented Nov 6, 2020

This removes the old "into system" code, which suffered from a variety of problems. Namely:

  • system params must follow an arbitrary set of order rules
  • hard to understand
  • slow to compile (bevy repo compiles in 1 minutes 13 seconds on my machine)

It replaces it with a new, much simpler approach that has the following benefits:

  • system params can be defined in any order
  • fast to compile (bevy repo compiles in 48 seconds on my machine)
  • much easier to understand

With the old implementation, this code wouldn't compile because it violated the arbitrary "order rules". Now it compiles just fine!

fn system(a_query: Query<&A>, commands: Commands, x: Res<X>, b_query: Query<&B>) {
}

So far these are pretty uncontroversial changes. Now on to the more controversial part: I also removed for-each systems.

For-each systems were problematic for the following reasons:

  1. They increased compile times (I save ~5 seconds in dev builds by removing them)
  2. They require a complicated macro, which doesn't fit into the approach used in this pr. We can use the old macro for for-each systems and the new approach for everything else, but that increases maintenance cost / retains an ugly hard to understand macro.
  3. Bevy should generally have "one way to do things". Foreach systems are a slightly more ergonomic way to define a subset of system types. This forces people to make a "design decision" we they shouldn't need to. It also makes examples and tutorials inconsistent according to people's preferences for one or the other.
  4. For-each systems aren't capable of some things, like iterating removed components, controlling iteration, multiple queries at the same time. This means they need to be converted to "query systems" when those features are needed.
  5. They have a number of "gotchas" for newcomers that constantly come up in our support forums + confuse newbies:
  • users expect &mut T queries to work in foreach systems (ex: fn system(a: &mut A) {}). These can't work because we require Mut<T> tracking pointers. The equivalent Query<&mut A> works because we can return the tracking pointer.
  • The "run on some criteria" bug thats common enough that we cover it in the Bevy Book: https://bevyengine.org/learn/book/getting-started/resources/

Changes Since Draft

  • SystemParam can now be derived. We now use this for DrawContext instead of a manual impl. You can use any types that impl SystemParam:

    #[derive(SystemParam)]
    struct MyParams<'a> {
      a: Res<'a, A>, 
      b: ResMut<'a, B>,
      commands: &'a mut Commands,
      #[system_param(ignore)]
      hello: bool // this will use Default
    }
    
    fn system(my_params: MyParams) {
      println!("{}", system.a);
    }
  • implemented SystemParam for tuples: (SystemParam, SystemParam, SystemParam). Params can now be infinitely nested

  • Implement Or for system params

  • removed ResourceQueries in favor of SystemParams. also removed UnsafeClone trait and associated impls (score!)

  • discovered closure systems work again!

  • Removed locks from Commands. We now use commands: &mut Commands instead of mut commands: Commands. This means Commands can no longer be used in parallel by default. Fortunately ...

  • Implemented SystemParam for Arc<Mutex<Commands>>.

  • Simplified ThreadLocalSystem impl

This new approach makes everything so much easier and clearer. I love it ❤️

@cart cart added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Nov 6, 2020
@cart cart marked this pull request as draft November 6, 2020 04:48
@@ -0,0 +1,121 @@
pub use super::Query;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll probably clean this up / tailor it more to thread local systems before merging

@smokku
Copy link
Member

smokku commented Nov 6, 2020

I agree with the decision of removing foreach systems. While good idea in principle, these become too limiting very quickly.
Even on the beginner's tutorial level it is confusing that you learn one way of writing systems, to be told right away "scratch that - there is another way".

Especially when the limitation of fixed ordering is being removed, which was a bit confusing (even for non-beginners), this should make "normal" systems as easy to grasp as foreach systems.

@inodentry
Copy link
Contributor

I agree with removing for-each systems. They are unnecessary, they don't simplify things at all (people need to learn 2 ways of doing things) and they are inferior, as in, almost every foreach system i have written i have then had to rewrite as a query system due to changing requirements. They have wasted more of my time than they have saved.

Comment on lines 529 to 532
// #[cfg(test)]
// mod tests {
// use super::ParallelExecutor;
// use crate::{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented them out while developing these changes and didn't uncomment them yet. I'll do that before "un-drafting" the pr.

@razaamir
Copy link

razaamir commented Nov 6, 2020 via email

@zicklag
Copy link
Member

zicklag commented Nov 6, 2020

I agree with removing foreach systems. I agree with all the cons about them. They just get in the way of the clean and far more often correct way to do things with query systems.

Also, love the order independence of the arguments!! When I first saw that note in the docs and tutorials about the order, I was thinking "that is going to be a gotcha for so many users, but at least they documented it well".

@razaamir
Copy link

razaamir commented Nov 6, 2020 via email

@tigregalis
Copy link
Contributor

tigregalis commented Nov 7, 2020

Agree with all of the comments so far. I think parameter order independence and having one way of doing things are much bigger wins for newcomers than having to wrap these parameters in a Query<>, and write a for loop, are a loss, particularly as:

  • not writing my own for-loop for each system is a bit too "magical"
    • it's easier to see what's actually happening with queries i.e. I find all of the entities that has this set of components, and I iterate over these entities, as opposed to (in for-each systems) the ECS will run this function for each entity that has these components
    • for-loops are common enough among programming languages that users should be comfortable with them
    • queries (and then having to do something with those queries) are a familiar enough concept to anyone who has queried a database or data API
  • having the parameters require a particular order definitely made things feel too magical
  • using Query<T> means it's a smaller leap to more advanced queries using Changed, Or, QuerySets, etc.
  • for-each systems are almost always mixed in with query systems so there's a bit of context switching within the same codebase or even file, and there's no particularly visible distinction between them either at the function signature or at the point the system gets added to the app
  • for-each systems are often rewritten into query systems eventually, and only after you give up trying to make the thing you're trying to do fit into a for-each system

@cart
Copy link
Member Author

cart commented Nov 7, 2020

So far the sentiment to remove for-each systems has been a unanimous "yes please". Thats enough validation for me 😄

I'll start polishing this up.

@razaamir
Copy link

razaamir commented Nov 7, 2020 via email

@cart
Copy link
Member Author

cart commented Nov 7, 2020

So I "fixed" the regression in that it was never really a regression 😆

Turns out my iteration benchmark looked like this:

for a in &mut query.iter_mut() {
}

Which i guess was an artifact of adapting the benchmark to the last round of ecs changes.

It still works even though we removed the iteration wrapper. However it tanks iteration performance, probably because the indirection prevents rustc from applying optimizations?

So yeah the problem wasn't a problem and now we know that using &mut query.iter_mut() instead of query.iter_mut() is bad for perf.

@Plecra
Copy link
Contributor

Plecra commented Nov 7, 2020

It'd be good to identify exactly which optimisation is being missed, in case there's anything that can be done about it. Composing iterators with by_ref is a pretty common pattern and it'd be nice to keep it quick

@cart
Copy link
Member Author

cart commented Nov 7, 2020

Yeah it would be good to know. But its also worth calling out that "tanking" in this case is 4x a very small number. Adding almost any operation to this benchmark is going to register in a big way (for any ecs framework already operating at this speed).

@cart cart marked this pull request as ready for review November 8, 2020 02:47
@cart
Copy link
Member Author

cart commented Nov 8, 2020

Alrighty I think this is probably good to go. I made a number of additional improvements (which I added to the PR description above). I'm very happy with how this turned out!

@memoryruins
Copy link
Contributor

This change definitely makes bevy take less time to build, especially on my laptop!

llvm cranelift
before 3m 36s 2m 46s
after 2m 40s 2m 05s

dev [unoptimized + debuginfo] target(s)

specs / environment
OS: Arch Linux x86_64
Host: ThinkPad T440
Kernel: 5.4.61-1-lts
CPU: Intel i5-4300U (4) @ 2.900GHz
Memory: 7655MiB
# ~/.cargo/config.toml
[target.x86_64-unknown-linux-gnu]
linker = "/usr/bin/clang"
rustflags = ["-Clink-arg=-fuse-ld=lld"]
active toolchain
----------------
nightly-x86_64-unknown-linux-gnu (default)
rustc 1.49.0-nightly (ffa2e7ae8 2020-10-24)

fields: Fields::Named(fields),
..
}) => &fields.named,
_ => panic!("expected a struct with named fields"),
Copy link
Contributor

@vladbat00 vladbat00 Nov 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to share how errors are designed in Legion derives:
https://github.com/amethyst/legion/blob/8fe4588d934abad8faffb54beebf415b4880773d/codegen/src/lib.rs#L173

I find it a pretty nice way to organize and make them more informative, especially in the case when you can point at a specific invalid argument and get such a compile-time error:
https://github.com/amethyst/legion/blob/8fe4588d934abad8faffb54beebf415b4880773d/tests/failures/value_argument.stderr

Probably you've already considered working on something like this in future. I don't want to suggest spending time on this right now, delaying this particular feature, but I hope this can serve as some inspiration for future improvements.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think thats one of the major selling points of their "proc macro" systems. I think derives like this don't benefit as much from that type of error handling because the logic is much more straightforward (ex: if these fields implement a trait, use it, otherwise fail). We'll already get decently informative errors for those cases. But yeah im definitely open to making improvements where we can.

@cart cart merged commit ebcdc9f into bevyengine:master Nov 8, 2020
@inodentry
Copy link
Contributor

After the new changes, since now I have to use Arc<Mutex<Commands>> to have parallel access to Commands from many systems, what if I want to use parking_lot::Mutex instead of std::sync::Mutex? Can this be supported?

@cart
Copy link
Member Author

cart commented Nov 9, 2020

The impl uses parking_lot::Mutex so you should be covered.

@inodentry
Copy link
Contributor

Nice! I didn't realize parking_lot was already the default. 👍

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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants