Merge BuildWorldChildren and BuildChildren traits.
#14052
+82
−106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add an associated type to `BuildChildren` which reflects the prior differences between the `BuildChildren` and `BuildWorldChildren` traits. - Add `ChildBuild` trait that is the bounds for `BuildChildren::Builder`, with impls for `ChildBuilder` and `WorldChildBuilder`. - Remove `BuildWorldChildren` trait and replace it with an impl of `BuildChildren` for `EntityWorldMut`.
alice-i-cecile
added
A-ECS
|
Lovely, I was hoping we could do this. |
alice-i-cecile
approved these changes
Jun 27, 2024
alice-i-cecile
added
the
S-Needs-Review
janhohenheim
suggested changes
Jun 28, 2024
| /// implementations of [`BuildChildren`] as a bound on the [`Builder`](BuildChildren::Builder) | ||
| /// associated type. The closure passed to [`BuildChildren::with_children`] accepts an | ||
| /// implementation of `ChildBuild` so that children can be spawned via [`ChildBuild::spawn`]. | ||
| pub trait ChildBuild { |
There was a problem hiding this comment.
Worth thinking about whether we want the user to be able to implement this trait on their own. The current API implies this and makes adding a new method a breaking change.
I'm personally leaning towards sealing the trait.
What do you think @alice-i-cecile?
Note that the same conversation can be had for BuildChildren.
There was a problem hiding this comment.
IMO we don't bother sealing this but we also just don't really care about adding new methods. This isn't long for the world: complexity isn't worth it here.
janhohenheim
approved these changes
Jun 29, 2024
janhohenheim
added
S-Ready-For-Final-Review
JoJoJet
approved these changes
Jun 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
The
BuildChildrenandBuildWorldChildrentraits are mostly identical, so I decided to try and merge them. I'm not sure of the history, maybe they were added before GATs existed.Solution
BuildChildrenwhich reflects the prior differences between theBuildChildrenandBuildWorldChildrentraits.ChildBuildtrait that is the bounds forBuildChildren::Builder, with impls forChildBuilderandWorldChildBuilder.BuildWorldChildrentrait and replace it with an impl ofBuildChildrenforEntityWorldMut.Testing
I ran several of the examples that use entity hierarchies, mainly UI.
Changelog
n/a
Migration Guide
n/a