Skip to content

Split Bundle and StaticBundle #19761

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

SkiFire13
Copy link
Contributor

Objective

Solution

  • Create a new StaticBundle type, mirroring Bundle for now
  • Switch all APIs that need to statically know the components in a Bundle (i.e. from its type) to use StaticBundle
  • Add &self parameters to Bundle to ensure all APIs that use it have a concrete instance to work with in the future.

Additional notes

For now this doesn't do anything functionally, as Bundle still requires the components to be statically fixed. However this opens the doors for changes to the Bundle trait that allow dynamic components without impacting APIs that actually rely on static components.

Copy link
Contributor

@ElliottjPierce ElliottjPierce left a comment

Choose a reason for hiding this comment

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

Looks good to me. The parallel contents of code for StaticBundle vs Bundle is kinda annoying, but I know that's changing in the next prs, so I'm not worried about it.

@ElliottjPierce ElliottjPierce added A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 21, 2025
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change X-Contentious There are nontrivial implications that should be thought through labels Jun 22, 2025
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.

Some small notes on docs that I think we should clean up quick. I would really appreciate a paragraph or two of module docs laying out these traits and how they interconnect, but that's fine to leave to follow-up if you prefer.

More importantly, I'm not sure why we should keep a split between DynamicBundle and Bundle around after these changes. It looks like Bundle should now be object-safe, allowing us to merge them together.

I haven't done a deep dive on this area of the code in a while though, so please feel free to explain why we can't / shouldn't do that :)

@SkiFire13
Copy link
Contributor Author

More importantly, I'm not sure why we should keep a split between DynamicBundle and Bundle around after these changes. It looks like Bundle should now be object-safe, allowing us to merge them together.

Bundle is not object safe yet (it still has a bunch of generic functions without where Self: Sized) but this will change in a followup PR.

However even then it will be a bit of a hassle to remove DynamicBundle. In the unsplitted PR I limited myself to renaming it to ComponentsFromBundle to avoid confusion with dynamic bundles after trying to remove it. The issue is that the insert_by_ids method internally uses it with a struct wrapping some OwningPtrs, which is not Send + Sync + 'static, while Bundle requires them (but not DynamicBundle/ComponentsFromList!). Removing those requirements is possible but leads to ton of boilerplate elsewhere (e.g. all commands require them).

@cart cart moved this to Respond (With Priority) in @cart's attention Jun 23, 2025
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-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
Status: Respond (With Priority)
Development

Successfully merging this pull request may close these issues.

3 participants