[Merged by Bors] - implemented #[bundle(ignore)] #6123

maccesch · 2022-09-29T00:36:02Z

Objective

Replaces #5628

Solution

Because the generated method from_components() creates an instance of Self my implementation requires any field type that is marked to be ignored to implement Default.

Changelog

Added the possibility to ignore fields in a bundle with #[bundle(ignore)]. Typically used when PhantomData needs to be added to a Bundle.

alice-i-cecile · 2022-09-29T02:42:41Z

FYI Github has a built-in "Draft mode" for WIP PRs :)

maccesch · 2022-09-30T00:33:23Z

@alice-i-cecile Ok all fixed

crates/bevy_ecs/macros/src/lib.rs

Co-authored-by: Nathan Ward <[email protected]>

Co-authored-by: Afonso Lage <[email protected]>

maccesch · 2022-09-30T19:27:47Z

@alice-i-cecile I was thinking about this, and I'm not sure about what I'm about to propose 😄

Instead of having a #[bundle(ignore)] should we just ignore PhantomData by default? Making it less redundant...

The downside would be that if you had a custom type called PhantomData you couldn't use it with bundles. But to me that feels like a very small price to pay.

NathanSWard · 2022-09-30T20:03:55Z

@alice-i-cecile I was thinking about this, and I'm not sure about what I'm about to propose 😄

Instead of having a #[bundle(ignore)] should we just ignore PhantomData by default? Making it less redundant...

The downside would be that if you had a custom type called PhantomData you couldn't use it with bundles. But to me that feels like a very small price to pay.

Don't actually think this is possible, at least at the macro level using the name PhantomData.
Since it's very easy someone could do something like:

type GhostData<T> = PhantomData<T>;

#[derive(Bundle)]
pub struct MyBundle<T> {
  ghost: GhostData<T>,
  // ..
}

And it's also impossible to do it via any sort of type matching e.g.

if TypeId::of<BundleField>() == TypeId::of<PhantomData<?>> {}

Since PhantomData is generic over T where T could be any type.

Granted I'm sure there's probably some hackery you could do with a trait, however, IMO #[bundle(ignore)] is by far the cleanest solution.
Not only that, it's possible there are other use cases to ignore a type inside a bundle. Granted PhantomData is probably like 90% of use cases.

NathanSWard · 2022-09-30T20:06:12Z

Oh, and one last thing, could we add a test for this?
Basically just have some bundle where we #[bundle(ignore)] one of the fields, and ensure that when we insert the bundle for an entity, the ignored component does not exist for that entity.

maccesch · 2022-09-30T20:14:55Z

@NathanSWard When making my suggestion I actually had these limitations in mind. I implemented a derive macro for Dioxus where we decided to make all Option<...> types optional by default (just matching by name) because even though you can do all of these shenanigans you demonstrated almost nobody will. So the gain in ergonomics was prioritized over these unlikely edge cases.

Having said that it is true that the aforementioned Option default is something that is used all the time in contrast to this PhantomData case which is probably not used that frequently.

I still think the ergonomics would be worth it. Maybe instead we could have a

#[bundle(include)]
data: PhantomData<...>,

that counteracts the default exclusion for the case that somebody really did name a component "PhantomData" (lol) and leave the #[bundle(ignore)] for those cases where somebody indeed made a type alias for PhantomData.

So in conclusion:

PhantomData is ignored by default
can be counteracted by #[bundle(include)]
you can manually force sth to be excluded by #[bundle(ignore)]

NathanSWard · 2022-09-30T22:02:56Z

Fwiw, I'm definitely not a big fan of #[bundle(include)].
And forcing people to explicitly use the words PhantomData in their bundle's member feels like more of an anti-pattern and more of a possible future headache then just requiring people to use #[bundle(ignore)].

I'm just generally against any sort of "stringly" typed imementation which is what you seem to be proposing.

NathanSWard · 2022-09-30T22:09:43Z

This also extends to other marker types, e.g. PhantomPinned. Having a simple #[bundle(ignore)] to account for any marker like type seems like the most generic/simplest solution, instead of just special casing for PhantomData where the implementation has sharp corner cases.

alice-i-cecile · 2022-10-01T00:19:59Z

This also extends to other marker types, e.g. PhantomPinned. Having a simple #[bundle(ignore)] to account for any marker like type seems like the most generic/simplest solution, instead of just special casing for PhantomData where the implementation has sharp corner cases.

Yep, this is my feeling. I also like how much more explicit the ignore solution is. Deviations from the expected behavior should be called out.

maccesch · 2022-10-01T12:13:41Z

Ok got it. Thanks for considering 😄

maccesch · 2022-10-01T12:35:39Z

@NathanSWard test added ✔️

SpecificProtagonist · 2022-10-01T23:34:18Z

I agree that we should have #[bundle(ignore)], but does it make sense to unsafe impl<T: Sync + Send + 'static> Bundle for PhantomData<T> (as a no-op) so derive(Bundle) just works, similarly how empty tuples implement Bundle?

alice-i-cecile · 2022-10-02T12:09:58Z

I agree that we should have #[bundle(ignore)], but does it make sense to unsafe impl<T: Sync + Send + 'static> Bundle for PhantomData (as a no-op) so derive(Bundle) just works, similarly how empty tuples implement Bundle?

IMO this is too implicit. It leads to weird behavior where you can e.g. insert a PhantomData<T> and it does nothing silently.

BoxyUwU · 2022-10-17T19:24:38Z

derive(WorldQuery) and derive(SystemParam) both use #[world_query(ignore)] [system_param(ignore)] instead of trying to be clever about implicitly ignoring PhantomData. Regardless of whether this is a good/bad idea, all 3 derives should be consistent and imo this is definitely not the PR to be implementing Bundle/SystemParam/WorldQuery for PhantomData (or trying to match the struct name in the proc macro).

