Skip to content

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