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

Fix camera to follow player eyes when player is attached #14231

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sfence
Copy link
Contributor

@sfence sfence commented Jan 8, 2024

To do

This PR is a Ready to Review

How to test

Run devtest game, add testentities:attach entity to the scene, and right-click to attach yourself to it.
Control keys are up, down, left, right, jump, sneak, and aux1. aux1 changes the mode of control.
To check get_look_dir, take the testlookdir:fire item and use the dig/place buttons to the fire entity in the direction of look_dir get from get_look_dir or calculated from get_look_vertical and get_look_horizontal.

You can detach this branch HEAD to one commit before commit "Fix camera position and rotation." to see how it works without a camera fix.

See before :
https://streamable.com/ih3lb5

And after:
https://streamable.com/tvdztt

@sfence
Copy link
Contributor Author

sfence commented Jan 8, 2024

@APercy This can be interesting for you and my rocket mod.

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Such a fix is much needed and useful, but I have some concerns about breakages, esp. since there are no changes to the Lua API.

Before this PR, the camera did not have roll, and even though the player visuals could be rotated, the eye position could always be found by adding eye height to the player pos. This PR changes that, and gives mods no convenient way to detect this or to find the new eye pos; mods would have to query attachments and compose transformations themselves.
(I'm unfortunately not sure whether we can expose a good server-side API, since the player might be attached to an animated model, which the server doesn't understand. But at least for the common special case where no bones are involved, we could make a decent effort.)

Calculating look dir from pitch and yaw will also be broken (there is no way around this when adding roll, but again, modders need to be able to deal with it.) Also, won't get_look_dir be broken (as in: was absolute, now is relative to attachment) too?

What it boils down to: Wouldn't this break firing a bow from the test entity, both in terms of finding the eye position and the direction the arrow should go? And if so, how can we best update the API such that modders can deal with this?

There also is a compromise that doesn't add roll (yet), where you would only apply yaw and attachment offsets. This would already be good enough for e.g. car mods.

src/client/camera.cpp Outdated Show resolved Hide resolved
@Zughy Zughy added the Bugfix 🐛 PRs that fix a bug label Jan 15, 2024
@sfence
Copy link
Contributor Author

sfence commented Jan 17, 2024

Works pretty well now until attachment rotation uses the roll (axis Z in vector) part of the rotation.
get_look_vertical and get_look_horizontal still unchanged.

@appgurueu appgurueu added the Breaking Change Changes an existing API behavior in an incompatible way label Jan 20, 2024
@appgurueu appgurueu self-requested a review January 20, 2024 17:57
@sfence
Copy link
Contributor Author

sfence commented Jan 22, 2024

So, now the matrixes are used to calculate positions and rotation in the way, how it Is done on the client side.
World relative pitch and yaw are calculated from the camera direction vector on the client side and sent to the server.
With this, pitch and yaw should work naturally on old and new clients on the same server.
Position returned by Lua minetest.get_pos respect attachments and rotations.

@sfence
Copy link
Contributor Author

sfence commented Jan 22, 2024

@appgurueu Is it a good idea to detect the player client protocol version in minetest.get_eye_pos and minetest.get_pos and return values in the old style for players playing on old clients?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Changes an existing API behavior in an incompatible way Bugfix 🐛 PRs that fix a bug @ Client / Controls / Input @ Client rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants