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

Support setting camera roll via lua #14333

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

Conversation

regulus79
Copy link

Goal

To make it possible to set the roll of the player camera through lua code

How it works

  • This PR adds two new functions to the lua api, namely player:set_look_roll(radians) and player:get_look_roll().

  • player.h now has a new property camera_roll which stores the current roll angle of the camera.

  • Two new functions are added to server.cpp/.h to assist in sending the function call to the client.

  • A new client/server code is also added to handle the setter function call, TOCLIENT_CAMERA_ROLL

  • The lua_api.md was updated accordingly.

To do

Maybe not in this PR, but sometime it would be good to enable interpolation, as Herowl suggested on the discord.

Status

This PR is Ready for Review.

How to test

Using worldedit:
//lua minetest.get_player_by_name("singleplayer"):set_look_roll(math.pi/2)

@Zughy Zughy added Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Feb 1, 2024
src/network/clientpackethandler.cpp Outdated Show resolved Hide resolved
src/network/clientpackethandler.cpp Outdated Show resolved Hide resolved
src/player.h Outdated Show resolved Hide resolved
src/script/lua_api/l_object.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_object.cpp Show resolved Hide resolved
regulus79 and others added 4 commits February 1, 2024 16:47
Co-authored-by: JosiahWI <41302989+JosiahWI@users.noreply.github.com>
Co-authored-by: JosiahWI <41302989+JosiahWI@users.noreply.github.com>
@appgurueu appgurueu added Concept approved Approved by a core dev: PRs welcomed! and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Feb 6, 2024
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.

Thanks for the PR. I'm giving this a concept approval because currently I don't see significant risk of breakage; this is a useful feature to have e.g. for immersive plane mods.

If we're adding camera roll, it might make sense to let the player inherit roll from whatever they're attached to (of course this would need to be opt-in, maybe via yet another attachment parameter, at which point set_attach should be refactored to use a table). What do you think?

Nitpicks:

  • / core::RADTODEG could be * core::DEGTORAD
  • I feel some blank lines are excessive
  • float vs f32 feels inconsistent (I prefer f32)
  • Nittiest of picks: m_camera_roll=roll; could use two spaces around the =

This PR looks fine, otherwise. I'm not sure about a separate packet for roll - why not extend the packet already used for yaw and pitch to support roll?

@epCode
Copy link

epCode commented Feb 7, 2024

Real quick thought: I tested this out and did some experimentation, and it works great that I've seen. But something that bothers me, which is not standard in normal fpv roll, is that when your view is rotated on the z axis (roll) your pitch and yaw (mostly pitch that bothers me) still rotate on the original axis.

Example: say I have set my roll to 90 degrees, if I move my head side to side I expect it to transform the camera on MY local axis, but instead what happens is it looks like I'm moving my head up and down.

This is unoptimal for if someone wanted to make an "upsidedown" sort of mod that flips your perspective. With this implementation, the controls would feel inverted.

Just a thought, I don't know if this is in the scope of this pr, and I know that x and y axis rotations don't effect each-other, so go figure.

Thanks for your patience!

@regulus79
Copy link
Author

I'm not sure about a separate packet for roll - why not extend the packet already used for yaw and pitch to support roll?

I agree that it would make sense to couple the roll with the pitch and yaw in one packet, but from what I can tell it appears that the player yaw and roll is sent through TOCLIENT_MOVE_PLAYER, which sends the pos, pitch, and yaw of the player every step.

Since the roll of the camera is not directly controlled by the player, I thought it would make more sense to send it through a different packet which is only called when the roll is updated. Of course, I may be wrong. Either way, it would not be too hard to bundle the roll with an existing packet if that would be more appropriate.

@JosiahWI
Copy link
Contributor

JosiahWI commented Feb 7, 2024

the roll of the camera is not directly controlled by the player

That sounds like a good argument for not putting the new field in the Player class.

@Zughy Zughy added Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR and removed Concept approved Approved by a core dev: PRs welcomed! labels Feb 7, 2024
@Zughy
Copy link
Member

Zughy commented Feb 7, 2024

(@appgurueu : you can't approve a PR directly, hence the changes. Unsubscribing)

@Support-hells-angels-bitch

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client rendering Feature ✨ PRs that add or enhance a feature @ Script API Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants