-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 register_components API for ergonomics #1889
Conversation
Added to both `World` and `AppBuilder`. Server as a convenience when intending to e.g. add many sparse set storage components in one go. Signed-off-by: Torstein Grindvik <torstein.grindvik@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registering components is a nuisance, but seems unavoidable here. For now, I think this is a nice ergonomics improvement with no downside.
crates/bevy_ecs/src/world/mod.rs
Outdated
where | ||
I: IntoIterator<Item = ComponentDescriptor>, | ||
{ | ||
let mut ids = vec![]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try to pre-allocate this vec based in the iterator size_hint? Or maybe replace the function body with descriptors.map(|descriptor| self.register_component(descriptor)).collect()
? That shouls work because of the FromIterator<Result<T, E>> for Result<Vec<T>, E>
impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree with the use of collect()
as it will allow the compiler to optimize this a little better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the changes from the second suggestion.
Btw. if one were to use size_hint()
, would it have been appropriate to unwrap the upper bound and use that?
Seems the upper bound can be None
is some cases ("infinite" iterators, or user defined iters) but those seem nonsensical for this API (just curious).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it have been appropriate to unwrap the upper bound and use that?
Probably not, I could want to use my own iterator that doesn't implement size_hint
well and that shouldn't crash
Obligatory pushback on apis that add a new way to do the same thing: do we need this? I agree that the World api with unwrapping is a bit boilerplatey, but most users won't be using World directly. When comparing the two options for AppBuilder (which the majority of people will use), I don't see a significant win: // individual
app
.register_component(ComponentDescriptor::new::<A>(StorageType::SparseSet))
.register_component(ComponentDescriptor::new::<B>(StorageType::SparseSet))
.register_component(ComponentDescriptor::new::<C>(StorageType::SparseSet))
// batch
app
.register_components(vec![
ComponentDescriptor::new::<A>(StorageType::SparseSet),
ComponentDescriptor::new::<B>(StorageType::SparseSet),
ComponentDescriptor::new::<C>(StorageType::SparseSet),
]) In general I want to push for "one way" to do things. With two apis, users will need to ask themselves "should I use a batch api here". Theres also the (very minor) issue of the additional allocations the batch api adds for the vec. |
This is a more relevant comparison, and I see your point here. Ultimately I would love to have this defined at the type-level somehow (see #1843), which would help reduce the boilerplate substantially and encourage much more local code organization. |
I agree, this comparison shows that the added cognitive load of more APIs is likely not worth. |
A suggestion for #1794.
If accepted, fixes #1794.
The linked issue original code was as follows:
With the new API, this is now:
Or perhaps simpler as done in the added docs+test: