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

Implement SystemSet for BoxedSystemSet #8839

Closed
wants to merge 1 commit into from

Conversation

yyogo
Copy link
Contributor

@yyogo yyogo commented Jun 14, 2023

Objective

Solution

Explicitly implement SystemSet (implying IntoSystemSet) for BoxedSystemSet


Rationale

BoxedSystemSet doesn't "need" to implement SystemSet because you can call the methods on the trait object reference. However, that means it can't be IntoSystemSet, since it can't unwrap the internal type, and you can't call .into_system_set(self) on dyn SystemSet, because it isn't sized.

The trivial solution is to impl SystemSet for Box<dyn SystemSet>. But that introduces a problem because IntoSystemSetConfig is implemented separately for SystemSet and BoxedSystemSet, and for SystemSet it boxes it before passing to the constructor. So to avoid this double-boxing I just added a default into_boxed() method that turns a normal, non-trait-object system set into a BoxedSystemSet, and override it for BoxedSystemSet to just return itself.

Add `.into_boxed()` trait method to keep `IntoSystemSetConfig`
specialization
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Jun 15, 2023
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.

I think enabling this sort of use case is important, but we've had troubles with this sort of thing before causing subtle confusion and ergonomics issues. Can you explain what you mean by "non-dispatchable"?

@yyogo
Copy link
Contributor Author

yyogo commented Jun 15, 2023

Can you explain what you mean by "non-dispatchable"?

I just mean a trait method with a Self: Sized bound, so it can't be called on a trait object (that's what the reference calls them).

I've updated the description a bit, hope my reasoning is clearer. Can you point me to a similar case that caused trouble in the past?

@alice-i-cecile
Copy link
Member

#8436 was the case I was thinking of :) Thanks for the clearer description!

@yyogo
Copy link
Contributor Author

yyogo commented Jun 17, 2023

Ah, I see. I'll take a closer look and set this to draft in the meantime

@yyogo yyogo marked this pull request as draft June 18, 2023 07:12
@yyogo yyogo closed this Mar 12, 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 simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECS: SystemConfig::after(BoxedSystemSet) does not work
2 participants