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

Screen shake example #15567

Merged
merged 17 commits into from
Oct 7, 2024
Merged

Screen shake example #15567

merged 17 commits into from
Oct 7, 2024

Conversation

moOsama76
Copy link
Contributor

@moOsama76 moOsama76 commented Oct 1, 2024

Objective

Closes #15564

Solution

Adds a screen shake example.

@moOsama76
Copy link
Contributor Author

Closes #15564

@alice-i-cecile
Copy link
Member

@m-edlund could I get your review here?

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 1, 2024
@moOsama76
Copy link
Contributor Author

A ci test fails probably because of the way I use rng, how can I fix this?

@alice-i-cecile
Copy link
Member

Swap to a seeded RNG, picking an arbitrary number for the seed rather than thread_rng. See https://rust-random.github.io/book/guide-seeding.html

Cargo.toml Outdated Show resolved Hide resolved
moOsama76 and others added 2 commits October 1, 2024 21:51
Co-authored-by: Alice Cecile <[email protected]>
examples/camera/screen_shake.rs Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Oct 1, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Copy link
Contributor

@atornity atornity left a comment

Choose a reason for hiding this comment

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

Reading the issue #15564, it says we should be using Camera::sub_camera_view to do this rather than transform directly. I suggested some changes that switches it to that. Feel free to disregard this if you think we should be using Transform instead.

examples/camera/screen_shake.rs Outdated Show resolved Hide resolved
fn screen_shake(
time: Res<Time>,
mut screen_shake: ResMut<ScreenShake>,
mut query: Query<&mut Transform, With<Camera>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mut query: Query<&mut Transform, With<Camera>>,
mut query: Query<&mut Camera>,

Comment on lines 142 to 145
for mut transform in query.iter_mut() {
transform.translation.x = rng.gen_range(-shake_amount..shake_amount);
transform.translation.y = rng.gen_range(-shake_amount..shake_amount);
}
Copy link
Contributor

@atornity atornity Oct 1, 2024

Choose a reason for hiding this comment

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

Suggested change
for mut transform in query.iter_mut() {
transform.translation.x = rng.gen_range(-shake_amount..shake_amount);
transform.translation.y = rng.gen_range(-shake_amount..shake_amount);
}
for mut camera in query.iter_mut() {
let sub_view = camera.sub_camera_view.as_mut().unwrap();
sub_view.offset = Vec2::new(
rng.gen_range(-shake_amount..shake_amount),
rng.gen_range(-shake_amount..shake_amount),
);
}

Comment on lines 95 to 101
fn new(intensity: f32, duration: f32) -> Self {
ScreenShake {
intensity,
duration,
timer: Timer::from_seconds(0.0, TimerMode::Once), // Start timer at 0
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If intensity and duration is set on every call of start_shake, setting the values in new serves no purpose and can be replaced by deriving Default for ScreenShake. As a bonus the Resource can then also be created with init_resource instead of insert_resource.

#[derive(Resource)]
struct ScreenShake {
intensity: f32,
duration: f32,
Copy link
Contributor

Choose a reason for hiding this comment

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

Duration is already encoded in the timer, and can thus be omitted.

Copy link
Contributor

github-actions bot commented Oct 1, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Comment on lines 126 to 128
if screen_shake.timer.finished() {
return; // No shake if the timer has finished
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be something to reset the transformation / the offset back to the default, once the screen shake timer has finished. Else the screen might end up stuck slightly off-center when the timer ends, depending on how intense the screen was shook.

Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution! I think having a screen shake example would be a great addition :)
This particular solution falls into a few traps discussed in this talk: Math for Game Programmers: Juicing Your Cameras With Math.
I would suggest you give it a watch and then overhaul the implementation. Feel free to ping me afterwards for a review.
Sorry I'm not enumerating my particular issues, but I don't think there is much value in doing that when the talk I linked does a way better job of explaining these things than I could hope to do :)

@janhohenheim janhohenheim 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 Oct 1, 2024
@moOsama76
Copy link
Contributor Author

@janhohenheim thank you I'll watch it

Copy link
Contributor

@m-edlund m-edlund left a comment

Choose a reason for hiding this comment

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

Looks good to me

@moOsama76
Copy link
Contributor Author

@m-edlund could I get your review here?

He approved the current version

Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

I think this would be good if the user could increase or decrease those variables with key bindings, but not a blocker for this PR, LGTM

@pablo-lua pablo-lua 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 6, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 7, 2024
@alice-i-cecile
Copy link
Member

@moOsama76 you'll need to adjust this example to match the latest changes to how cameras work :) Let us know if you need a hand.

@moOsama76
Copy link
Contributor Author

@moOsama76 you'll need to adjust this example to match the latest changes to how cameras work :) Let us know if you need a hand.

Sure

@moOsama76
Copy link
Contributor Author

moOsama76 commented Oct 7, 2024

@alice-i-cecile I need help
sub_camera_view position not changing after new camera syntax

Copy link
Contributor

@atornity atornity left a comment

Choose a reason for hiding this comment

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

You changed the size/full_size of SubCameraView (it was defualt previously, witch is 1.0:1.0 I think) and since offset is relative to those values, you also have to change MAX_OFFSET.

@alice-i-cecile
Copy link
Member

@Jondolf I think there's a bug introduced in #15641 that needs resolution. Adding this to the milestone so we don't accidentally ship it.

@alice-i-cecile alice-i-cecile added S-Needs-Help The author needs help finishing this PR. and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Oct 7, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 7, 2024

// screen_shake parameters, maximum addition by frame not actual maximum overall values
const MAX_ANGLE: f32 = 0.5;
const MAX_OFFSET: f32 = 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const MAX_OFFSET: f32 = 0.5;
const MAX_OFFSET: f32 = 500.0;

@Jondolf
Copy link
Contributor

Jondolf commented Oct 7, 2024

@alice-i-cecile Is there an issue? It works for me at the latest commit, assuming this is what it's supposed to look like:

2024-10-08.00-06-33.mp4

@alice-i-cecile
Copy link
Member

Looks like it's been resolved :)

@alice-i-cecile alice-i-cecile 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-Help The author needs help finishing this PR. labels Oct 7, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.15 milestone Oct 7, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 7, 2024
Merged via the queue into bevyengine:main with commit 91bed8c Oct 7, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen 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
Status: Done
Development

Successfully merging this pull request may close these issues.

Add a screen shake example
7 participants