Skip to content

[Merged by Bors] - Break out Breakout components into a more sensible organization #4261

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

[Merged by Bors] - Break out Breakout components into a more sensible organization #4261

wants to merge 7 commits into from

Conversation

@alice-i-cecile
Copy link
Member

Objective

  • The components in the Breakout game are defined in a strange fashion.
    • Components should decouple behavior wherever possible.
    • Systems should be as general as possible, to make extending behavior easier.
    • Marker components are idiomatic and useful, but marker components and query filters were not used.
  • The existing design makes it challenging for beginners to extend the example into a high-quality game.

Solution

  • Refactor component definitions in the Breakout example to reflect principles above.

Context

A small portion of the changes made in #2094. Interacts with changes in #4255; merge conflicts will have to be resolved.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 19, 2022
@alice-i-cecile alice-i-cecile added C-Examples An addition or correction to our examples C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Mar 19, 2022
@alice-i-cecile
Copy link
Member Author

@bevyengine/docs-team can I get some feedback on this? There's more clean up I'd like to do on this example, but I want to keep the PRs small and sequential.

ghost
ghost suggested changes Mar 20, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I definitely like these changes from an ECS point of view. Having speed or velocity be a field instead of a component or constant is really unECS'y and shouldn't be encouraged.

Could you also move this comment to be above the function or somewhere else, because it looks like there is something missing below it in the current state. GitHub wouldn't let me comment on code that you didn't change :(

@alice-i-cecile alice-i-cecile requested a review from a user March 23, 2022 17:21
IceSentry
IceSentry approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@IceSentry IceSentry left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like it as it is, but I wonder if there could be a Speed component that can be changed with a slider just to show that these values don't need to be const. This change can wait until we have a slider ui widget though.

dataphract
dataphract approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@dataphract dataphract left a comment

Choose a reason for hiding this comment

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

Good changes, it reads much more idiomatically now.

@rparrett
Copy link
Contributor

This is extremely opiniony, but I think that

struct Velocity {
    value: Vec2
}

overall would improve things further.

This looks good to me though.

doup
doup reviewed Mar 23, 2022
View reviewed changes
@alice-i-cecile
Copy link
Member Author

Awesome, thanks y'all. I'll clean this up; I like your suggestions a lot.

@alice-i-cecile
Copy link
Member Author

This is extremely opiniony, but I think that

struct Velocity {
value: Vec2
}
overall would improve things further.

So, I made this change in the last commit. Overall, I think it's the more idiomatic than the alternative, but it does litter .0 (or worse, .value) everywhere. IMO we should fix that up later with a Deref / DerefMut impl, but that's a problem for another day.

@alice-i-cecile
Copy link
Member Author

bors r+

@doup doup mentioned this pull request Mar 23, 2022
Closed
6 tasks
@alice-i-cecile
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Mar 23, 2022
@alice-i-cecile
Break out Breakout components into a more sensible organization (#4261)
d51b54a 
# Objective

- The components in the Breakout game are defined in a strange fashion.
   - Components should decouple behavior wherever possible.
   - Systems should be as general as possible, to make extending behavior easier.
   - Marker components are idiomatic and useful, but marker components and query filters were not used.
- The existing design makes it challenging for beginners to extend the example into a high-quality game.

## Solution

- Refactor component definitions in the Breakout example to reflect principles above.

## Context

A small portion of the changes made in #2094. Interacts with changes in #4255; merge conflicts will have to be resolved.
@bors
Copy link
Contributor

bors bot commented Mar 23, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Break out Breakout components into a more sensible organization [Merged by Bors] - Break out Breakout components into a more sensible organization Mar 23, 2022
@bors bors bot closed this Mar 23, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
@alice-i-cecile @aevyrie
Break out Breakout components into a more sensible organization (bevy…
799c2b0 
…engine#4261)

# Objective

- The components in the Breakout game are defined in a strange fashion.
   - Components should decouple behavior wherever possible.
   - Systems should be as general as possible, to make extending behavior easier.
   - Marker components are idiomatic and useful, but marker components and query filters were not used.
- The existing design makes it challenging for beginners to extend the example into a high-quality game.

## Solution

- Refactor component definitions in the Breakout example to reflect principles above.

## Context

A small portion of the changes made in bevyengine#2094. Interacts with changes in bevyengine#4255; merge conflicts will have to be resolved.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@alice-i-cecile @ItsDoot
Break out Breakout components into a more sensible organization (bevy…
8f3cb82 
…engine#4261)

# Objective

- The components in the Breakout game are defined in a strange fashion.
   - Components should decouple behavior wherever possible.
   - Systems should be as general as possible, to make extending behavior easier.
   - Marker components are idiomatic and useful, but marker components and query filters were not used.
- The existing design makes it challenging for beginners to extend the example into a high-quality game.

## Solution

- Refactor component definitions in the Breakout example to reflect principles above.

## Context

A small portion of the changes made in bevyengine#2094. Interacts with changes in bevyengine#4255; merge conflicts will have to be resolved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

5 more reviewers

@doup doup doup approved these changes

@rparrett rparrett rparrett approved these changes

@IceSentry IceSentry IceSentry approved these changes

@dataphract dataphract dataphract approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alice-i-cecile @rparrett @doup @IceSentry @dataphract