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

Add Self: Sized bounds to Bundle trait methods #14879

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jnhyatt
Copy link
Contributor

@jnhyatt jnhyatt commented Aug 22, 2024

Objective

Fixes #3227.

I'll admit upfront this might be some silly shenanigans.

This came about based on some Rust-fu I was trying to do to enable generic dynamic bundle insertion/removal. Consider the following snippet:

#[derive(Component)]
pub struct InsertThisLater(Box<dyn Bundle>);

This doesn't compile, and from my perspective, understandably: a Bundle that doesn't know its type can't get into the world. A workaround I came up with is this:

pub trait DynBundle {
    fn insert(self: Box<Self>, mut e: EntityWorldMut);
}

pub struct InsertThisLater(Box<DynBundle>);

A component can impl this trait (or derive it) and suddenly you have a working system! Only problem is Rust thinks it's not possible since some of Bundle's methods (erroneously) assume Self: ?Sized.

Solution

Add where Self: Sized to the rest of Bundle's (and DynamicBundle's) trait methods


Breaking change?

Technically, I think?

I'm not exactly sure what kind of code you'd have to write to run into this breaking change, but this change expands what you can do with bundles, not the other way around. I'll try and find some weird bit of code that breaks with this, but I don't think it needs a migration guide section, and honestly, I don't think it would be bad to shove it into 0.14.2, however I understand that still breaks the letter of the law and is a solid argument against doing so.

@jnhyatt
Copy link
Contributor Author

jnhyatt commented Aug 22, 2024

@alice-i-cecile 🙂

@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 22, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Neat! Can you please add a doc test to Bundle to verify that Box<dyn Bundle> works, and can be inserted and removed correctly?

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Aug 22, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Aug 22, 2024

I've linked the issue I made several years ago for this problem in your PR description. I'm going to put this in 0.15 instead of a 0.14 point release just in case we've missed some terrible edge case on the breaking changes.

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Aug 22, 2024
@jnhyatt
Copy link
Contributor Author

jnhyatt commented Aug 22, 2024

To be clear, this isn't a drop-in dynamic bundle solution, however, reading through that issue, it looks like it wasn't far from being pushed through. I can turn this into a full-on dynamic bundle PR, I think my preferred solution would be to add a couple object-safe helper methods on Bundle and then implement them in the derive.

@alice-i-cecile
Copy link
Member

I'd prefer to turn this into a full-on dynamic bundle PR IMO :) This is very unintrusive, and there would be a lot of value in a solution there. Probably spin up an ECS folder example?

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Aug 24, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.15 milestone Sep 30, 2024
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-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better tools for working with dynamic collections of components
2 participants