Zoom: Set zoom FOV per-player using a player object property #6650

paramat · 2017-11-20T02:08:37Z

Zoom: Set zoom FOV per-player using a player object property

Remove player object property 'can zoom'.
Add player object property 'zoom fov'.
Remove clientside setting for 'zoom fov'.
Object property default is 15 degrees in creative mode, zoom disabled
in survival mode.

Needed due to zoom now loading and/or generating distant world
according to zoom FOV.

Update object properties serialisation version to 3.
///////////////

Merge minetest/minetest_game#1956 soon after this.

Together with lhofhansl's recent work this will allow a game to provide an item which enables a particular zoom FOV, which in turn allows a certain distance of world to be loaded in when using zoom.
For example a powerful telescope item that allows a zoom FOV of 1 degree, which is capable of loading (and/or generating) world 2000 nodes away.

A server mod can still disable zooming for a player by setting zoom FOV to 0. So the functionality of the previous 'can zoom' property is kept.

paramat · 2017-11-20T02:10:41Z

src/object_properties.cpp

@@ -109,7 +109,7 @@ void ObjectProperties::serialize(std::ostream &os) const
 	writeF1000(os, automatic_face_movement_max_rotation_per_sec);
 	os << serializeString(infotext);
 	os << serializeString(wield_item);
-	writeU8(os, can_zoom);
+	writeF1000(os, zoom_fov);


Is it ok to just change this and not move it? The comment for this section says to only add stuff to the bottom and not remove anything, but maybe that doesn't apply since we are breaking compatibility and 'can zoom' was added during 0.5.0dev.

This will cause an offset in the ObjectProperties' network packet (before: 8 bits, now: 32 bits)
Yes, we are in backwards compat breakage but nevertheless you should either raise the version number here from 2 to 3 or deprecate this single byte.

Raising version just for this seems excessive.
Maybe it's best to keep using 'can zoom' also.
By deprecation i assume you mean keep writing 'can zoom' but set it to a fixed bool and don't read it? I don't like the messiness of that.

SmallJoker · 2017-11-20T18:48:11Z

src/script/common/c_content.cpp

-	lua_pushboolean(L, prop->can_zoom);
-	lua_setfield(L, -2, "can_zoom");
+	lua_pushnumber(L, prop->zoom_fov);
+	lua_setfield(L, -2, "zoom_fov");


Needs documentation.

Will do, i always do docs last.

paramat · 2017-11-21T03:21:35Z

Not sure which of the 3 options to do:
Raise version 2>3.
Deprecate the byte (packets then have an unused byte).
Keep 'can zoom' and just add 'zoom fov'.

EDIT: I decided to raise version to 3.

ghost · 2017-11-21T15:47:34Z

Both seems reasonable, given that you may want an on/off as well as a zoom fov value. (Speaking of setting (zoom_)FOV during gameplay, is that possible?)

…

On Mon, Nov 20, 2017 at 10:21 PM, Paramat ***@***.***> wrote: Not sure which of the 3 options to do: Raise version 2>3. Deprecate the byte (packets then have an unused byte). Keep 'can zoom' and just add 'zoom fov'. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6650 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWXKYtLBHdcUQB2RAKnFIvlrv4pWqvydks5s4kHGgaJpZM4Qjp4l> .

paramat · 2017-11-22T15:28:35Z

Updated with version raised to 3.

paramat · 2017-11-24T06:12:31Z

Has a bug. I'm not actually changing m_cache_zoom_fov.

paramat · 2017-11-24T07:19:49Z

Ok fixed, retested and docs added, ready for review.

paramat · 2017-11-28T20:57:31Z

Tested with MTG PR.
Only thing i'm not confident about is my increasing of object property version number.

SmallJoker · 2017-12-01T08:13:52Z

src/camera.cpp

