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

Allow setting custom third person front view camera offset #13686

Merged
merged 5 commits into from Oct 2, 2023

Conversation

grorp
Copy link
Member

@grorp grorp commented Jul 24, 2023

Inspired by #11965.

This PR adds a third argument called thirdperson_front to the set_eye_offset Lua API function. It sets the camera offset in third-person front view and defaults to thirdperson in order not to break backwards compatibility. This was suggested by @srifqi in #11965 (comment).

Because the thirdperson_front argument defaults to thirdperson, not to vector.new(thirdperson.x, thirdperson.y, -thirdperson.z), this PR effectively reverts #13369. I did that because #13369 theoretically broke backwards compatibility by changing the behavior of set_eye_offset, and this compatibility break is now no longer necessary.

^ This part is up for discussion.

To do

This PR is a Ready for Review.

How to test

Use the thirdperson_front argument of set_eye_offset.

Test some mods that use set_eye_offset. Verify that they behave the same as they do before this PR (i.e. that thirdperson_front falls back to thirdperson if not specified).

Join an old server, verify that the third person camera offset sent by the server is used in third person front view as well.

@lhofhansl
Copy link
Contributor

I'm obviously against reverting #13369.

@grorp
Copy link
Member Author

grorp commented Jul 28, 2023

I'm obviously against reverting #13369.

It wasn't obvious to me that you would be against that :) In that case, I'll change this PR to preserve the new behavior introduced by #13369.

@grorp
Copy link
Member Author

grorp commented Jul 28, 2023

In that case, I'll change this PR to preserve the new behavior introduced by #13369.

Done.

Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

It would be nice to also change thirdperson to thirdperson_back (and _third to _third_back etc.) instead of implying it for the back-side.

PS Thank you for creating this PR!

src/client/camera.cpp Show resolved Hide resolved
@lhofhansl
Copy link
Contributor

Missed the updates here... It just "obvious" because I had added #13369. :)

Lemme test this.

@srifqi srifqi added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 15, 2023
@grorp
Copy link
Member Author

grorp commented Aug 21, 2023

It would be nice to also change thirdperson to thirdperson_back (and _third to _third_back etc.) instead of implying it for the back-side.

That "third person" means "third person back view" goes quite far:

enum CameraMode {CAMERA_MODE_FIRST, CAMERA_MODE_THIRD, CAMERA_MODE_THIRD_FRONT};

I'd prefer not to change this everywhere. However, I did change thirdperson to thirdperson_back in lua_api.md, as you requested: f848584.

@grorp grorp requested a review from srifqi August 21, 2023 13:38
Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

Except for a single line, this PR looks good.

doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
@Zughy Zughy added One approval ✅ ◻️ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Aug 22, 2023
@grorp grorp requested a review from lhofhansl August 24, 2023 18:02
@rubenwardy rubenwardy added the Concept approved Approved by a core dev: PRs welcomed! label Sep 17, 2023
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Testing mod: (N°144)

local V = vector.new
local PP = minetest.pos_to_string

local views = {
	reset = {},
	[""] = {},

	-- (P1 view) up deplacement
	y_z = { V(0, 10, 0), V(0, 15, 0), V() },
	y_p = { V(0, 10, 0), V(0, 15, 0), V(0, 10, 0) },
	y_n = { V(0, -5, 0), V(0, -10, 0), V(0, -10, 0) },

	-- (P1 view) right deplacement
	x_z = { V(5, 0, 0), V(10, 0, 0), V() },
	x_p = { V(5, 0, 0), V(10, 0, 0), V(10, 0, 0) },
	x_n = { V(-5, 0, 0), V(-10, 0, 0), V(-10, 0, 0) },

	-- (P1 view) forward deplacement
	z_z = { V(0, 0, 5), V(0, 0, 5), V() },
	z_p = { V(0, 0, 5), V(0, 0, 5), V(0, 0, 5) },
	z_n = { V(0, 0, -5), V(0, 0, -5), V(0, 0, -5) },
}

minetest.register_chatcommand("view", {
	func = function(name, param)
		local player = minetest.get_player_by_name(name)
		local vals = views[param]
		if not vals then
			return false, "Unknown view type"
		end
		player:set_eye_offset(vals[1], vals[2], vals[3])
		local v1, v3b, v3f = player:get_eye_offset()
		print(PP(v1), PP(v3b), PP(v3f))
    end
})

x and z work as expected but negative y values cause a close-up of the character with negative values. This behaviour does however not seem to be caused by this PR.

src/script/lua_api/l_object.cpp Outdated Show resolved Hide resolved
Co-authored-by: SmallJoker <SmallJoker@users.noreply.github.com>
@sfan5 sfan5 merged commit 33cc29b into minetest:master Oct 2, 2023
13 checks passed
Zughy pushed a commit to Zughy/minetest that referenced this pull request Oct 8, 2023
…13686)

Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
Co-authored-by: SmallJoker <SmallJoker@users.noreply.github.com>
@grorp grorp deleted the third-person-front-offset branch December 18, 2023 16:37
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
…13686)

Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
Co-authored-by: SmallJoker <SmallJoker@users.noreply.github.com>
cosin15 pushed a commit to cosin15/minetest that referenced this pull request Feb 24, 2024
…13686)

Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
Co-authored-by: SmallJoker <SmallJoker@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature One approval ✅ ◻️ @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants