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.

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

Improve first person camera in example #15109

Merged
merged 18 commits into from
Sep 10, 2024
Merged

Improve first person camera in example #15109

merged 18 commits into from
Sep 10, 2024

Conversation

janhohenheim
Copy link
Member

@janhohenheim janhohenheim commented Sep 9, 2024 •
edited
Loading

Objective

  • I've seen quite a few people on discord copy-paste the camera code of the first-person example and then run into problems with the pitch.
  • Additionally, the code is framerate-dependent. it's not, see comment in PR

Solution

  • Make the code good enough to be copy-pasteable
    • Use dt to make the code framerate-independent Add comment explaining why we don't use dt
    • Clamp the pitch
    • Move the camera sensitivity into a component for better configurability

Testing

Didn't run the example again, but the code is straight from another project I have open, so I'm not worried.

@janhohenheim janhohenheim added 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 Sep 9, 2024
@NiseVoid
Copy link
Contributor

NiseVoid commented Sep 9, 2024

Multiplying mouse motion by delta time actually has the opposite effect. The mouse motion already matches the amount you moved it during the frame, so multipling it by delta time makes your mouse movement FPS-dependant, with a higher FPS meaning slower movement. However, if the input was a gamepad stick it would need to be multiplied by delta, which might be worth including in the example for that reason. Ideally we would of course upstream LWIM and have it handle those differences instead.

I do like the limit for the angle, tho the comment could be better. Not only is rotating too far not particularly useful, it also makes the other axis function like it's reversed.

@alice-i-cecile alice-i-cecile 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 Sep 9, 2024
alice-i-cecile
alice-i-cecile reviewed Sep 9, 2024
View reviewed changes
examples/camera/first_person_view_model.rs Outdated
accumulated_mouse_motion: Res<AccumulatedMouseMotion>,
mut player: Query<&mut Transform, With<Player>>,
) {
let mut transform = player.single_mut();
let delta = accumulated_mouse_motion.delta;
let dt = time.delta_seconds();
// The factors are just arbitrary mouse sensitivity values.
Copy link
Member

Choose a reason for hiding this comment

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

Pull these out into constants please :)

Copy link
Member Author

@janhohenheim janhohenheim Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree. Pulling them into MOUSE_SENSITIVITY_X and MOUSE_SENSITIVITY_Y

  • makes them less nicely grouped than now, and
  • is semantically wrong. PI is a constant, an aspect ratio is a constant. Mouse sensitivity is a value that should be configurable for accessibility.

As a compromise, I could extract them into a component if you want. Or add a component mentioning that this should be configurable.

Copy link
Member

Choose a reason for hiding this comment

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

Component is nice: it makes it easy for the values to be twiddled via inspectors.

Copy link
Member Author

Choose a reason for hiding this comment

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

3e372d1 (#15109)

@alice-i-cecile
Copy link
Member

@richchurcher, if you're interested in helping out by reviewing, I'd welcome your opinions here :) Bevy uses community reviews to vet when things are ready to merge, so please leave an approval when you think this work is ready.

@janhohenheim
Copy link
Member Author

@NiseVoid thanks for pointing out that dt makes the mouse actually frame dependent in this case. I didn't consider that before, but it totally makes sense. Time to change all the code that works like this in my other projects :D
Addressed all review comments.

@janhohenheim janhohenheim 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 Sep 10, 2024
alice-i-cecile
alice-i-cecile reviewed Sep 10, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Sticking data on the Player component like this feels messy, but it does keep the example simpler. I'd prefer a dedicated CameraSensitivity component.

@janhohenheim
Copy link
Member Author

@alice-i-cecile yeah I was considering that as well and used the Player component in fear of bloat. But you're right, showing good ECS patterns is probably worth it here. Changed it :)

@janhohenheim @chompaa
Apply suggestions from code review
d3098c8 
Co-authored-by: Antony <antony.m.3012@gmail.com>
@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-Review Needs reviewer attention (from anyone!) to move forward labels Sep 10, 2024
bas-ie
bas-ie reviewed Sep 10, 2024
View reviewed changes
examples/camera/first_person_view_model.rs Outdated
// The camera has no way of knowing what direction was "forward" before landing in that extreme position,
// so the direction picked will for all intents and purposes be arbitrary.
// Another issue is that for mathematical reasons, the yaw will effectively be flipped when the pitch is at the extremes.
// To not run into these issues, we clamp the pitch to a safe range.
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
// To not run into these issues, we clamp the pitch to a safe range.
// To not run into these issues, we clamp the pitch to a safe range. (FRAC_PI_2 is just a constant representing half the value of π).

Feel free to ignore this, but my first glimpse of this value gave me a bit of a WTF moment 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point :) I changed the wording a bit: 2bc51d1 (#15109)

bas-ie
bas-ie approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@bas-ie bas-ie left a comment

Choose a reason for hiding this comment

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

All looks very familiar!

@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 4de67b5 Sep 10, 2024
26 checks passed
@janhohenheim janhohenheim mentioned this pull request Sep 10, 2024
Merged
@janhohenheim janhohenheim deleted the patch-1 branch September 10, 2024 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chompaa chompaa chompaa approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@bas-ie bas-ie bas-ie approved these changes

@NiseVoid NiseVoid Awaiting requested review from NiseVoid

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@janhohenheim @NiseVoid @alice-i-cecile @bas-ie @chompaa