-	if (player->getPlayerControl().zoom && player->getCanZoom()) {
-		fov_degrees = m_cache_zoom_fov;
+	// Disable zoom with zoom FOV = 0
+	if (player->getPlayerControl().zoom && player->getZoomFOV() != 0.0f) {


perhaps change this to player->getZoomFOV() > 0.001f for floating point errors and invalid input from Lua-side (as negative values sometimes disable other features)

SmallJoker · 2017-12-01T08:20:21Z

src/object_properties.h

@@ -60,6 +59,7 @@ struct ObjectProperties
 	std::string wield_item;
 	bool static_save = true;
 	float eye_height = 1.625f;
+	float zoom_fov = 15.0f;


This means the server will force a zoom fov of 15 as soon the ObjectProperties are sent to the player. The client has no control about the limits.

Actually this struct does not set the defaults for properties, these values are overriden in content_sao, however currently to the wrong value (the server's zoom_fov setting).

But see my other line comments for a related problem.

paramat · 2017-12-01T20:21:27Z

Is the way i increased object property version correct?
I need to add a mention of this to network protocol, but any need to increase protocol number?

paramat · 2017-12-01T21:51:36Z

IRC:

Krock paramat, the object property version change was done correctly. the network version could be raised too, but is not required as we're breaking the backwards compatibility and clients will get an error when connecting to the server

paramat · 2017-12-02T03:19:57Z

Updated but more testing coming.

paramat · 2017-12-02T03:34:04Z

src/content_sao.cpp

 	m_hp = m_prop.hp_max;
 	m_breath = m_prop.breath_max;
+	// Allow the client to set a limited range of zoom FOV, a more powerful
+	// zoom can only be set by server mods.
+	m_prop.zoom_fov = rangelim(g_settings->getFloat("zoom_fov"), 15.0f, 160.0f);


Not sure about this, seems this will get the 'zoom_fov' of the server not the client.
My testing has been with 2 instances of MT, both with identical zoom_fov settings, which may have caused false testing results.

Problem is, when any property is set, all of these are set, which will then override the client's 'zoom_fov' setting. I need to work on this.

paramat · 2017-12-02T03:51:25Z

src/localplayer.h

-	bool getCanZoom() const { return m_can_zoom; }
-	void setCanZoom(bool can_zoom) { m_can_zoom = can_zoom; }
+	float getZoomFOV() const { return m_zoom_fov; }
+	void setZoomFOV(float zoom_fov) { m_zoom_fov = zoom_fov; }


So i think this should only be changed if the value is being altered.

No that won't work, the server has no idea what the client's local setting is.

paramat · 2017-12-02T04:35:36Z

This is getting more tricky the more i think about it.

paramat · 2017-12-02T04:44:38Z

The client's local zoomFOV setting is the problem and has to be removed, unfortunately. All other object properties are not client-settable which is why they avoid the problems i'm encountering.
This is probably for the best as zoom now loads and/or generates distant world, so should really be under full control of server mods, as zoom now gives players significant powers and advantages. If necessary a server mod can set zoomFOV per-player to their desired values.

paramat · 2017-12-02T04:56:26Z

Updated but in testing.

Remove player object property 'can zoom'. Add player object property 'zoom fov'. Remove clientside setting for 'zoom fov'. Object property default is 15 degrees in creative mode, zoom disabled in survival mode. Needed due to zoom now loading and/or generating distant world according to zoom FOV. Update object properties serialisation version to 3.

paramat · 2017-12-03T02:42:04Z

Updated and tested, ready for merge.
@SmallJoker

The client 'zoom fov' setting is removed. As zoom now loads and/or generated distant world beyond the server's send/generate distances, zoom FOV should be under server control (not client setting or CSM).
The default zoom FOV is 15 degrees in creative mode, disabled (using the value 0) in survival mode.
Server mods are needed for a zoom FOV other than 15 degrees.

SmallJoker · 2017-12-03T17:01:25Z

doc/lua_api.txt

+    --  ^ For players only. Zoom FOV in degrees.
+    --    Note that zoom loads and/or generates world beyond the server's maximum
+    --    send and generate distances, so acts like a telescope.
+    --    Smaller zoomFOV values increase the distance loaded and/or generated.


Not really necessary to provide this information, as people expect this to happen anyway.

Those used to MT don't expect it, so it's a warning about this powerful ability.

SmallJoker

LGTM, code and functionality-wise.

paramat · 2017-12-04T02:31:33Z

f470cb7

lhofhansl · 2017-12-11T06:13:37Z

So in creative mode a "zoom-tool" can still override the default 15 degrees zoom FOV, right?

paramat · 2017-12-11T07:21:46Z

Yes as in the binoculars mod:

function binoculars.update_player_property(player)
	local creative_enabled =
		(creative_mod and creative.is_enabled_for(player:get_player_name())) or
		creative_mode_cache
	local new_zoom_fov = 0

	if player:get_inventory():contains_item(
			"main", "binoculars:binoculars") then
		new_zoom_fov = 10
	elseif creative_enabled then
		new_zoom_fov = 15
	end

	-- Only set property if necessary to avoid player mesh reload
	if player:get_properties().zoom_fov ~= new_zoom_fov then
		player:set_properties({zoom_fov = new_zoom_fov})
	end
end

lhofhansl · 2017-12-20T01:48:49Z

Trying this a bit. I'm not a fan of not being to able to control the zoom in creative mode anymore (if just for testing higher zooms). In fact I think this should be a prominent config option (which can overridden by mods)

@paramat

paramat · 2017-12-20T14:22:21Z

You can set zoomFOV easily in creative with a 3 line mod. A tiny bit less convenient but no issue.

You mean you prefer clientside control of zoomFOV in creative mode? If so i disagree with that due to the power zoom now has, it may be creative mode but i don't think clients can have that power clientside, after all it loads distant terrain and overrides the server's own send and generate distances. The server should obviously have control over it.

It's like CSM, things that can significantly affect a server can't be clientside.

Servers can use a mod that gives players the ability to choose their own zoomFOV, or can set any zoomFOV they want for creative.
The clientside zoomFOV setting made everything complex, i had to remove it to make the feature work.

lhofhansl · 2017-12-24T11:39:59Z

I see. Client can still set the FOV (though there's a lower limit of 30). It seems inconsistent not to be able to set the zoom fov. The fact that there's perhaps more server load is a but specious; that applies to the normal fov as well, which can be set to 180 degrees, which causes about 3x the server load.

I get the point, though.

In that case I would prefer that there be absolutely no zoom by default (i.e. unless granted by some mod in some way, such as now done in MT game). The hardcoded 15 is definitely arbitrary.

paramat · 2017-12-24T16:05:35Z

I kept 15 for creative because that was the former default and i expected there would be complaints if it was disabled by default in creative, but i personally don't mind if it's disabled by default in creative mode.

paramat added @ Server / Client / Env. Feature ✨ PRs that add or enhance a feature labels Nov 20, 2017

paramat commented Nov 20, 2017

View reviewed changes

SmallJoker reviewed Nov 20, 2017

View reviewed changes

paramat added the WIP The PR is still being worked on by its author and not ready yet. label Nov 20, 2017

paramat removed the WIP The PR is still being worked on by its author and not ready yet. label Nov 24, 2017

paramat mentioned this pull request Nov 26, 2017

Binoculars: Update to use 'zoom_fov' player property minetest/minetest_game#1956

Closed

paramat mentioned this pull request Nov 30, 2017

[CSM] Add function to set the FOV of the local player #5797

Closed

SmallJoker reviewed Dec 1, 2017

View reviewed changes

paramat added the WIP The PR is still being worked on by its author and not ready yet. label Dec 1, 2017

paramat commented Dec 2, 2017

View reviewed changes

paramat removed the WIP The PR is still being worked on by its author and not ready yet. label Dec 3, 2017

SmallJoker reviewed Dec 3, 2017

View reviewed changes

SmallJoker approved these changes Dec 3, 2017

View reviewed changes

SmallJoker added the One approval ✅ ◻️ label Dec 3, 2017

paramat closed this Dec 4, 2017

paramat mentioned this pull request Dec 4, 2017

Set per-player zoom FOV using existing player object property #6561

Closed

paramat deleted the zoomfovpop branch December 18, 2017 22:06

Zoom: Set zoom FOV per-player using a player object property #6650

Zoom: Set zoom FOV per-player using a player object property #6650

Conversation

paramat commented Nov 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramat commented Nov 21, 2017 • edited

ghost commented Nov 21, 2017 via email

paramat commented Nov 22, 2017

paramat commented Nov 24, 2017 • edited

paramat commented Nov 24, 2017

paramat commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmallJoker Dec 1, 2017 • edited

Choose a reason for hiding this comment

paramat Dec 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramat commented Dec 1, 2017 • edited

paramat commented Dec 1, 2017

paramat commented Dec 2, 2017 • edited

paramat Dec 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramat commented Dec 2, 2017

paramat commented Dec 2, 2017 • edited

paramat commented Dec 2, 2017

paramat commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmallJoker left a comment • edited

Choose a reason for hiding this comment

paramat commented Dec 4, 2017

lhofhansl commented Dec 11, 2017

paramat commented Dec 11, 2017

lhofhansl commented Dec 20, 2017 • edited

paramat commented Dec 20, 2017 • edited

lhofhansl commented Dec 24, 2017

paramat commented Dec 24, 2017

paramat commented Nov 20, 2017 •

edited

paramat commented Nov 21, 2017 •

edited

paramat commented Nov 24, 2017 •

edited

SmallJoker Dec 1, 2017 •

edited

paramat Dec 2, 2017 •

edited

paramat commented Dec 1, 2017 •

edited

paramat commented Dec 2, 2017 •

edited

paramat Dec 2, 2017 •

edited

paramat commented Dec 2, 2017 •

edited

SmallJoker left a comment •

edited

lhofhansl commented Dec 20, 2017 •

edited

paramat commented Dec 20, 2017 •

edited