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

Initial draft of the entities-from-manifests example #25

Merged
merged 8 commits into from
Apr 17, 2024

Conversation

alice-i-cecile
Copy link
Contributor

Still needs assets, but otherwise should be reviewable.

@alice-i-cecile alice-i-cecile added the documentation Improvements or additions to documentation label Apr 15, 2024
Copy link
Contributor

@sixfold-origami sixfold-origami left a 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 Show resolved Hide resolved
@@ -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.
Copy link
Contributor

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"

///
/// This is the simplest approach to spawning entities from a manifest,
/// but is not very flexible as your needs change.
mod item_as_bundle {
Copy link
Contributor

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.

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.
Copy link
Contributor

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


// TextBundle doesn't implement Clone :(
// Tracked in <https://github.com/bevyengine/bevy/issues/12985>
impl Clone for DialogBox {
Copy link
Contributor

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

/// 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 {
Copy link
Contributor

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.

@sixfold-origami sixfold-origami marked this pull request as ready for review April 17, 2024 00:18
@sixfold-origami sixfold-origami merged commit bfdd0f8 into main Apr 17, 2024
4 checks passed
@sixfold-origami sixfold-origami deleted the entities-from-manifests branch April 17, 2024 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants