-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Initial draft of the entities-from-manifests example #25
Conversation
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.
Very cool stuff! I left a few suggestions for these examples. I think they could also be tightened up a bit in general, but that can wait for a second pass.
examples/entities_from_manifests.rs
Outdated
@@ -13,4 +13,340 @@ | |||
//! The item-as-partial-bundle pattern is more flexible, and is suitable for cases where you have a lot of duplicated data between items that you don't want to bloat your manifest with. |
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'd start with something here that talks about what this example showcases in general. Something like "This example shows various methods for spawning entities based on manifest entries"
examples/entities_from_manifests.rs
Outdated
/// | ||
/// This is the simplest approach to spawning entities from a manifest, | ||
/// but is not very flexible as your needs change. | ||
mod item_as_bundle { |
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 would split these three into three examples, since they're basically separate already.
examples/entities_from_manifests.rs
Outdated
pub struct RawDialogBox { | ||
// If you were using a localization solution like fluent, | ||
// you might store a key here instead of the actual text | ||
// and use that as the name as well. |
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 would omit the fluent note here, since I think it might distract/confuse new readers, especially if they're not familiar with fluent
examples/entities_from_manifests.rs
Outdated
|
||
// TextBundle doesn't implement Clone :( | ||
// Tracked in <https://github.com/bevyengine/bevy/issues/12985> | ||
impl Clone for DialogBox { |
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 would just use a different bundle here that already implements Clone
, to keep the example clean
examples/entities_from_manifests.rs
Outdated
/// This module demonstrates the item-as-partial-bundle pattern. | ||
/// | ||
/// This pattern is useful when you have a lot of duplicated data between items in the manifest. | ||
mod items_as_partial_bundle { |
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 for the partial bundle, I would use something closer to the Item
from the simple example. That way, the manifest item can be a component in a larger bundle. I don't feel too strongly though.
Still needs assets, but otherwise should be reviewable.