Make all builtin bundles !Component

[DJMcNab]

Fixes #392

Component requires Sync, so this PR makes all built-in bundles !Sync

This prevents the following from compiling, among others

fn query_system(mut commands: Commands, mut query: Query<[Error~]&LightComponents~>){
    commands.spawn(([Error~]LightComponents::default()~,));
}

This is incredibly hacky, but it works to fix the motivating issue with no other side effects. Additionally, this issue has appeared several times in #help on the Discord
Ideally, we'd just impl !Sync directly, but that currently gives an error that negative impls aren't stable, which doesn't appear to be likely to be fixed any time soon

[cart]

This is really clever and does accomplish the goal, but I'm hesitant to merge this change for a number of reasons:

1. increases implementation complexity
2. puts the onus on devs composing bundle structs to add the !Sync field (and associated ignore attributes).
3. overloads Sync semantics for non-Sync purposes
4. prevents bundle structs from being Sync if the need ever arises
5. further diverges from hecs upstream, which increases maintenance burden

I think I would prefer to wait for negative impls to stabilize, even if it takes more time. The "people try to use bundles as components" problem is something that is largely solved by better educational material.

[DJMcNab]

really clever

Thanks, I suppose.
A numbered list like that is just inviting me to reply to each in turn :P

1. I don't think it makes reading the macros lib.rs any worse, and if you're reading that, you should already know why;
2. Time to make #[bundle] a full procedural macro then 🤔;
3. whistles.
4. The hack is easy to remove if it came to that. I don't think we care about backwards-compatability that much at this point.
5. The divergence is only in the macros crate, which has not been significantly changed upstream since December last year.

I would have much rather made it not 'static, which I think at least sort of makes sense, but I don't think there is a non-'static type without a lifetime parameter, unfortunately. Maybe I just need to find a legitimate reason for each of the bundles to be !Sync.

I hadn't seen rust-lang/rust#68318 - trying to impl !Send, which I thought might have worked only sent me to rust-lang/rust#13231. It still doesn't look like there's going to be any progress on that feature for a while yet.

And regarding educational material, to me this feels more like something we should prevent entirely, because it's never going to be correct. I'm already getting bored of telling people how wrong it is :| . I think it would be possible to add runtime validation using type_name, but that to me seems even more ugly than this. Although I am tempted to add that anyway, so we can prevent &'static and fn types being added, both of which are other things which I've had people try to do in the Discord.

I can close this and pivot to that if you want @cart

[cart]

I can close this and pivot to that if you want @cart

I think runtime validation using type_name would introduce more overhead than its worth. Component insertion is a "hot path". I think I'd take the solution in this pr over that.

I don't think it makes reading the macros lib.rs any worse, and if you're reading that, you should already know

I agree that its a minor complexity increase. Not a huge deal.

Time to make #[bundle] a full procedural macro then thinking;

I think you're suggesting bundle!(MyBundle { ... }), which would allow us to add struct fields. I'm generally against macros that modify the fields / layout of a struct. It makes everything much harder to reason about and can break ides.

The hack is easy to remove if it came to that. I don't think we care about backwards-compatability that much at this point.

Yeah backward compat isn't a big deal at this point.

The divergence is only in the macros crate, which has not been significantly changed upstream since December last year.

Yup also not a huge deal.

Your points are valid, but I'm still not convinced that this is the right call. There are enough concessions being made here that it doesn't seem worth it to me. This is a hard call and merging is "valid" for the same reason not merging is "valid". My bias here is to opt for simplicity and non-hackey solutions.

Another option that we'll have when specialization lands (which is probably closer to stabilization than negative trait bounds):

pub trait Component: Send + Sync + 'static {
  default fn is_valid() -> bool {
    true
  }
}
// in derive macro
impl Component for MyBundle {
  fn is_valid() -> bool {
    false
  }
} 

// when adding components to a World:
assert!(component.is_valid())

[DJMcNab]

In theory, since 1.46 there's no reason why we can't make the type_name stuff a const fn, making the runtime cost the same. Alternatively, we could make it only debug_assertions, for example.
If we wanted, we could also have a 'yes I am sure' for all of the additions in the bevy_crates, to limit the cost to only the end-user crates.

The specialisation solution is very neat - I hadn't considered that.

Edit: Or it could be const if we had const type_name :(

[cart]

Yeah non-const type_name is a real shame. We also want to use that for StableTypeId / dynamic plugin loading, but sadly its a no-go for now.

I'd consider a debug-only type_name validation, but we already have complaints about debug perf and adding "expensive" checks to hot paths would only make that situation worse. And using type name here throws away type safety in favor of naming conventions, which also feels really dirty to me.

Sorry I'm not trying to be difficult here. This is just a hard problem. Its not satisfying, but waiting for specialization and improving documentation in the short term seems like a good non-hackey solution.

[cart]

I'm closing this for now, but we may come back to this implementation in the future if alternative impls dont work out.

[DJMcNab]

Yeah, that's fine
I was going to close it myself yesterday, but wasn't quite certain that it had been completely abandoned.

[ashneverdawn]

@cart I've been experimenting quite a bit with bundles, especially when it comes to nesting as well and creating more complex entities and I'm starting to think we may be on a wrong approach. Every issue I tried to solve seemed to require more advanced/unstable rust features to work nicely. Eventually I settled on something super simple & very powerful that doesn't need any creative rust features. Just a regular struct (not bundle) that derives/implements default and has a pub fn spawn(self, mut commands: Commands) -> Commands and pub fn spawn_as_child(self, &mut child_builder: ChildBuilder). This pattern ended up working super well and it only uses super basic rust.
For example:

//ScrollViewWidget is the complex entity I want to spawn
#[derive(Default)]
pub struct ScrollViewWidget {
  pub root : ScrollViewRoot,
  pub scrollbar_child : ScrollbarChild,
  pub view_area_child : ViewAreaChild
}
impl ScrollViewWidget {
  pub fn spawn(self, mut commands: Commands) -> Commands {
    //assemble the entity and its children here
  }
  pub fn spawn_as_child(self, &mut child_builder: ChildBuilder) {
    //assemble the entity and its children here
  }
}


//Each struct containing the components for the entity and child entities: (shortened for brevity)
//These are basically the same as bundles.

pub struct ScrollViewRoot {
  pub com1 : Component1
  //etc...
}
pub struct ScrollViewRoot {
  pub com2 : Component2
  //etc...
}
pub struct ViewAreaChild {
  pub com3 : Component3
  //etc...
}

Then to use it I just do:

commands = ScrollViewRoot::default().spawn(commands);

And because it derives/implements default, it is fully data driven and anything can be modified. For example:

let scrollview = ScrollViewRoot::default();
scrollview.view_area_child.com3.value = "my value";
commands = scrollview.spawn(commands);

or for those who prefer the more verbose way:

commands = ScrollViewRoot{
  view_area_child: {
    com3: Component3{
      value: "my value",
      ..default()
    }
    ..default()
  }
  ..default()
}.spawn(commands);

(Sorry if the code doesn't compile. I'm writing this from memory)