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

Implemented #[bundle(ignore)] and #[bundle(nest)] #5579

Merged
merged 0 commits into from
Aug 9, 2022

Conversation

maccesch
Copy link
Contributor

@maccesch maccesch commented Aug 5, 2022

Objective

Fixes #5559

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.


Migration Guide

In #[derive(Bundle)] structs:

#[bundle] => #[bundle(nest)]

@alice-i-cecile 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 Aug 5, 2022
@alice-i-cecile
Copy link
Member

my implementation requires any field type that is marked to be ignored to implement Default.

I'm fine with this limitation as this feature is overwhelmingly going to be used for PhantomData.

@maxwellodri
Copy link
Contributor

maxwellodri commented Aug 5, 2022

From a user perspective, I like this change. I tested the changes with a field with [bundle(ignore)] that doesn't implement Default and the error message is clear that you need to impl Default FWIW.

@Nilirad Nilirad added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 5, 2022
@Nilirad
Copy link
Contributor

Nilirad commented Aug 5, 2022

Breaking change since #[bundle] becomes #[bundle(nest)]

/// a: A,
/// z: Z,
/// #[bundle(ignore)]
/// other: Other,
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should use a PhantomData type + a bundle generic here, as it much more clearly motivates this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the new PR I added the PhantomData but left this because I figured it demonstrates the Default trait requirement. Let me know if you want me to remove it.

@@ -20,6 +20,16 @@ pub struct Node {
}

/// An enum that describes possible types of value in flexbox layout options
///
/// There are numeric traits implemented for this enum so you can multiply, divide, add, and subtract
Copy link
Member

Choose a reason for hiding this comment

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

These changes are unrelated; please remove them :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sorry! that must have slipped in somehow!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to a bug in GitHub I had to create another PR for this: #5628

@maccesch maccesch merged commit 0ffb544 into bevyengine:main Aug 9, 2022
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-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to ignore field of bundle
4 participants