alice-i-cecile

Great macro code, good tests, useful little feature.

bors r+

# Objective Fixes #5559 Replaces #5628 ## Solution Because the generated method from_components() creates an instance of Self my implementation requires any field type that is marked to be ignored to implement Default. --- ## Changelog Added the possibility to ignore fields in a bundle with `#[bundle(ignore)]`. Typically used when `PhantomData` needs to be added to a `Bundle`.

bors · 2022-10-24T14:14:04Z

Build failed (retrying...):

check-compiles

# Objective Fixes #5559 Replaces #5628 ## Solution Because the generated method from_components() creates an instance of Self my implementation requires any field type that is marked to be ignored to implement Default. --- ## Changelog Added the possibility to ignore fields in a bundle with `#[bundle(ignore)]`. Typically used when `PhantomData` needs to be added to a `Bundle`.

bors · 2022-10-24T14:52:57Z

Pull request successfully merged into main.

Build succeeded:

# Objective Fixes bevyengine#5559 Replaces bevyengine#5628 ## Solution Because the generated method from_components() creates an instance of Self my implementation requires any field type that is marked to be ignored to implement Default. --- ## Changelog Added the possibility to ignore fields in a bundle with `#[bundle(ignore)]`. Typically used when `PhantomData` needs to be added to a `Bundle`.

implemented #[bundle(ignore)]

8bc1f5a

maccesch changed the title ~~implemented #[bundle(ignore)]~~ WIP: implemented #[bundle(ignore)] Sep 29, 2022

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Sep 29, 2022

fixed tests

8262270

maccesch changed the title ~~WIP: implemented #[bundle(ignore)]~~ implemented #[bundle(ignore)] Sep 30, 2022

afonsolage suggested changes Sep 30, 2022

View reviewed changes

crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/macros/src/lib.rs Show resolved Hide resolved

NathanSWard suggested changes Sep 30, 2022

View reviewed changes

crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved

NathanSWard suggested changes Sep 30, 2022

View reviewed changes

crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved

maccesch and others added 6 commits September 30, 2022 11:39

Update crates/bevy_ecs/macros/src/lib.rs

a69c1bc

Co-authored-by: Nathan Ward <[email protected]>

Update crates/bevy_ecs/macros/src/lib.rs

129d3e7

Co-authored-by: Nathan Ward <[email protected]>

Update crates/bevy_ecs/macros/src/lib.rs

70bc74d

Co-authored-by: Afonso Lage <[email protected]>

Update crates/bevy_ecs/macros/src/lib.rs

27b696b

Co-authored-by: Afonso Lage <[email protected]>

fixed tests

7fabf1a

removed redundant else

decbc93

alice-i-cecile approved these changes Oct 1, 2022

View reviewed changes

added unit test for #[bundle(ignore)]

c4a89ff

maccesch requested a review from NathanSWard October 2, 2022 00:36

maccesch requested review from afonsolage and NathanSWard and removed request for NathanSWard and afonsolage October 2, 2022 00:36

maccesch requested review from afonsolage and removed request for NathanSWard October 9, 2022 19:57

BoxyUwU approved these changes Oct 17, 2022

View reviewed changes

BoxyUwU added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 17, 2022

alice-i-cecile reviewed Oct 24, 2022

View reviewed changes

bors bot changed the title ~~implemented #[bundle(ignore)]~~ [Merged by Bors] - implemented #[bundle(ignore)] Oct 24, 2022

bors bot closed this Oct 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - implemented #[bundle(ignore)] #6123

[Merged by Bors] - implemented #[bundle(ignore)] #6123

maccesch commented Sep 29, 2022

alice-i-cecile commented Sep 29, 2022

maccesch commented Sep 30, 2022

maccesch commented Sep 30, 2022

NathanSWard commented Sep 30, 2022 •

edited

Loading

NathanSWard commented Sep 30, 2022

maccesch commented Sep 30, 2022 •

edited

Loading

NathanSWard commented Sep 30, 2022

NathanSWard commented Sep 30, 2022 •

edited

Loading

alice-i-cecile commented Oct 1, 2022

maccesch commented Oct 1, 2022

maccesch commented Oct 1, 2022

SpecificProtagonist commented Oct 1, 2022

alice-i-cecile commented Oct 2, 2022

BoxyUwU commented Oct 17, 2022

alice-i-cecile left a comment

bors bot commented Oct 24, 2022

bors bot commented Oct 24, 2022

[Merged by Bors] - implemented #[bundle(ignore)] #6123

[Merged by Bors] - implemented #[bundle(ignore)] #6123

Conversation

maccesch commented Sep 29, 2022

Objective

Solution

Changelog

alice-i-cecile commented Sep 29, 2022

maccesch commented Sep 30, 2022

maccesch commented Sep 30, 2022

NathanSWard commented Sep 30, 2022 • edited Loading

NathanSWard commented Sep 30, 2022

maccesch commented Sep 30, 2022 • edited Loading

NathanSWard commented Sep 30, 2022

NathanSWard commented Sep 30, 2022 • edited Loading

alice-i-cecile commented Oct 1, 2022

maccesch commented Oct 1, 2022

maccesch commented Oct 1, 2022

SpecificProtagonist commented Oct 1, 2022

alice-i-cecile commented Oct 2, 2022

BoxyUwU commented Oct 17, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

bors bot commented Oct 24, 2022

bors bot commented Oct 24, 2022

NathanSWard commented Sep 30, 2022 •

edited

Loading

maccesch commented Sep 30, 2022 •

edited

Loading

NathanSWard commented Sep 30, 2022 •

edited

Loading