Allow zoom even with overwritten default FOV

[SmallJoker]

Fixes #12951

Zoom can still be disabled using the player object properties. I created a PR in case someone thinks that the set_fov value should be enforced (i.e. whether disabling zoom is intentional).

To do

This PR is Ready for Review.

How to test

Based on code from #12951

minetest.register_on_joinplayer(function(player)
	player:set_properties({
		zoom_fov = 60.0, -- enables zoom
	})
end)

minetest.register_chatcommand("p", {
	func = function(name, param)
		local player = minetest.get_player_by_name(name)
		-- the following line can be commented out to remove the message
		player:set_fov(tonumber(param) or 0, true, 0.1)
	end
})

[lhofhansl]

I do not feel strongly one way or the other. @paramat insisted back then to put this extra check in.

[SumianVoice]

Hmm, if I'm reading this correctly, this would completely remove set_fov's effect if a player's zoom_fov is being used, and then if you tried to remove the inbuilt zoom by setting zoom_fov to 0, the message would display because the zoom_fov == 0. That might cause some pretty serious issues.

Wouldn't it be better to allow set_fov to override the zoom_fov set, or multiply them?

Also example of a mod that allows setting of fov without conflicts; it uses a product of all existing set_fov requests. that could be used in the case of zoom_fov and set_fov, to just multiply them. That way you could set zoom_fov to some value other than 0 but close to the default fov and effectively remove the message while implementing your own system.

[SumianVoice]

Really, I'm not sure why the message actually exists; if the game supports zoom, it will work. Otherwise it won't zoom. It not zooming tells you the game doesn't support it?

If zoom_fov and set_fov gave a product instead of being only one or the other, then the message would be redundant; it's only because they are mutually exclusive that you might want to know that zoom is disabled. Think about it this way, if you set step_height to 0, the game doesn't tell you "Step height is currently disabled by the game or mod" whenever you try to walk up stairs.

[SmallJoker]

@SumianVoice If zoom is not wanted, mods can still set zoom_fov = 0 or scale this value depending on the mods' requirements.

For example the following combinations are currently possible in master:

• ✔️ No zoom + default FOV
• ✔️ No zoom + modified FOV
• ✔️ Allow zoom + default FOV
• ❎ Allow zoom + modified FOV

Now with the PR:

• ✔️ : No zoom + default FOV
• ✔️ No zoom + modified FOV
• ✔️ Allow zoom + default FOV
• ✔️ Allow zoom + modified FOV

[SumianVoice]

I guess if it's tested and works; it just seemed from reading the code that it does:

if player using zoom key and zoom fov > 0 then use that
else if set_fov is in effect then use that

And then later does

if zoom fov <= 0 then show the message

So although you can enable both at the same time, zoom fov key will override everything else, and if it doesn't override everything else, it shows the message. The original issue was noted when trying to remove the inbuilt zoom and implement it properly using set_fov; I tried to use zoom_fov = player's fov setting and use get_player_control().zoom to apply my zoom effect, but found out the message will appear. If it works like the pseudo code above, then mods would be unable to do that, and become incompatible with other mods if those mods set zoom_fov to anything other than 0

[SmallJoker]

(It's been a while and I forgot about the matter)

Have you tested the PR or is the answer based on static analysis only? Mods can still overwrite both FOV's (zoom and normal) by overwriting both values. Would you please be so nice to provide a short Lua snippet to demonstrate the issue you're facing?

[SumianVoice]

I can't build from source for some unknown reason / error unfortunately otherwise I'd have a shot at testing different things. So yeah it's all from reading the code.

minetest.register_on_joinplayer(function(player)
	player:set_properties({
		zoom_fov = 70.0, -- enables zoom
	})
end)

minetest.register_chatcommand("p", {
	func = function(name, param)
		local player = minetest.get_player_by_name(name)
		player:set_fov(tonumber(param) or 0, true, 0.2)
	end
})

If I read the code correctly, this would result in the player:set_fov having no effect while inbuilt zoom is active, meaning that if one mod uses the in-built zoom, it will break other mods that use player:set_zoom. So if a mod uses set_fov for an effect, the player can just overwrite it by pressing the zoom key, meaning the only way for a mod to reliably give the player a certain FOV is to disable the inbuilt zoom with set props every time, check it constantly and then remember its value and re-enable it later.

The best solution would be to use the product of both FOV values. This is one way:

    } else if (player->getPlayerControl().zoom && player->getZoomFOV() > 0.001f) {
        // Player requests zoom, apply zoom FOV
        m_curr_fov_degrees = player->getZoomFOV();
        // For simultaneous use of both fov factors
        if (m_server_sent_fov) {
            m_curr_fov_degrees *= m_target_fov_degrees
        }

That would allow both inbuilt zoom and set_fov to function together instead of the engine choosing between one or the other and creating incompatibilities.

I don't know if I'm making sense here, but hopefully that explains my thought process.

[SmallJoker]

So if a mod uses set_fov for an effect, the player can just overwrite it by pressing the zoom key

If I understand you correctly, you'd like to use set_fov to disable the buitin zoom effect (i.e. zoom_fov property) temporarily. This means that #12951 is technically correct because you cannot use zoom while it is overwritten.
What if a mod wants to use set_fov and allow zooming? This PR would now allow this case; which in turn requires mods to disable the zoom_fov property if they want to disable the normal zoom feature.