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.
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
Create a DynamicScene from a world and an entity root #6013
Create a DynamicScene from a world and an entity root #6013
Changes from 3 commits
9e43c0d
1938d01
daaddd6
f0554e9
793f693
70347fb
8307f14
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Bikeshed: i think the "at root" naming is confusing. I'd probably have called this
from_world_entity_recursive
.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.
totally open to changing the name, even more if we change it to take several entities as input
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.
IMO
from_entity_recursive
orfrom_entities_and_children
or something. I think thefrom_world
bit is unhelpful in context and can just be dropped.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.
I actually cast my vote for a simple
from_root
. Imo it's implicit that we're talking about worlds and entities here. If we want to be a little more specific, I thinkfrom_entity
is fine too— although, it could be slightly confusing as to whether this recurses into children or not.Next best would probably be
from_entity_recursive
.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.
Personally, I think this method would have limited practical utility. Who wants to create a scene from just one entity (+ children)? Sure, sometimes you might have such a use case, but it seems needlessly limited. What if you want a second entity? Or a few more?
Why not change this to
impl IntoIterator<Item=Entity>
, allowing users to provide any number of root entities?Would add a little extra complexity to the body of the function. Perhaps there should be some sort of deduplication logic to prevent redundant/duplicate entities in the scene. One suggestion might be to use an intermediary
HashSet
instead of aVec
. Or maybe we could just not do any deduplication (just accumulate all the entities into the scene without regard for duplicates), and document it as a caveat / leave it to the users to make sure they don't have duplicate entities in their list of roots.It would make this function much more practical and useful.
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 you prefer to be able to append to an existing scene? Or just creating from multiple entities
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.
Well now that you have said it ... i guess both? 😆
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.
I imagine single-entity scenes could be super useful for prefab-like serialization and usage.
But I agree it could also be useful to handle multiple at a time. Perhaps a method for a single root and another for multiple?
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.
Documentation should include information about the registry filter.
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.
I think this should be a separate method so users that want to extract everything can don't need to add the extra
world.resource::<AppTypeRegistry>()
call.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.
I'm reluctant to add these derives; they make things more implicit and are likely to break once we need to add more fields.
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.
IMO the Deref / DerefMut impls on Scene should be their own PR so they get appropriate review attention.
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.
Then let's remove them later if we need to add more fields? For now they make working with scenes a lot nicer.
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.
Disagreed. Why introduce extra implicitness and potential for future breaking changes, just to save on typing a
.0
. I wouldn't call this "a lot nicer".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.
Well then we're back to arguing against the
Deref
derive that we promotes in Bevy: https://bevyengine.org/news/bevy-0-7/#deref-derefmut-derivesI'm not sure there are plans to add more fields to
Scene
at the moment?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.
Hmm, I'm fine to leave it. We should write a manual Deref / DerefMut impl if and when we add more fields then, and call this out in the docs though.
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.
Personally, I don't think the
Deref
/DerefMut
derives are needed. It's probably better for users to explicitly be aware that what they're working with is basically just a plain ol'World
. And yeah, it makes future additions toScenes
less problematic (e.g. versioning, etc.).