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

Split zoom/orbit into separate examples #15135

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

richchurcher
Copy link
Contributor

Objective

As previously discussed, split camera zoom and orbiting examples to keep things less cluttered. See discussion on #15092 for context.

@richchurcher
Copy link
Contributor Author

@janhohenheim as promised!

@NthTensor NthTensor added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples A-Input Player input via keyboard, mouse, gamepad, and more S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 10, 2024
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

First time I have gotten to watch a PR get written before I review it. Tested locally, works great. We should really fix the scale on that fox asset, it's very silly.

Thanks for your contribution!

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.

Looks great :D Left some minor improvements.
I also want to note that orbit cameras are traditionally controlled by the mouse, so that may be what the user expects to find here. I'll leave the choice if you want to switch to mouse up to you. If you do so, see the comments in #15109 for how to correctly read and multiply by the motion delta.
You could still use Q and E or some other key for roll, but imo it's fine to also just leave it out. It's very rare that someone needs to adjust the roll of a camera if they're not doing some spaceship simulator or super mario galaxy stuff, and if they need to, they already know how to do it based on this code.
Again, feel free to leave it at keyboard inputs if you believe that choice to be better in this case :)

Comment on lines +145 to +147
delta_pitch *= time.delta_seconds();
delta_roll *= time.delta_seconds();
delta_yaw *= time.delta_seconds();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
delta_pitch *= time.delta_seconds();
delta_roll *= time.delta_seconds();
delta_yaw *= time.delta_seconds();
let speed = camera_settings.orbit_speed * time.delta_seconds();
delta_pitch *= speed;
delta_roll *= speed;
delta_yaw *= speed;

Bit easier to understand where the numbers come from this way :)

Comment on lines +123 to +142
if keyboard_input.pressed(KeyCode::KeyW) {
delta_pitch += camera_settings.orbit_speed;
}
if keyboard_input.pressed(KeyCode::KeyS) {
delta_pitch -= camera_settings.orbit_speed;
}

if keyboard_input.pressed(KeyCode::KeyQ) {
delta_roll -= camera_settings.orbit_speed;
}
if keyboard_input.pressed(KeyCode::KeyE) {
delta_roll += camera_settings.orbit_speed;
}

if keyboard_input.pressed(KeyCode::KeyA) {
delta_yaw -= camera_settings.orbit_speed;
}
if keyboard_input.pressed(KeyCode::KeyD) {
delta_yaw += camera_settings.orbit_speed;
}
Copy link
Member

@janhohenheim janhohenheim Sep 10, 2024

Choose a reason for hiding this comment

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

Suggested change
if keyboard_input.pressed(KeyCode::KeyW) {
delta_pitch += camera_settings.orbit_speed;
}
if keyboard_input.pressed(KeyCode::KeyS) {
delta_pitch -= camera_settings.orbit_speed;
}
if keyboard_input.pressed(KeyCode::KeyQ) {
delta_roll -= camera_settings.orbit_speed;
}
if keyboard_input.pressed(KeyCode::KeyE) {
delta_roll += camera_settings.orbit_speed;
}
if keyboard_input.pressed(KeyCode::KeyA) {
delta_yaw -= camera_settings.orbit_speed;
}
if keyboard_input.pressed(KeyCode::KeyD) {
delta_yaw += camera_settings.orbit_speed;
}
if keyboard_input.pressed(KeyCode::KeyW) {
delta_pitch += 1.0;
}
if keyboard_input.pressed(KeyCode::KeyS) {
delta_pitch -= 1.0;
}
if keyboard_input.pressed(KeyCode::KeyQ) {
delta_roll -= 1.0;
}
if keyboard_input.pressed(KeyCode::KeyE) {
delta_roll += 1.0;
}
if keyboard_input.pressed(KeyCode::KeyA) {
delta_yaw -= 1.0;
}
if keyboard_input.pressed(KeyCode::KeyD) {
delta_yaw += 1.0;
}

See comment below in the code or above (for some reason) when looking at the GitHub discussion

Name::new("Fox"),
SceneBundle {
scene: asset_server
.load(GltfAssetLabel::Scene(0).from_asset("models/animated/Fox.glb")),
Copy link
Member

Choose a reason for hiding this comment

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

TIL of this syntax 👀 Is this better than assert_server.load("models/animated/Fox.glb#Scene0")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily! Yanked straight from one of our other examples. Not sure if "better", possibly exposes to editor/LSP help? Since it's not just a string...

Copy link
Member

Choose a reason for hiding this comment

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

I like it better because it's harder to typo.

Comment on lines +161 to +162
// Adjust the translation to maintain the correct orientation toward the orbit target.
transform.translation = Vec3::ZERO - transform.forward() * camera_settings.orbit_distance;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Adjust the translation to maintain the correct orientation toward the orbit target.
transform.translation = Vec3::ZERO - transform.forward() * camera_settings.orbit_distance;
// Adjust the translation to maintain the correct orientation toward the orbit target.
let target = Vec3::ZERO;
transform.translation = target - transform.forward() * camera_settings.orbit_distance;

That should make it a bit more obvious how to customize this code

@janhohenheim janhohenheim 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 Sep 10, 2024
@richchurcher
Copy link
Contributor Author

if they're not doing some spaceship simulator

But... surely they're making an Elite clone! 😆

@janhohenheim
Copy link
Member

@NthTensor regarding the fox, the scaling is not the only thing that's suboptimal unfortunately: #8099

@richchurcher
Copy link
Contributor Author

@NthTensor regarding the fox, the scaling is not the only thing that's suboptimal unfortunately: #8099

I mean, it's labelled Trivial so if I continue my backwards quest through the oldest trivial tickets in the backlog I'll eventually get to it 😉

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 10, 2024
Merged via the queue into bevyengine:main with commit 8e7ef64 Sep 10, 2024
31 checks passed
@richchurcher
Copy link
Contributor Author

@janhohenheim next looks? I couldn't resist keeping roll in, because I played too much Elite as a youth. It's possibly a tiny bit difficult to control, but seems to work OK.

@richchurcher
Copy link
Contributor Author

Oops! Let's make that a separate PR 😆

github-merge-queue bot pushed a commit that referenced this pull request Sep 13, 2024
# Objective

Applies feedback from previous PR #15135 'cause it got caught up in the
merge train 🚂

I couldn't resist including roll, both for completeness and due to
playing too many games that implemented it as a child.

cc: @janhohenheim
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more A-Rendering Drawing game state to the screen 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.

4 participants