[Merged by Bors] - Move magic numbers into constants in Breakout example #4255
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alice-i-cecile
added
C-Examples
hymm
reviewed
Mar 19, 2022
Co-authored-by: Mike <mike.hsu@gmail.com>
Co-authored-by: Mike <mike.hsu@gmail.com>
hymm
reviewed
Mar 19, 2022
hymm
approved these changes
Mar 19, 2022
ghost
suggested changes
Mar 19, 2022
There was a problem hiding this comment.
LGTM. As a beginner I would've greatly appreciated the constants so this change makes sense to me.
I think the amount of documentation on the constants is fine as is for now, because otherwise the example would be drowned in comments that don't add a lot to the example itself.
If we see users asking questions about particular constants we should add documentation to those, but they are mostly self explanatory.
Co-authored-by: KDecay <KDecayMusic@protonmail.com>
alice-i-cecile
added
the
S-Ready-For-Final-Review
ghost
approved these changes
Mar 19, 2022
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Mar 19, 2022
# Objective - The Breakout example has a lot of configurable constant values for setup, but these are buried in the source code. - Magic numbers scattered in the source code are hard to follow. - Providing constants up front makes tweaking examples very approachable. ## Solution - Move magic numbers into constants ## Context Part of the changes made in #2094; split out for easier review.
bors
bot
changed the title
mockersf
reviewed
Mar 20, 2022
| // These constants are defined in `Transform` units. | ||
| // Using the default 2D camera they correspond 1:1 with screen pixels. | ||
| // The `const_vec3!` macros are needed as functions that operate on floats cannot be constant in Rust. | ||
| const PADDLE_HEIGHT: f32 = -215.0; |
There was a problem hiding this comment.
the meaning of this one is not very obvious, even less with the PADDLE_SIZE just under
There was a problem hiding this comment.
Agreed. I'll fix this in a follow-up PR.
bors bot
pushed a commit
that referenced
this pull request
Mar 23, 2022
# 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.
aevyrie
pushed a commit
to aevyrie/bevy
that referenced
this pull request
Jun 7, 2022
# Objective - The Breakout example has a lot of configurable constant values for setup, but these are buried in the source code. - Magic numbers scattered in the source code are hard to follow. - Providing constants up front makes tweaking examples very approachable. ## Solution - Move magic numbers into constants ## Context Part of the changes made in bevyengine#2094; split out for easier review.
aevyrie
pushed a commit
to aevyrie/bevy
that referenced
this pull request
Jun 7, 2022
…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
# Objective - The Breakout example has a lot of configurable constant values for setup, but these are buried in the source code. - Magic numbers scattered in the source code are hard to follow. - Providing constants up front makes tweaking examples very approachable. ## Solution - Move magic numbers into constants ## Context Part of the changes made in bevyengine#2094; split out for easier review.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Solution
Context
Part of the changes made in #2094; split out for easier review.