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

Updated the 2D examples to make them uniform #17237

Merged

Conversation

chamaloriz
Copy link
Contributor

@chamaloriz chamaloriz commented Jan 8, 2025

Objective

Make the examples look more uniform and more polished.
following the issue #17167

Solution

  • Added a minimal UI explaining how to interact with the examples only when needed.
  • Used the same notation for interactions ex : "Up Arrow: Move Forward \nLeft / Right Arrow: Turn"
  • Set the color to GRAY when it's not visible enough
  • Changed some colors to be easy on the eyes
  • removed the //camera comment
  • Unified the use of capital letters in the examples.
  • Simplified the mesh2d_arc offset calculations.

...

…ons at the top of the window, choosing better colors, and centering the props.
Copy link
Contributor

github-actions bot commented Jan 8, 2025

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@chamaloriz chamaloriz changed the title Updated the 2D examples to match each other by adding small explanati… Updated the examples to match each other by adding small explanati… Jan 8, 2025
@chamaloriz chamaloriz changed the title Updated the examples to match each other by adding small explanati… Updated the examples to make them uniform Jan 8, 2025
Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

I like most of the tweaks, but feel like most of the added Text is not useful.

(Feel free to wait for some consensus on that point before making any related changes)

examples/2d/cpu_draw.rs Outdated Show resolved Hide resolved
examples/2d/bounding_2d.rs Show resolved Hide resolved
examples/2d/mesh2d_vertex_color_texture.rs Outdated Show resolved Hide resolved
examples/2d/move_sprite.rs Outdated Show resolved Hide resolved
@@ -118,6 +132,7 @@ fn setup_camera(mut commands: Commands, mut images: ResMut<Assets<Image>>) {
// render before the "main pass" camera
order: -1,
target: RenderTarget::Image(image_handle.clone().into()),
clear_color: ClearColorConfig::Custom(GRAY.into()),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to keep the default clear color unless it's totally necessary to change it.

We may want to discuss a "standard light clear color" for situations like this. The default clear color was chosen for branding reasons and isn't quite just "dark gray."

Copy link
Member

Choose a reason for hiding this comment

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

We should keep the standard clear color when possible. A standard light clear color is a good idea: we should consider adding a bevy::color::palettes::bevy_branding for this in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I think GRAY is fine for now while we discuss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied the same method used in 'mesh2d_arcs' and set GRAY as the default color in both this example and there. I only changed it because it wasn't legible enough. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should keep the standard clear color when possible. A standard light clear color is a good idea: we should consider adding a bevy::color::palettes::bevy_branding for this in another PR.

I'll try to work on that when I finish this pull request :)

Copy link
Contributor Author

@chamaloriz chamaloriz Jan 23, 2025

Choose a reason for hiding this comment

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

We should keep the standard clear color when possible. A standard light clear color is a good idea: we should consider adding a bevy::color::palettes::bevy_branding for this in another PR.

I found this pull request talking about a bevy palette

examples/2d/sprite_animation.rs Outdated Show resolved Hide resolved
examples/2d/transparency_2d.rs Show resolved Hide resolved
examples/2d/rotation.rs Outdated Show resolved Hide resolved
examples/2d/sprite.rs Outdated Show resolved Hide resolved
@rparrett
Copy link
Contributor

rparrett commented Jan 8, 2025

I would suggset keeping this PR scoped to the 2d examples and opening another later.

@mockersf
Copy link
Member

mockersf commented Jan 8, 2025

I like most of the tweaks, but feel like most of the added Text is not useful.

I agree on that. Text is useful for instructions, not for explanations

@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 A-Cross-Cutting Impacts the entire engine S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jan 8, 2025
@chamaloriz chamaloriz changed the title Updated the examples to make them uniform Updated the 2D examples to make them uniform Jan 9, 2025
@chamaloriz
Copy link
Contributor Author

chamaloriz commented Jan 9, 2025

I like most of the tweaks, but feel like most of the added Text is not useful.

I agree on that. Text is useful for instructions, not for explanations

I gave my bevy beginner opinion here, would love to know what you think :)

@chamaloriz chamaloriz marked this pull request as ready for review January 21, 2025 16:27
@chamaloriz chamaloriz requested a review from rparrett January 21, 2025 16:28
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 22, 2025
examples/2d/2d_viewport_to_world.rs Outdated Show resolved Hide resolved
examples/2d/mesh2d_arcs.rs Outdated Show resolved Hide resolved
examples/2d/rotation.rs Outdated Show resolved Hide resolved
examples/2d/sprite_animation.rs Outdated Show resolved Hide resolved
examples/2d/sprite_animation.rs Outdated Show resolved Hide resolved
Co-authored-by: Rob Parrett <[email protected]>
@alice-i-cecile
Copy link
Member

Almost there :) Let me know when you've fixed @rparrett's last round of review and I think this will be good to merge.

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 23, 2025
@chamaloriz
Copy link
Contributor Author

Almost there :) Let me know when you've fixed @rparrett's last round of review and I think this will be good to merge.

I don't mind taking the time, since my plan is to apply these changes to the other examples :)

@chamaloriz chamaloriz requested a review from rparrett January 23, 2025 11:14
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 23, 2025
@chamaloriz chamaloriz requested a review from rparrett January 23, 2025 15:53
@rparrett rparrett added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 23, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 23, 2025
@alice-i-cecile
Copy link
Member

Thanks for your patience here, and thanks @rparrett for the thorough review :)

Merged via the queue into bevyengine:main with commit 94e0e1f Jan 23, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and 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