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

[Merged by Bors] - Improve many sprites example #2785

Closed

Conversation

willolisp
Copy link
Contributor

@willolisp willolisp commented Sep 7, 2021

Objective

My attempt at fixing #2075 .

This is my very first contribution to this repo. Also, I'm very new to both Rust and bevy, so any feedback is deeply appreciated.

Solution

  • Changed move_camera_system so it only targets the camera entity. My approach here differs from the one used in the cheatbook (in which a marker component is used to track the camera), so please, let me know which of them is more idiomatic.
  • move_camera_system does not require both Position and Transform anymore (I used rotate for rotating the Transform in place, but couldn't find an equivalent translate method).
  • Changed tick_system so it only targets the timer entity.
  • Sprites are now spawned via a single spawn_batch instead of multiple spawns.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 7, 2021
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples and removed S-Needs-Triage This issue needs to be labelled labels Sep 8, 2021
@alice-i-cecile alice-i-cecile added the S-Duplicate This issue or PR already exists label Sep 8, 2021
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Sep 8, 2021

I think we should merge this over #2078; be sure to credit @neosam in the appropriate places too however.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 8, 2021
@Sheepyhead
Copy link
Contributor

Sheepyhead commented Sep 8, 2021

I disagree with the addition of the _system postfix, but other than that it looks good

@alice-i-cecile alice-i-cecile removed the S-Duplicate This issue or PR already exists label Sep 8, 2021
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

While I'm not opposed to anything here, I would prefer not using unwrap() in example code, but it's still good without.

@cart
Copy link
Member

cart commented Sep 10, 2021

bors r+

bors bot pushed a commit that referenced this pull request Sep 10, 2021
# Objective

My attempt at fixing #2075 .

This is my very first contribution to this repo. Also, I'm very new to both Rust and bevy, so any feedback is *deeply* appreciated.

## Solution
- Changed `move_camera_system` so it only targets the camera entity. My approach here differs from the one used in the [cheatbook](https://bevy-cheatbook.github.io/cookbook/cursor2world.html?highlight=maincamera#2d-games) (in which a marker component is used to track the camera), so please, let me know which of them is more idiomatic.
- `move_camera_system` does not require both `Position` and `Transform` anymore (I used `rotate` for rotating the `Transform` in place, but couldn't find an equivalent `translate` method).
- Changed `tick_system` so it only targets the timer entity.
- Sprites are now spawned via a single `spawn_batch` instead of multiple `spawn`s.
@bors
Copy link
Contributor

bors bot commented Sep 10, 2021

Build failed:

@alice-i-cecile
Copy link
Member

bors retry

bors bot pushed a commit that referenced this pull request Sep 10, 2021
# Objective

My attempt at fixing #2075 .

This is my very first contribution to this repo. Also, I'm very new to both Rust and bevy, so any feedback is *deeply* appreciated.

## Solution
- Changed `move_camera_system` so it only targets the camera entity. My approach here differs from the one used in the [cheatbook](https://bevy-cheatbook.github.io/cookbook/cursor2world.html?highlight=maincamera#2d-games) (in which a marker component is used to track the camera), so please, let me know which of them is more idiomatic.
- `move_camera_system` does not require both `Position` and `Transform` anymore (I used `rotate` for rotating the `Transform` in place, but couldn't find an equivalent `translate` method).
- Changed `tick_system` so it only targets the timer entity.
- Sprites are now spawned via a single `spawn_batch` instead of multiple `spawn`s.
@bors bors bot changed the title Improve many sprites example [Merged by Bors] - Improve many sprites example Sep 10, 2021
@bors bors bot closed this Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants