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

new example: multiple sprites, one parent entity #13050

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

awwsmm
Copy link
Contributor

@awwsmm awwsmm commented Apr 21, 2024

Objective

Solution

  • Create an example (following @cart's advice on Reddit) showing how to create a parent entity which contains multiple sprites

@awwsmm
Copy link
Contributor Author

awwsmm commented Apr 21, 2024

Screen.Recording.2024-04-21.at.09.21.31.mov

Screen capture showing the parent entity moving around the window in response to the user pressing the arrow keys

@NthTensor NthTensor added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples labels Apr 21, 2024
Copy link
Contributor

@wilk10 wilk10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I think the example has its merit, given it's something that beginners trip up on initially.

I just left two comments to improve it, imo, especially the one about Parent.

examples/2d/multiple_sprites_one_entity.rs Outdated Show resolved Hide resolved
examples/2d/multiple_sprites_one_entity.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@wilk10 wilk10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the previous comments! One more thing that in my opinion needs to be corrected, otherwise it's good to go from my side.

examples/2d/multiple_sprites_one_entity.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@wilk10 wilk10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! LGTM

@IceSentry
Copy link
Contributor

I'm not sure the name of the example is correct. It's mostly showing how to use bevy_hierarchy in 2d. It's using mulitple entities, not just one.

IceSentry's suggestion

Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
@awwsmm
Copy link
Contributor Author

awwsmm commented Apr 23, 2024

I'm not sure the name of the example is correct. It's mostly showing how to use bevy_hierarchy in 2d. It's using mulitple entities, not just one.

What would you suggest instead? I was aiming at searchability (see the linked Reddit thread). Beginners (like me) will wonder "how can I use multiple sprites inside a single entity?" That's how I ended up on the current name.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add instructions on screen to use the arrow keys?

@awwsmm
Copy link
Contributor Author

awwsmm commented Apr 23, 2024

could you add instructions on screen to use the arrow keys?

Done!

@IceSentry
Copy link
Contributor

@awwsmm

Why would you prefer input handling inside the system itself? Just for readability?
There are other examples already which use const generics. virtual_time, for example

The main reason for me is that it breaks up the logic in muliple places instead of having everything co-located. It adds runtime logic to setup code that is otherwise not about runtime. It makes it harder/impossible to configure the buttons/values at runtime. It's also a bit unconventional compared to almost all other examples (I would have had the same criticism for the virtual_time example) and uses more advanced features that beginners might not be familiar with. It's not a huge deal, and I guess for an example it doesn't matter, but that's not something that would scale well outside of a simple example.

What would you suggest instead? I was aiming at searchability (see the linked Reddit thread). Beginners (like me) will wonder "how can I use multiple sprites inside a single entity?" That's how I ended up on the current name.

I don't really have a suggestion. To me the issue is more about bevy_hierarchy not being obvious or easily discoverable. It seems to me like it should be a github discussions with an answer so it's easily googleable. The example itself seems redundant since the hierarchy example already essentially does the same thing.

@awwsmm
Copy link
Contributor Author

awwsmm commented Apr 23, 2024

@awwsmm

...

What would you suggest instead? I was aiming at searchability (see the linked Reddit thread). Beginners (like me) will wonder "how can I use multiple sprites inside a single entity?" That's how I ended up on the current name.

I don't really have a suggestion. To me the issue is more about bevy_hierarchy not being obvious or easily discoverable. It seems to me like it should be a github discussions with an answer so it's easily googleable. The example itself seems redundant since the hierarchy example already essentially does the same thing.

Updated both of these examples (hierarchy and multiple_entities_one_sprite) to refer to one another in their doc comment at the top of each file.

@awwsmm awwsmm requested a review from mockersf May 1, 2024 00:50
@awwsmm
Copy link
Contributor Author

awwsmm commented May 29, 2024

Bump @mockersf

@mockersf
Copy link
Member

Could you rename the example (and everywhere it's mentioned) to "multiple sprites, one parent entity?

Saying "one entity" is misleading otherwise

@awwsmm awwsmm changed the title new example: multiple sprites, one entity new example: multiple sprites, one parent entity May 29, 2024
@awwsmm
Copy link
Contributor Author

awwsmm commented May 29, 2024

Could you rename the example (and everywhere it's mentioned) to "multiple sprites, one parent entity?

Saying "one entity" is misleading otherwise

Done

@mgi388
Copy link
Contributor

mgi388 commented Jun 5, 2024

@awwsmm

Why would you prefer input handling inside the system itself? Just for readability?
There are other examples already which use const generics. virtual_time, for example

The main reason for me is that it breaks up the logic in muliple places instead of having everything co-located. It adds runtime logic to setup code that is otherwise not about runtime. It makes it harder/impossible to configure the buttons/values at runtime. It's also a bit unconventional compared to almost all other examples (I would have had the same criticism for the virtual_time example) and uses more advanced features that beginners might not be familiar with. It's not a huge deal, and I guess for an example it doesn't matter, but that's not something that would scale well outside of a simple example.

What would you suggest instead? I was aiming at searchability (see the linked Reddit thread). Beginners (like me) will wonder "how can I use multiple sprites inside a single entity?" That's how I ended up on the current name.

I don't really have a suggestion. To me the issue is more about bevy_hierarchy not being obvious or easily discoverable. It seems to me like it should be a github discussions with an answer so it's easily googleable. The example itself seems redundant since the hierarchy example already essentially does the same thing.

I agree that this seems more or less the same as examples/ecs/hierarchy.rs except the hierarchy one is a more useful example in my opinion for these reasons:

  1. It shows the child entity moving independently of the parent, but the example in this PR visually just looks like one sprite
  2. The hierarchy example is commented well to explain to users what's going on

If this example is merged, looks like there are still instances of @mockersf's issue that need fixing:

Could you rename the example (and everywhere it's mentioned) to "multiple sprites, one parent entity?

E.g. https://github.com/bevyengine/bevy/pull/13050/files#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542R553

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants