Support generics in bundle derive macro

[Veykril]

Fixes #87

This still does the duplication check, unlike tuples which don't do that. Should that stay or not?

Edit: Maybe the dupe check can be made cheaper by making it lazy and only execute once via AtomicBool and closures?

[Ralith]

We definitely don't want to be constantly rerunning the duplicate check. I think an AtomicBool scheme will have the same problems that a static array did, i.e. we can't have one per type parameter set. However, I think there's an easy solution here: add assert!(types.windows(2).all(|x| x[0] != x[1]), "duplicate types"); or similar to Archetype::new. This will catch that case for tuples and derived bundles both without significant cost.

[Veykril]

That will only catch duplicate types that are right beside each other though, as in (i32, i64, i32) won't fail.

I pushed a commit in which I experimented a bit, let me know what you think about it. I do check for duplicates in the Archetype constructor now and in debug builds it also emits the names of the component types in case it fails.

Theoretically this could also be made to output the type name of Bundles but I didn't want to make too big changes at once now.

[Ralith]

That will only catch duplicate types that are right beside each other though

The Vec<TypeInfo> passed to Archetype::new is required to be sorted, per the debug_assert. In a sorted list, equal elements are guaranteed to be adjacent.

in debug builds it also emits the names of the component types in case it fails.

Nice! I'd prefer to just print the name of the duplicated type rather than all the types, though. Additionally, we should find a way to communicate the error without mentioning archetypes, as the average user shouldn't be required to know what those are. Maybe "attempted to allocate an entity with duplicate components"?

[Veykril]

The Vec passed to Archetype::new is required to be sorted, per the debug_assert. In a sorted list, equal elements are guaranteed to be adjacent.

Right, not sure why I thought that wouldn't work 😅

Alright, it should print the type instead now together with your proposed message which I think fits quite well

[Veykril]

Alright I hope I finally addressed everything. Really sorry for making you have to review more than necessary 😅

[Veykril]

One other small thing I noticed, the proc-macro currently assumes std to be always there, should this maybe also be feature gated so that it uses alloc::vec instead of std::vec and otherwise core?

[Ralith]

Thanks, this looks great!

Really sorry for making you have to review more than necessary sweat_smile

No worries, still easier than writing it myself 😀

the proc-macro currently assumes std to be always there

Good catch; this should be fixed, but we can leave it for a separate PR.